Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209963002:
Record some foreign fetch registration metrics. (Closed)
|
Created:
4 years, 4 months ago by Marijn Kruisselbrink Modified:
4 years, 4 months ago CC:
asvitkine+watch_chromium.org, blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+watch, michaeln, nhiroki, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord some foreign fetch registration metrics.
Specifically this records how many sub scopes a service worker registered
for foreign fetch with, and for the service workers that registered for
foreign fetch, this also records the number of origins they are intercepting
requests from.
BUG=540509
Committed: https://crrev.com/4c007590505cceecde0aa2db3b8fa4ebdc221f99
Cr-Commit-Position: refs/heads/master@{#412676}
Patch Set 1 #
Total comments: 11
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : better histogram summaries #
Messages
Total messages: 27 (17 generated)
The CQ bit was checked by mek@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.
Description was changed from ========== Record some more metrics for foreign fetch. BUG= ========== to ========== Record some foreign fetch registration metrics. Specifically this records how many sub scopes a service worker registered for foreign fetch with, and for the service workers that registered for foreign fetch, this also records the number of origins they are intercepting requests from. BUG=540509 ==========
mek@chromium.org changed reviewers: + michaeln@chromium.org, mpearson@chromium.org
michaeln for content/browser/service_worker OWNERS mpearson for histograms OWNERS
lgtm
mpearson: ping?
https://codereview.chromium.org/2209963002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_metrics.cc (right): https://codereview.chromium.org/2209963002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_metrics.cc:391: scope_count); Why is the histogram "subscopecount" yet you're using a "scope_count"? These things sound different to me. https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51985: + origin. Recorded t the end of a successful install event, if the service "t" -> "at"? https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51986: + worker register for foreign fetch. Can a service worker register for foreign fetch but not claim any origins? https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51993: + The number of subscopes a particular service worker registered for. Recorded Please briefly explain what a subscope is. https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51994: + at the end of every succesful install event. nit: every -> a (to be identical to the sentence you used in the other histogram)
The CQ bit was checked by mek@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by mek@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/2209963002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_metrics.cc (right): https://codereview.chromium.org/2209963002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_metrics.cc:391: scope_count); On 2016/08/11 at 22:42:18, Mark P wrote: > Why is the histogram "subscopecount" yet you're using a "scope_count"? These things sound different to me. Changed it to consistently be "scope count", at least on the metrics side. After all in the spec and rest of the implementation I also call these things "foreign fetch scopes" and not "sub scopes". https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51985: + origin. Recorded t the end of a successful install event, if the service On 2016/08/11 at 22:42:18, Mark P wrote: > "t" -> "at"? fixed https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51986: + worker register for foreign fetch. On 2016/08/11 at 22:42:18, Mark P wrote: > Can a service worker register for foreign fetch but not claim any origins? No, hence why "all origins" is internally represented as an empty list of origins. https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51993: + The number of subscopes a particular service worker registered for. Recorded On 2016/08/11 at 22:42:18, Mark P wrote: > Please briefly explain what a subscope is. changed it to "foreign fetch scopes". And I'm not sure it makes sense to really explain every concept that is referenced in a histogram in great detail. It doesn't seem feasible to try to make these descriptions understandable for people that don't know anything about the feature they're logging stuff about. https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51994: + at the end of every succesful install event. On 2016/08/11 at 22:42:18, Mark P wrote: > nit: every -> a > (to be identical to the sentence you used in the other histogram) I think this is clearer by not having this be identical to the other histogram. After all the other histograms is only recorded for some install events (namely those where foreign fetch registration took place), but this histogram is recorded for every install event.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
histograms.xml lgtm with minor comments below --mark https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2209963002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:51994: + at the end of every succesful install event. On 2016/08/12 20:33:23, Marijn Kruisselbrink wrote: > On 2016/08/11 at 22:42:18, Mark P wrote: > > nit: every -> a > > (to be identical to the sentence you used in the other histogram) > > I think this is clearer by not having this be identical to the other histogram. > After all the other histograms is only recorded for some install events (namely > those where foreign fetch registration took place), but this histogram is > recorded for every install event. Ah, thank you for pointing out the difference. https://codereview.chromium.org/2209963002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2209963002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52487: + registered for foreign fetch. On 2016/08/12 20:33:23, Marijn Kruisselbrink wrote: > On 2016/08/11 at 22:42:18, Mark P wrote: > > Can a service worker register for foreign fetch but not claim any origins? > > No, hence why "all origins" is internally represented as an empty list of > origins. Can you add something to this effect to the end of the description? A service worker that registers for foreign fetch must either register at least one origin to intercept or intercept all origins. https://codereview.chromium.org/2209963002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52495: + for. Recorded at the end of every successful install event. Will this always be 0 for workers with no foreign fetch registration? If so, that might be a good example to add.
https://codereview.chromium.org/2209963002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2209963002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52487: + registered for foreign fetch. On 2016/08/16 at 22:27:14, Mark P wrote: > On 2016/08/12 20:33:23, Marijn Kruisselbrink wrote: > > On 2016/08/11 at 22:42:18, Mark P wrote: > > > Can a service worker register for foreign fetch but not claim any origins? > > > > No, hence why "all origins" is internally represented as an empty list of > > origins. > > Can you add something to this effect to the end of the description? > A service worker that registers for foreign fetch must either register at least one origin to intercept or intercept all origins. Done https://codereview.chromium.org/2209963002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:52495: + for. Recorded at the end of every successful install event. On 2016/08/16 at 22:27:14, Mark P wrote: > Will this always be 0 for workers with no foreign fetch registration? If so, that might be a good example to add. Done
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2209963002/#ps80001 (title: "better histogram summaries")
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.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Record some foreign fetch registration metrics. Specifically this records how many sub scopes a service worker registered for foreign fetch with, and for the service workers that registered for foreign fetch, this also records the number of origins they are intercepting requests from. BUG=540509 ========== to ========== Record some foreign fetch registration metrics. Specifically this records how many sub scopes a service worker registered for foreign fetch with, and for the service workers that registered for foreign fetch, this also records the number of origins they are intercepting requests from. BUG=540509 Committed: https://crrev.com/4c007590505cceecde0aa2db3b8fa4ebdc221f99 Cr-Commit-Position: refs/heads/master@{#412676} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4c007590505cceecde0aa2db3b8fa4ebdc221f99 Cr-Commit-Position: refs/heads/master@{#412676} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
