Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551013003:
Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. (Closed)
|
Created:
4 years ago by horo Modified:
4 years ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, asvitkine+watch_chromium.org, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs.
The result of the experiment of Speculative launch Service Workers was not so good as we thought.
We think this is because:
- Low percentage of navigations by link click
- High percentage of navigations with redirects
To confirm this, I'd like to introduce 2 UMAs.
- ServiceWorker.MainFramePageLoad.CoreTransition
- ServiceWorker.MainFramePageLoad.RedirectChainLength
BUG=616502
Committed: https://crrev.com/b7ebb2a06b4074b7fb9ecd3ef03d307321c5e910
Cr-Commit-Position: refs/heads/master@{#437250}
Patch Set 1 #
Total comments: 11
Patch Set 2 : incorporated falken's comment #Patch Set 3 : hardcode the max number of rediects. #
Total comments: 4
Patch Set 4 : s/HHTP/HTTP/ #Patch Set 5 : Add comment about where the 20 is defined #
Messages
Total messages: 58 (40 generated)
The CQ bit was checked by horo@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...
The CQ bit was checked by horo@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 ========== Introduce ServiceWorker.MainFramePageLoad.RedirectChainLength UMA BUG= ========== to ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigation with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 BUG= ==========
Description was changed from ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigation with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 BUG= ========== to ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigation with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 ==========
Description was changed from ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigation with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 ========== to ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigations with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by horo@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...
The CQ bit was checked by horo@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Please review this.
https://codereview.chromium.org/2551013003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_metrics.cc (right): https://codereview.chromium.org/2551013003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_metrics.cc:345: static_cast<int>(ui::PAGE_TRANSITION_LAST_CORE)); should be LAST_CORE + 1? https://codereview.chromium.org/2551013003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_metrics.cc:347: // length is 21. Could we make kMaxRedirects public and refer to it here instead of a literal 21? https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101181: +<enum name="ServiceWorkerCoreTransition" type="int"> Since these come from ui::PageTransition, could these possibly just be called PageTransition. Since there's already a deprecated PrerenderLocalVisitCoreTransition could we just reuse/rename that? https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101182: + <int value="0" label="LINK"/> Add a <summary> field with a pointer to the corresponding enum in .h file? https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101192: + <int value="10" label="GENERATED"/> 5 and 10 are both GENERATED
The CQ bit was checked by horo@chromium.org to run a CQ dry run
https://codereview.chromium.org/2551013003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_metrics.cc (right): https://codereview.chromium.org/2551013003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_metrics.cc:345: static_cast<int>(ui::PAGE_TRANSITION_LAST_CORE)); On 2016/12/06 01:25:14, falken wrote: > should be LAST_CORE + 1? Done. https://codereview.chromium.org/2551013003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_metrics.cc:347: // length is 21. On 2016/12/06 01:25:14, falken wrote: > Could we make kMaxRedirects public and refer to it here instead of a literal 21? Done. https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101181: +<enum name="ServiceWorkerCoreTransition" type="int"> On 2016/12/06 01:25:14, falken wrote: > Since these come from ui::PageTransition, could these possibly just be called > PageTransition. Since there's already a deprecated > PrerenderLocalVisitCoreTransition could we just reuse/rename that? Done. https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101182: + <int value="0" label="LINK"/> On 2016/12/06 01:25:14, falken wrote: > Add a <summary> field with a pointer to the corresponding enum in .h file? Done. https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101182: + <int value="0" label="LINK"/> On 2016/12/06 01:25:14, falken wrote: > Add a <summary> field with a pointer to the corresponding enum in .h file? Done. https://codereview.chromium.org/2551013003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:101192: + <int value="10" label="GENERATED"/> On 2016/12/06 01:25:14, falken wrote: > 5 and 10 are both GENERATED Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, lgtm
horo@chromium.org changed reviewers: + xunjieli@chromium.org
xunjieli@ Could you please review net/url_request/url_request.*?
horo@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@ Could you please review tools/metrics/histograms/histograms.xml?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke@ Matt mentioned that using kMaxRedirects in UMA is probably not a good idea. If we chage kMaxRedirects in the future, it will mess up the buckets.
On 2016/12/06 21:50:27, xunjieli wrote: > +mmenke@ > Matt mentioned that using kMaxRedirects in UMA is probably not a good idea. If > we chage kMaxRedirects in the future, it will mess up the buckets. True, if this is something that is likely to change I would not use it as max.
On 2016/12/06 22:42:31, rkaplow wrote: > On 2016/12/06 21:50:27, xunjieli wrote: > > +mmenke@ > > Matt mentioned that using kMaxRedirects in UMA is probably not a good idea. If > > we chage kMaxRedirects in the future, it will mess up the buckets. > > True, if this is something that is likely to change I would not use it as max. It's unclear if this is likely to change. It's been 20 for ages, but I doubt anyone has experimented with it. I suggest just hardcoding 20. If we end up with more, then they'll end up in the 20 bucket, no big loss.
The CQ bit was checked by horo@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...
On 2016/12/06 22:59:12, mmenke wrote: > On 2016/12/06 22:42:31, rkaplow wrote: > > On 2016/12/06 21:50:27, xunjieli wrote: > > > +mmenke@ > > > Matt mentioned that using kMaxRedirects in UMA is probably not a good idea. > If > > > we chage kMaxRedirects in the future, it will mess up the buckets. > > > > True, if this is something that is likely to change I would not use it as max. > > It's unclear if this is likely to change. It's been 20 for ages, but I doubt > anyone has experimented with it. I suggest just hardcoding 20. If we end up > with more, then they'll end up in the 20 bucket, no big loss. done
shimazu@chromium.org changed reviewers: + shimazu@chromium.org
drive-by: added a nit https://codereview.chromium.org/2551013003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_metrics.cc (right): https://codereview.chromium.org/2551013003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_metrics.cc:346: // Currently the max number of HHTP redirects is 20. So the max number of nit: HTTP redirects? and how about adding "as |kMaxRedirects| in url_request.cc." to the comment for reference?
The CQ bit was checked by horo@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...
https://codereview.chromium.org/2551013003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_metrics.cc (right): https://codereview.chromium.org/2551013003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_metrics.cc:346: // Currently the max number of HHTP redirects is 20. So the max number of On 2016/12/07 01:18:51, shimazu wrote: > nit: HTTP redirects? and how about adding "as |kMaxRedirects| in > url_request.cc." to the comment for reference? Fixed "HHTP". I incorporated mmenke@'s comment. https://codereview.chromium.org/2551013003/#msg36
https://codereview.chromium.org/2551013003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_metrics.cc (right): https://codereview.chromium.org/2551013003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_metrics.cc:346: // Currently the max number of HHTP redirects is 20. So the max number of On 2016/12/07 01:26:49, horo wrote: > On 2016/12/07 01:18:51, shimazu wrote: > > nit: HTTP redirects? and how about adding "as |kMaxRedirects| in > > url_request.cc." to the comment for reference? > > Fixed "HHTP". > I incorporated mmenke@'s comment. > https://codereview.chromium.org/2551013003/#msg36 It makes sense to hardcode it as per mmenke, but +1 to adding the comment about where the 20 is defined.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
https://codereview.chromium.org/2551013003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_metrics.cc (right): https://codereview.chromium.org/2551013003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_metrics.cc:346: // Currently the max number of HHTP redirects is 20. So the max number of On 2016/12/07 01:28:49, falken wrote: > On 2016/12/07 01:26:49, horo wrote: > > On 2016/12/07 01:18:51, shimazu wrote: > > > nit: HTTP redirects? and how about adding "as |kMaxRedirects| in > > > url_request.cc." to the comment for reference? > > > > Fixed "HHTP". > > I incorporated mmenke@'s comment. > > https://codereview.chromium.org/2551013003/#msg36 > > It makes sense to hardcode it as per mmenke, but +1 to adding the comment about > where the 20 is defined. Done.
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2551013003/#ps140001 (title: "Add comment about where the 20 is defined")
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": 140001, "attempt_start_ts": 1481203742872320,
"parent_rev": "6f2bea4f0fb474a8ba665ac2a1de555da47aed2e", "commit_rev":
"3f9e2b70e480f25836db8b3d362db411c3ec3f4d"}
Message was sent while issue was closed.
Description was changed from ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigations with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 ========== to ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigations with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigations with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 ========== to ========== Introduce ServiceWorker.MainFramePageLoad.CoreTransition and RedirectChainLength UMAs. The result of the experiment of Speculative launch Service Workers was not so good as we thought. We think this is because: - Low percentage of navigations by link click - High percentage of navigations with redirects To confirm this, I'd like to introduce 2 UMAs. - ServiceWorker.MainFramePageLoad.CoreTransition - ServiceWorker.MainFramePageLoad.RedirectChainLength BUG=616502 Committed: https://crrev.com/b7ebb2a06b4074b7fb9ecd3ef03d307321c5e910 Cr-Commit-Position: refs/heads/master@{#437250} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b7ebb2a06b4074b7fb9ecd3ef03d307321c5e910 Cr-Commit-Position: refs/heads/master@{#437250} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
