Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644543002:
Add counter for number of page loads where doc.write blocking triggers. (Closed)
|
Created:
3 years, 11 months ago by Bryan McQuade Modified:
3 years, 11 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, Charlie Harrison Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd counter for number of page loads where doc.write blocking triggers.
When doing logs analysis for doc.write blocking, it's desirable to
analyze only cases where doc.write blocking could have triggered, to
minimize noise from samples where the feature couldn't have triggered.
This is especially important given that the blocking only triggers on
~7% of page loads when on 2G.
This change adds a counter that allows us to keep track of cases where
the feature could have triggered, in a way that's comparable across
control and experiment populations.
BUG=682220
Review-Url: https://codereview.chromium.org/2644543002
Cr-Commit-Position: refs/heads/master@{#444519}
Committed: https://chromium.googlesource.com/chromium/src/+/6cc8974e18028a1e18779c36318acfe5c069ec2c
Patch Set 1 #Patch Set 2 : fix broken code #
Total comments: 7
Patch Set 3 : mark constructor default #Patch Set 4 : switch to UMA_HISTOGRAM_BOOLEAN. #
Messages
Total messages: 33 (23 generated)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add counter for number of page loads where doc.write blocking triggers. BUG= ========== to ========== Add counter for number of page loads where doc.write blocking triggers. BUG=682220 ==========
Description was changed from ========== Add counter for number of page loads where doc.write blocking triggers. BUG=682220 ========== to ========== Add counter for number of page loads where doc.write blocking triggers. When doing logs analysis for doc.write blocking, it's desirable to analyze only cases where doc.write blocking could have triggered, to minimize noise from samples where the feature couldn't have triggered. This is especially important given that the blocking only triggers on ~7% of page loads when on 2G. This change adds a counter that allows us to keep track of cases where the feature could have triggered, in a way that's comparable across control and experiment populations. BUG=682220 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bmcquade@chromium.org changed reviewers: + jkarlin@chromium.org, rkaplow@chromium.org
jkarlin, PTAL for page_load_metrics rkaplow, PTAL for histograms.xml Thanks!
https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:135: UMA_HISTOGRAM_COUNTS(internal::kHistogramDocWriteBlockCount, 1); I don't understand. How does this give you any more data in the control population than the FCP metric? https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h (right): https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: bool doc_write_block_observed_; Can you = false this here, and do the same for the bool below?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Addressed comments. https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:135: UMA_HISTOGRAM_COUNTS(internal::kHistogramDocWriteBlockCount, 1); On 2017/01/18 at 14:07:31, jkarlin wrote: > I don't understand. How does this give you any more data in the control population than the FCP metric? Good question. This is covered briefly in the bug but is somewhat subtle, so here's some more detail: The FCP metric is only recorded if a page encounters a blocking script, and then reaches FCP. Pages in the experiment population are more likely to reach FCP as a result of encountering a blocking script than those in the control, since in experiment the page loads faster as a result of blocking the script. Thus we encounter cases in experiment where doc.write FCP is recorded, but wouldn't have been in control, as the user would have given up on the page load before FCP was reached. This means that we can't use the presence of the FCP metric as a way to drill down on the population of 'affected' users between control and experiment in a way that makes those populations comparable across control and experiment. This change adds a simple counter that is recorded as soon as we discover a blocking script. The presence of this counter allows us to know that the user was affected by blocking (in experiment) or would have been affected by blocking (in control) in a way that is comparable across populations for analysis purposes. Hope that helps to explain. Happy to discuss more. https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h (right): https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: bool doc_write_block_observed_; On 2017/01/18 at 14:07:31, jkarlin wrote: > Can you = false this here, and do the same for the bool below? Sure. I do prefer this to setting it in the constructor, but do you know if this is in the style guide? Would be useful for me to know for future code reviews.
lgtm https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:135: UMA_HISTOGRAM_COUNTS(internal::kHistogramDocWriteBlockCount, 1); On 2017/01/18 14:27:32, Bryan McQuade wrote: > On 2017/01/18 at 14:07:31, jkarlin wrote: > > I don't understand. How does this give you any more data in the control > population than the FCP metric? > > Good question. This is covered briefly in the bug but is somewhat subtle, so > here's some more detail: > > The FCP metric is only recorded if a page encounters a blocking script, and then > reaches FCP. > > Pages in the experiment population are more likely to reach FCP as a result of > encountering a blocking script than those in the control, since in experiment > the page loads faster as a result of blocking the script. > > Thus we encounter cases in experiment where doc.write FCP is recorded, but > wouldn't have been in control, as the user would have given up on the page load > before FCP was reached. > > This means that we can't use the presence of the FCP metric as a way to drill > down on the population of 'affected' users between control and experiment in a > way that makes those populations comparable across control and experiment. > > This change adds a simple counter that is recorded as soon as we discover a > blocking script. The presence of this counter allows us to know that the user > was affected by blocking (in experiment) or would have been affected by blocking > (in control) in a way that is comparable across populations for analysis > purposes. > > Hope that helps to explain. Happy to discuss more. Acknowledged. https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h (right): https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: bool doc_write_block_observed_; On 2017/01/18 14:27:32, Bryan McQuade wrote: > On 2017/01/18 at 14:07:31, jkarlin wrote: > > Can you = false this here, and do the same for the bool below? > > Sure. I do prefer this to setting it in the constructor, but do you know if this > is in the style guide? Would be useful for me to know for future code reviews. It's certainly an allowed C++ 11 feature, but I can't find it in the guide. Guess it falls to preference. Doesn't hurt to ask though ;)
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h (right): https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: bool doc_write_block_observed_; On 2017/01/18 14:52:24, jkarlin wrote: > On 2017/01/18 14:27:32, Bryan McQuade wrote: > > On 2017/01/18 at 14:07:31, jkarlin wrote: > > > Can you = false this here, and do the same for the bool below? > > > > Sure. I do prefer this to setting it in the constructor, but do you know if > this > > is in the style guide? Would be useful for me to know for future code reviews. > > It's certainly an allowed C++ 11 feature, but I can't find it in the guide. > Guess it falls to preference. Doesn't hurt to ask though ;) There was a chromium-dev discussion about this that seemed to slightly prefer in-class definitions: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Style$...
On 2017/01/18 15:00:58, Charlie Harrison wrote: > https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... > File > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h > (right): > > https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: > bool doc_write_block_observed_; > On 2017/01/18 14:52:24, jkarlin wrote: > > On 2017/01/18 14:27:32, Bryan McQuade wrote: > > > On 2017/01/18 at 14:07:31, jkarlin wrote: > > > > Can you = false this here, and do the same for the bool below? > > > > > > Sure. I do prefer this to setting it in the constructor, but do you know if > > this > > > is in the style guide? Would be useful for me to know for future code > reviews. > > > > It's certainly an allowed C++ 11 feature, but I can't find it in the guide. > > Guess it falls to preference. Doesn't hurt to ask though ;) > > There was a chromium-dev discussion about this that seemed to slightly prefer > in-class definitions: > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Style$... Ah, thanks for that. And it used to be in the style guide: " Use in-class member initialization for simple initializations, especially when a member variable must be initialized the same way in more than one constructor." But it has since been removed and they don't mention member initialization at all now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/18 at 15:04:04, jkarlin wrote: > On 2017/01/18 15:00:58, Charlie Harrison wrote: > > https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... > > File > > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h > > (right): > > > > https://codereview.chromium.org/2644543002/diff/20001/chrome/browser/page_loa... > > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: > > bool doc_write_block_observed_; > > On 2017/01/18 14:52:24, jkarlin wrote: > > > On 2017/01/18 14:27:32, Bryan McQuade wrote: > > > > On 2017/01/18 at 14:07:31, jkarlin wrote: > > > > > Can you = false this here, and do the same for the bool below? > > > > > > > > Sure. I do prefer this to setting it in the constructor, but do you know if > > > this > > > > is in the style guide? Would be useful for me to know for future code > > reviews. > > > > > > It's certainly an allowed C++ 11 feature, but I can't find it in the guide. > > > Guess it falls to preference. Doesn't hurt to ask though ;) > > > > There was a chromium-dev discussion about this that seemed to slightly prefer > > in-class definitions: > > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Style$... > > Ah, thanks for that. And it used to be in the style guide: > > " Use in-class member initialization for simple initializations, > especially when a member variable must be initialized the same way > in more than one constructor." > > But it has since been removed and they don't mention member initialization at all now. Ah, ok, thanks! I'll leave it as we have it then. I prefer setting the default where the variable is declared, as opposed to in the initializer list, as you suggested.
bmcquade@chromium.org changed reviewers: - csharrison@chromium.org
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2644543002/#ps60001 (title: "switch to UMA_HISTOGRAM_BOOLEAN.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484778654171450,
"parent_rev": "a66fe8713400ed760cd5d78931e536f33c5828d5", "commit_rev":
"6cc8974e18028a1e18779c36318acfe5c069ec2c"}
Message was sent while issue was closed.
Description was changed from ========== Add counter for number of page loads where doc.write blocking triggers. When doing logs analysis for doc.write blocking, it's desirable to analyze only cases where doc.write blocking could have triggered, to minimize noise from samples where the feature couldn't have triggered. This is especially important given that the blocking only triggers on ~7% of page loads when on 2G. This change adds a counter that allows us to keep track of cases where the feature could have triggered, in a way that's comparable across control and experiment populations. BUG=682220 ========== to ========== Add counter for number of page loads where doc.write blocking triggers. When doing logs analysis for doc.write blocking, it's desirable to analyze only cases where doc.write blocking could have triggered, to minimize noise from samples where the feature couldn't have triggered. This is especially important given that the blocking only triggers on ~7% of page loads when on 2G. This change adds a counter that allows us to keep track of cases where the feature could have triggered, in a way that's comparable across control and experiment populations. BUG=682220 Review-Url: https://codereview.chromium.org/2644543002 Cr-Commit-Position: refs/heads/master@{#444519} Committed: https://chromium.googlesource.com/chromium/src/+/6cc8974e18028a1e18779c36318a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6cc8974e18028a1e18779c36318a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
