Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23483002:
UMA Data entries for SiteIsolation are added to histograms.xml. (Closed)
|
Created:
7 years, 3 months ago by dsjang Modified:
7 years, 3 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
DescriptionUMA Data entries for SiteIsolation are added to histograms.xml.
BUG=268640
R=asvitkine@chromium.org, creis@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219860
Patch Set 1 #
Total comments: 10
Patch Set 2 : Choose better names for enum types #
Total comments: 4
Patch Set 3 : Comments are more properly formatted #
Total comments: 6
Patch Set 4 : Units and comments are revised. #
Total comments: 6
Patch Set 5 : Comments are revised. #
Total comments: 2
Patch Set 6 : Comments are updated. #
Total comments: 4
Patch Set 7 : Problems in comments are corrected. #
Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:14666: + identified as illegal cross-site. Do we know it's illegal yet? Isn't it "potentially an illegal cross-site document"? https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:14672: + The count of blocked responses due to having HTML content type header and blocked cross-site document responses (Please spell out "cross-site document responses" in all the stats below, since it's not obvious what XSD means when someone is browsing the histogram data.) https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:14796: + responses due to their Plain.HTML contents. Please add something about the count being broken down by the type of resource that requested it (here and in the other places that use this enum). https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:21021: +<enum name="MimeTypeCode" type="int"> It would help to convey what this is used for in the name, like SiteIsolationMimeType. https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:22414: +<enum name="ResourceTypeCode" type="int"> SiteIsolationResourceType
https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:14666: + identified as illegal cross-site. On 2013/08/26 22:32:13, creis wrote: > Do we know it's illegal yet? Isn't it "potentially an illegal cross-site > document"? Done. https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:14672: + The count of blocked responses due to having HTML content type header and On 2013/08/26 22:32:13, creis wrote: > blocked cross-site document responses > > (Please spell out "cross-site document responses" in all the stats below, since > it's not obvious what XSD means when someone is browsing the histogram data.) Done. https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:14796: + responses due to their Plain.HTML contents. On 2013/08/26 22:32:13, creis wrote: > Please add something about the count being broken down by the type of resource > that requested it (here and in the other places that use this enum). Done. https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:21021: +<enum name="MimeTypeCode" type="int"> On 2013/08/26 22:32:13, creis wrote: > It would help to convey what this is used for in the name, like > SiteIsolationMimeType. Done. https://codereview.chromium.org/23483002/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:22414: +<enum name="ResourceTypeCode" type="int"> On 2013/08/26 22:32:13, creis wrote: > SiteIsolationResourceType Done.
LGTM with nits. You'll need to add an owner from tools/metrics to review. https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14688: + their requesting context type(e.g., image, script, etc.) among blocked nit: space before open parenthesis (here and below) https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14782: + MIME type codes for content type header values of responses. responses -> potentially cross-site document responses (This doesn't include same-site or binary responses, right? Let's be specific to make sure the data isn't misinterpreted.)
UMA Data entries for the cross-site document blocking policy of SiteIsolation are added to histograms.xml. This is for collecting data needed to measure the compatibility impact caused by an illegal network response blocker (/content/child/site_isolation_policy.cc). More detailed information is found on https://codereview.chromium.org/23483002/ The UMA collection for this feature is temporary. After we gather enough data(taking about 2 weeks), we might not need the UMA entries. https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14688: + their requesting context type(e.g., image, script, etc.) among blocked On 2013/08/26 23:13:02, creis wrote: > nit: space before open parenthesis (here and below) Done. https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14782: + MIME type codes for content type header values of responses. On 2013/08/26 23:13:02, creis wrote: > responses -> potentially cross-site document responses > > (This doesn't include same-site or binary responses, right? Let's be specific > to make sure the data isn't misinterpreted.) Done.
On 2013/08/27 00:34:09, dsjang wrote: > UMA Data entries for the cross-site document blocking policy of SiteIsolation > are added to histograms.xml. > > This is for collecting data needed to measure the compatibility impact caused by > an illegal network response blocker (/content/child/site_isolation_policy.cc). > More detailed information is found on https://codereview.chromium.org/23483002/ > > The UMA collection for this feature is temporary. After we gather enough > data(taking about 2 weeks), we might not need the UMA entries. Well, we'll want them at least long enough to get data from the beta and stable channels, which will be more than 2 weeks. Anyway, asvitkine, can you take a look for owner's approval? > > https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... > tools/metrics/histograms/histograms.xml:14688: + their requesting context > type(e.g., image, script, etc.) among blocked > On 2013/08/26 23:13:02, creis wrote: > > nit: space before open parenthesis (here and below) > > Done. > > https://codereview.chromium.org/23483002/diff/5001/tools/metrics/histograms/h... > tools/metrics/histograms/histograms.xml:14782: + MIME type codes for content > type header values of responses. > On 2013/08/26 23:13:02, creis wrote: > > responses -> potentially cross-site document responses > > > > (This doesn't include same-site or binary responses, right? Let's be specific > > to make sure the data isn't misinterpreted.) > > Done.
See my comments below. By the way, in the future, please include the histograms.xml changes in the same CL you're adding the histograms. https://codereview.chromium.org/23483002/diff/4001/tools/metrics/histograms/h... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/4001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14587: + corresponding to one URL requested by a renderer. Here and below: Please mention how this is sampled. Is it sampled every time there is a network response (i.e. if there are 6 responses, we get 6 histogram calls with values 1,2,3,4,5,6) or something else? For each histogram, please mention how/when it is sampled. https://codereview.chromium.org/23483002/diff/4001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14663: +<histogram name="SiteIsolation.XSD.DataLength"> Add unit="bytes" here. https://codereview.chromium.org/23483002/diff/4001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14677: +<histogram name="SiteIsolation.XSD.HTML.Blocked.NonRenderableStatusCode"> Please consider using a <fieldtrial> tag to permute these histograms that have suffixes like "NonRenderableStatusCode". See comment at the top of this file for an example. You can have multiple fieldtrial constructs if you have multiple levels of suffixes (i.e. Blocked and RenderableStatusCode).
https://codereview.chromium.org/23483002/diff/4001/tools/metrics/histograms/h... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/4001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14587: + corresponding to one URL requested by a renderer. On 2013/08/27 13:59:59, Alexei Svitkine wrote: > Here and below: > > Please mention how this is sampled. Is it sampled every time there is a network > response (i.e. if there are 6 responses, we get 6 histogram calls with values > 1,2,3,4,5,6) or something else? > > For each histogram, please mention how/when it is sampled. Done. https://codereview.chromium.org/23483002/diff/4001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14663: +<histogram name="SiteIsolation.XSD.DataLength"> On 2013/08/27 13:59:59, Alexei Svitkine wrote: > Add unit="bytes" here. Done. https://codereview.chromium.org/23483002/diff/4001/tools/metrics/histograms/h... tools/metrics/histograms/histograms.xml:14677: +<histogram name="SiteIsolation.XSD.HTML.Blocked.NonRenderableStatusCode"> I tried to use fieldtrials, but the current naming scheme has to be changed to apply this smoothly, and already some data has been collected under the current naming scheme. On 2013/08/27 13:59:59, Alexei Svitkine wrote: > Please consider using a <fieldtrial> tag to permute these histograms that have > suffixes like "NonRenderableStatusCode". See comment at the top of this file for > an example. > > You can have multiple fieldtrial constructs if you have multiple levels of > suffixes (i.e. Blocked and RenderableStatusCode).
https://codereview.chromium.org/23483002/diff/15001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/15001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14667: + that imply potential illegal cross-site access. Incremented when the first Nit: "Incremented" -> "Recorded", since this is the number of bytes. https://codereview.chromium.org/23483002/diff/15001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14672: +<histogram name="SiteIsolation.XSD.HTML.Blocked"> This sounds like a Boolean-style histogram (i.e. always records the value 1). I suggest adding a new BooleanBlocked enum type (similar to e.g. BooleanHit or BooleanAttempted) and marking all of these types of histograms to use that type. I guess also add a BooleanNotBlocked as well. Ideally, these should have been combined into the same histogram (i.e. value 0 to mean NotBlocked and value 1 to mean Blocked), but like you said you've already committed them like that before this review. :\ https://codereview.chromium.org/23483002/diff/15001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14971: + packet of any response is received. Nit: I am guessing this should "of this type of response is received", rather "of any response is received". Please fix throughout.
https://codereview.chromium.org/23483002/diff/15001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/15001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14667: + that imply potential illegal cross-site access. Incremented when the first On 2013/08/27 20:10:44, Alexei Svitkine wrote: > Nit: "Incremented" -> "Recorded", since this is the number of bytes. Done. https://codereview.chromium.org/23483002/diff/15001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14672: +<histogram name="SiteIsolation.XSD.HTML.Blocked"> I'd love to revise the naming scheme as you suggested, but as you said, we would need the already collected data. I'll keep the idea of designing a better naming scheme for future uses of histogram. On 2013/08/27 20:10:44, Alexei Svitkine wrote: > This sounds like a Boolean-style histogram (i.e. always records the value 1). > > I suggest adding a new BooleanBlocked enum type (similar to e.g. BooleanHit or > BooleanAttempted) and marking all of these types of histograms to use that type. > > I guess also add a BooleanNotBlocked as well. Ideally, these should have been > combined into the same histogram (i.e. value 0 to mean NotBlocked and value 1 to > mean Blocked), but like you said you've already committed them like that before > this review. :\ https://codereview.chromium.org/23483002/diff/15001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14971: + packet of any response is received. On 2013/08/27 20:10:44, Alexei Svitkine wrote: > Nit: I am guessing this should "of this type of response is received", rather > "of any response is received". Please fix throughout. Done.
lgtm, but please address my comment below https://codereview.chromium.org/23483002/diff/20001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/20001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14971: + packet of a response of this type is received. Sorry, given that you can't express this as a bool enum, please do mention the "value of 1" bit that I told you to remove, throughout.
https://codereview.chromium.org/23483002/diff/20001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/20001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14971: + packet of a response of this type is received. On 2013/08/27 20:57:04, Alexei Svitkine wrote: > Sorry, given that you can't express this as a bool enum, please do mention the > "value of 1" bit that I told you to remove, throughout. Done.
Thanks for the feedback, Alexei. We'll take the other suggestions into account for future CLs. https://codereview.chromium.org/23483002/diff/15002/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/15002/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14694: + resource type(0-15) when the first network packet of a response of this type nit: Please always put a space before an open parenthesis, so that it doesn't look like a function call. https://codereview.chromium.org/23483002/diff/15002/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14801: + document responses, excluding same-site or not http(s) urls. Incremented Shouldn't this be "Sampled with a MIME type (0-4)" to be consistent with your other descriptions?
https://codereview.chromium.org/23483002/diff/15002/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/23483002/diff/15002/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14694: + resource type(0-15) when the first network packet of a response of this type On 2013/08/27 22:05:36, creis wrote: > nit: Please always put a space before an open parenthesis, so that it doesn't > look like a function call. Done. https://codereview.chromium.org/23483002/diff/15002/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:14801: + document responses, excluding same-site or not http(s) urls. Incremented On 2013/08/27 22:05:36, creis wrote: > Shouldn't this be "Sampled with a MIME type (0-4)" to be consistent with your > other descriptions? Done.
Message was sent while issue was closed.
Committed patchset #7 manually as r219860 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
