Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2302433002:
[NoStatePrefetch] Track redirects in UMA (Closed)
Description[NoStatePrefetch] Track redirects in UMA
BUG=642371
Committed: https://crrev.com/88d0480c2a22d6fcd74bce2fa621ac0a1ff33385
Cr-Commit-Position: refs/heads/master@{#416907}
Patch Set 1 #Patch Set 2 : Add histograms for redirect count #
Total comments: 14
Patch Set 3 : review comments #
Total comments: 8
Patch Set 4 : review comments #
Total comments: 5
Patch Set 5 : add units #Patch Set 6 : Rebase #
Messages
Total messages: 35 (13 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== [NoStatePrefetch] Track redirects in UMA ========== to ========== [NoStatePrefetch] Track redirects in UMA BUG=642371 ==========
droger@chromium.org changed reviewers: + mattcary@chromium.org, pasko@chromium.org
This is blocked on other CLs, but FYI this is one possible implementation for tracking redirects in NoStatePrefetch. This tracks individual redirects, as opposed to whole redirect chains, mostly because the implementation is much more straightforward that way.
On 2016/08/31 10:53:34, droger wrote: > This is blocked on other CLs, but FYI this is one possible implementation for > tracking redirects in NoStatePrefetch. > > This tracks individual redirects, as opposed to whole redirect chains, mostly > because the implementation is much more straightforward that way. I would think that counting requests-with-any-redirects would be the most interpretable, but that would require counting redirect chains. Is there any simpler way to do it than something like the following: * keep global dict of request chains * on each redirect, see if we've started a new chain or if we're in an existing one * on WillProcessResponse remove the request chain from the dict, and write to the histogram. That would be a groovy stat to have but I have to concur that it might be too expensive to be worth it.
On 2016/08/31 11:40:46, mattcary wrote: > On 2016/08/31 10:53:34, droger wrote: > > This is blocked on other CLs, but FYI this is one possible implementation for > > tracking redirects in NoStatePrefetch. > > > > This tracks individual redirects, as opposed to whole redirect chains, mostly > > because the implementation is much more straightforward that way. > > I would think that counting requests-with-any-redirects would be the most > interpretable, but that would require counting redirect chains. Is there any > simpler way to do it than something like the following: > > * keep global dict of request chains > * on each redirect, see if we've started a new chain or if we're in an existing > one > * on WillProcessResponse remove the request chain from the dict, and write to > the histogram. > > That would be a groovy stat to have but I have to concur that it might be too > expensive to be worth it. I can definitely collect information about redirect chains. What kind of info do we want? In particular, do we care about the no-store status for redirect chains? Should the histogram about redirect chain be in addition or instead of this CL?
On 2016/08/31 11:40:46, mattcary wrote: > Is there any > simpler way to do it than something like the following: It's very simple to have the redirect count associated to a request (URLRequest has a url_chain() method that lists all the URLs in the redirect chain). However, these are only the URLs. If we want more information on the redirects, such as http headers, we have to do something like you suggested.
On 2016/08/31 12:57:56, droger wrote: > On 2016/08/31 11:40:46, mattcary wrote: > > Is there any > > simpler way to do it than something like the following: > > It's very simple to have the redirect count associated to a request (URLRequest > has a url_chain() method that lists all the URLs in the redirect chain). > However, these are only the URLs. If we want more information on the redirects, > such as http headers, we have to do something like you suggested. That's probably enough. I just think we want to distinguish between 5 requests each with a redirect, and 5 requests, 4 of which were direct and the last with a chain of 6 redirects.
I added two more histograms to record the length of the redirect chain (one for main resources, one for sub resources).
lgtm
sorry for the delayed review :( https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.cc:40: return static_cast<NoStatePrefetchResourceType>( This was confusing to me: * casting explicitly to the enum that gets casted back when FactoryGet is called * not using the values declared in the enum except one I think the code will be more straightforward if we return int32_t (or perhaps base::HistogramBase::Sample?) This would be equivalent, but would better suggest that we are using bit fields: sample = is_main_resource << 2 | is_redirect << 1 | is_no_store; kPossibleSampleValues = 1 << 3; https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.cc:440: // Unrolls UMA_HISTOGRAM_CUSTOM_COUNTS, required to support dynamic histogram can we avoid repeating ourselves twice here? void PrerenderHistograms::RecordEnumHistogram(std::sting name, base::HistogramBase::Sample sample, base::HistogramBase::Sample possible_value_count) { DCHECK(thread_checker_.CalledOnValidThread()); base::LinearHistogram::FactoryGet(name, 1, possible_value_count, possible_value_count + 1, base::HistogramBase::kUmaTargetedHistogramFlag)->Add(sample); } https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_histograms.h (right): https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.h:113: void RecordResourcePrefetch(Origin origin, Seems like a more self-descriptivename would be: RecordPrefetchResponseReceived https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:31: bool IsNoStoreResponse(net::URLRequest* request) { would it be possible to pass the argument by const reference, as recommended in the style guide? https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:88: IsNoStoreResponse(request_), request_->url_chain().size() - 1, Even though size of the chain is unlikely to be zero here, I want to be on the safe side with this unsigned to signed conversion how about: base::saturated_cast<int32_t>(request_->url_chain().size()), and having int32_t everywhere after that just as base::HistogramBase::Sample? https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:158: content::IsResourceTypeFrame(resource_type), true /*is_redirect*/, most code I see surrounds the parameter name with spaces, e.g.: /* is_redirect */ Also aesthetically this looks nicer to me. https://codereview.chromium.org/2302433002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2302433002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:87821: <enum name="NoStatePrefetchResourceType" type="int"> this is used for a histogram called "Prerender.NoStatePrefetchResourceTypes"? I think I mistakenly assumed in another change that this histogram is recorded once per resource (i.e. once per redirect chain), but in a closer look I am seeing these counts would be recorded once at having the 1st byte of the last request in the chain and once at every redirect. Is that right? In this case the I'd like to take my words back .. this name should reflect counts and probably should be rephrased in terms of URL requests and not resources, right?
Patchset #3 (id:80001) has been deleted
Thanks for the review. https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.cc:40: return static_cast<NoStatePrefetchResourceType>( On 2016/09/02 16:34:48, pasko wrote: > This was confusing to me: > * casting explicitly to the enum that gets casted back when FactoryGet is called > * not using the values declared in the enum except one > > I think the code will be more straightforward if we return int32_t (or perhaps > base::HistogramBase::Sample?) > > This would be equivalent, but would better suggest that we are using bit fields: > > sample = is_main_resource << 2 | is_redirect << 1 | is_no_store; > kPossibleSampleValues = 1 << 3; Done using a bitfield. https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.cc:440: // Unrolls UMA_HISTOGRAM_CUSTOM_COUNTS, required to support dynamic histogram On 2016/09/02 16:34:48, pasko wrote: > can we avoid repeating ourselves twice here? > > void PrerenderHistograms::RecordEnumHistogram(std::sting name, > base::HistogramBase::Sample sample, base::HistogramBase::Sample > possible_value_count) { > DCHECK(thread_checker_.CalledOnValidThread()); > base::LinearHistogram::FactoryGet(name, 1, possible_value_count, > possible_value_count + 1, > base::HistogramBase::kUmaTargetedHistogramFlag)->Add(sample); > } Done. https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_histograms.h (right): https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_histograms.h:113: void RecordResourcePrefetch(Origin origin, On 2016/09/02 16:34:48, pasko wrote: > Seems like a more self-descriptivename would be: RecordPrefetchResponseReceived Done. https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:31: bool IsNoStoreResponse(net::URLRequest* request) { On 2016/09/02 16:34:48, pasko wrote: > would it be possible to pass the argument by const reference, as recommended in > the style guide? Done. https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:88: IsNoStoreResponse(request_), request_->url_chain().size() - 1, On 2016/09/02 16:34:48, pasko wrote: > Even though size of the chain is unlikely to be zero here, I want to be on the > safe side with this unsigned to signed conversion > > how about: > base::saturated_cast<int32_t>(request_->url_chain().size()), > > and having int32_t everywhere after that just as base::HistogramBase::Sample? Done. https://codereview.chromium.org/2302433002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:158: content::IsResourceTypeFrame(resource_type), true /*is_redirect*/, On 2016/09/02 16:34:48, pasko wrote: > most code I see surrounds the parameter name with spaces, e.g.: > > /* is_redirect */ > > Also aesthetically this looks nicer to me. Done. https://codereview.chromium.org/2302433002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2302433002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:87821: <enum name="NoStatePrefetchResourceType" type="int"> On 2016/09/02 16:34:48, pasko wrote: > this is used for a histogram called "Prerender.NoStatePrefetchResourceTypes"? I > think I mistakenly assumed in another change that this histogram is recorded > once per resource (i.e. once per redirect chain), but in a closer look I am > seeing these counts would be recorded once at having the 1st byte of the last > request in the chain and once at every redirect. Is that right? You're right. Before this CL it was called once per resource, but now it is called once per response (including redirects). > > In this case the I'd like to take my words back .. this name should reflect > counts and probably should be rephrased in terms of URL requests and not > resources, right? Sure, renamed by changing *ResourceType into *ResponseType everywhere.
almost there, just a few more things that are easy to change https://codereview.chromium.org/2302433002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2302433002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:38: // Records a sample such as 0 <= sample < max_value, in a histogram with nit: I find it slightly inconvenient that all possible values are always strictly less than something called "max_value". Tiny better naming would be "possible_samples", "sample_buckets"... https://codereview.chromium.org/2302433002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2302433002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_resource_throttle.cc:88: base::saturated_cast<int>(request_->url_chain().size() - 1); size() returns an unsigned int. Suppose it returns zero, then by subtracting 1 we would have an UINT_MAX, which after saturated_cast turns into INT_MAX, I believe (modulo some weird platforms). I wanted it to become a -1 and let the histogram code deal with it (I think they have underflow and overflow buckets, and underflow would be easier to read on the dashboard). This would be achievable with: int redirect_count = base::saturated_cast<int>(request_->url_chain().size()) - 1; WDYT? https://codereview.chromium.org/2302433002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2302433002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45409: + Length of the redirect chain for main resources loaded by NoStatePrefetch. We should specify: "Recorded when the final response in the chain is received." https://codereview.chromium.org/2302433002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45429: + Length of the redirect chain for sub-resources loaded by NoStatePrefetch. Please mention the time of recording here as well.
https://codereview.chromium.org/2302433002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2302433002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_histograms.cc:38: // Records a sample such as 0 <= sample < max_value, in a histogram with On 2016/09/05 14:39:48, pasko wrote: > nit: I find it slightly inconvenient that all possible values are always > strictly less than something called "max_value". Tiny better naming would be > "possible_samples", "sample_buckets"... Done. https://codereview.chromium.org/2302433002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2302433002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_resource_throttle.cc:88: base::saturated_cast<int>(request_->url_chain().size() - 1); On 2016/09/05 14:39:48, pasko wrote: > size() returns an unsigned int. Suppose it returns zero, then by subtracting 1 > we would have an UINT_MAX, which after saturated_cast turns into INT_MAX, I > believe (modulo some weird platforms). > > I wanted it to become a -1 and let the histogram code deal with it (I think they > have underflow and overflow buckets, and underflow would be easier to read on > the dashboard). This would be achievable with: > > int redirect_count = base::saturated_cast<int>(request_->url_chain().size()) - > 1; > > WDYT? Good catch. Of course. That's what I wanted to do, but misplaced the bracket. https://codereview.chromium.org/2302433002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2302433002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45409: + Length of the redirect chain for main resources loaded by NoStatePrefetch. On 2016/09/05 14:39:48, pasko wrote: > We should specify: "Recorded when the final response in the chain is received." Done. https://codereview.chromium.org/2302433002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45429: + Length of the redirect chain for sub-resources loaded by NoStatePrefetch. On 2016/09/05 14:39:48, pasko wrote: > Please mention the time of recording here as well. Done.
lgtm, thank you!
droger@chromium.org changed reviewers: + asvitkine@chromium.org, mmenke@chromium.org
+mmenke as OWNER of prerender +asvitkine as OWNER of histograms.xml
On 2016/09/05 15:30:54, droger wrote: > +mmenke as OWNER of prerender > +asvitkine as OWNER of histograms.xml LGTM, rubberstamp, deferring to pasko.
https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45404: -<histogram name="Prerender.NoStatePrefetchResourceTypes" Instead of deleting the previous metric, please mark it as <obsolete>. This way, old data can continue to be viewed on the dashboard.
Thanks for the reviews. https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45404: -<histogram name="Prerender.NoStatePrefetchResourceTypes" On 2016/09/06 19:50:23, Alexei Svitkine (slow) wrote: > Instead of deleting the previous metric, please mark it as <obsolete>. This way, > old data can continue to be viewed on the dashboard. This metric is actually very recent (not even landed yet!) so nobody is looking at it and no data has been collected.
lgtm % comments https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45404: -<histogram name="Prerender.NoStatePrefetchResourceTypes" On 2016/09/06 20:43:51, droger wrote: > On 2016/09/06 19:50:23, Alexei Svitkine (slow) wrote: > > Instead of deleting the previous metric, please mark it as <obsolete>. This > way, > > old data can continue to be viewed on the dashboard. > > This metric is actually very recent (not even landed yet!) so nobody is looking > at it and no data has been collected. Okay, if it was never logged, then I'm fine with this. Thanks for clarifying. https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45404: +<histogram name="Prerender.NoStatePrefetchMainResourceRedirects"> Please add units="" e.g. units="redirects" https://codereview.chromium.org/2302433002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45426: +<histogram name="Prerender.NoStatePrefetchSubResourceRedirects"> Ditto.
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattcary@chromium.org, pasko@chromium.org, asvitkine@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2302433002/#ps140001 (title: "add units")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, asvitkine@chromium.org, mattcary@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2302433002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [NoStatePrefetch] Track redirects in UMA BUG=642371 ========== to ========== [NoStatePrefetch] Track redirects in UMA BUG=642371 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [NoStatePrefetch] Track redirects in UMA BUG=642371 ========== to ========== [NoStatePrefetch] Track redirects in UMA BUG=642371 Committed: https://crrev.com/88d0480c2a22d6fcd74bce2fa621ac0a1ff33385 Cr-Commit-Position: refs/heads/master@{#416907} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/88d0480c2a22d6fcd74bce2fa621ac0a1ff33385 Cr-Commit-Position: refs/heads/master@{#416907} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
