Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1011363005:
[Extensions] Add metrics for UserScriptListener (Closed)
|
Created:
5 years, 9 months ago by Devlin Modified:
5 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions] Add metrics for UserScriptListener
Add metrics to determine how often and for how long UserScriptListener is
throttling network traffic.
BUG=467003
Committed: https://crrev.com/2dc9e6c84f233ba1e69db99ae9514cd00ce1dda7
Cr-Commit-Position: refs/heads/master@{#321480}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Mark's #
Total comments: 6
Patch Set 3 : #
Messages
Total messages: 16 (4 generated)
rdevlin.cronin@chromium.org changed reviewers: + kalman@chromium.org
After a bit of hand testing, it looks like this doesn't really do anything (because user scripts are always ready). But let's get some real data.
lgtm. A metrics owner may be able to suggest a description that is less tailored towards somebody who knows precisely what this is all about. https://codereview.chromium.org/1011363005/diff/1/chrome/browser/extensions/u... File chrome/browser/extensions/user_script_listener.cc (right): https://codereview.chromium.org/1011363005/diff/1/chrome/browser/extensions/u... chrome/browser/extensions/user_script_listener.cc:41: timer_->Elapsed()); For completeness, timer_.reset()?
rdevlin.cronin@chromium.org changed reviewers: + mpearson@chromium.org
+Mark for histograms. https://codereview.chromium.org/1011363005/diff/1/chrome/browser/extensions/u... File chrome/browser/extensions/user_script_listener.cc (right): https://codereview.chromium.org/1011363005/diff/1/chrome/browser/extensions/u... chrome/browser/extensions/user_script_listener.cc:41: timer_->Elapsed()); On 2015/03/19 15:41:14, kalman wrote: > For completeness, timer_.reset()? These are deleted right after resume is called, but on the offchance the API somehow changes and multiple resumes are allowed, it almost seems like then this would still do the right thing. ;)
https://codereview.chromium.org/1011363005/diff/1/chrome/browser/extensions/u... File chrome/browser/extensions/user_script_listener.cc (right): https://codereview.chromium.org/1011363005/diff/1/chrome/browser/extensions/u... chrome/browser/extensions/user_script_listener.cc:8: #include "base/metrics/histogram.h" histogram_macros is more appropriate https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:9862: + extension user script load. Is this recorded on every network request? (What if there are no extension user scripts to load?) Or on every network request where there is an extension user script that needs to load? What happens if multiple extension user scripts need to load--do you record this once or each time? similar questions apply to the below histogram https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:9870: + The total number of network requests (per profile) that were throttled by What does "per profile" mean here? How could this not be "per profile"? Either you're implying that the same network request could be delayed on separate profiles (which makes no sense because profiles shouldn't share network requests) or the same extension user script load can delay network requests across multiple profiles (which also makes no sense because extensions are per-profile). https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:9871: + extension user script load. When is this emitted? Periodically? By the way, does throttled = delayed? You use "delayed" in the other histogram description.
https://codereview.chromium.org/1011363005/diff/1/chrome/browser/extensions/u... File chrome/browser/extensions/user_script_listener.cc (right): https://codereview.chromium.org/1011363005/diff/1/chrome/browser/extensions/u... chrome/browser/extensions/user_script_listener.cc:8: #include "base/metrics/histogram.h" On 2015/03/19 18:59:15, Mark P wrote: > histogram_macros is more appropriate Done. https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:9862: + extension user script load. On 2015/03/19 18:59:15, Mark P wrote: > Is this recorded on every network request? (What if there are no extension user > scripts to load?) Or on every network request where there is an extension user > script that needs to load? What happens if multiple extension user scripts need > to load--do you record this once or each time? > > similar questions apply to the below histogram Tried to clear it up a bit - sound better? https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:9870: + The total number of network requests (per profile) that were throttled by On 2015/03/19 18:59:15, Mark P wrote: > What does "per profile" mean here? How could this not be "per profile"? Either > you're implying that the same network request could be delayed on separate > profiles (which makes no sense because profiles shouldn't share network > requests) or the same extension user script load can delay network requests > across multiple profiles (which also makes no sense because extensions are > per-profile). Cleared up. But could potentially be not-per-profile if we emitted this once per browser session, and tracked the total number of network requests that were delayed across all profiles. https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:9871: + extension user script load. On 2015/03/19 18:59:15, Mark P wrote: > When is this emitted? Periodically? > > By the way, does throttled = delayed? You use "delayed" in the other histogram > description. Emitted once per profile when all user scripts have loaded (added to the description). s/throttle/delay.
https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:9862: + extension user script load. On 2015/03/19 19:45:54, Devlin wrote: > On 2015/03/19 18:59:15, Mark P wrote: > > Is this recorded on every network request? (What if there are no extension > user > > scripts to load?) Or on every network request where there is an extension > user > > script that needs to load? What happens if multiple extension user scripts > need > > to load--do you record this once or each time? > > > > similar questions apply to the below histogram > > Tried to clear it up a bit - sound better? Definitely better, thanks. https://codereview.chromium.org/1011363005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:9870: + The total number of network requests (per profile) that were throttled by On 2015/03/19 19:45:54, Devlin wrote: > On 2015/03/19 18:59:15, Mark P wrote: > > What does "per profile" mean here? How could this not be "per profile"? > Either > > you're implying that the same network request could be delayed on separate > > profiles (which makes no sense because profiles shouldn't share network > > requests) or the same extension user script load can delay network requests > > across multiple profiles (which also makes no sense because extensions are > > per-profile). > > Cleared up. But could potentially be not-per-profile if we emitted this once > per browser session, and tracked the total number of network requests that were > delayed across all profiles. Nice job clearing it up. https://codereview.chromium.org/1011363005/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1011363005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9884: + The duration for which a network request was delayed while waiting for nit: for->by https://codereview.chromium.org/1011363005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9885: + extension user script load. Recorded once per resource that is delayed, at resource == request? https://codereview.chromium.org/1011363005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9896: + user scripts have loaded. If a user adds a extension with user script (that thus needs to load), does that cause a new emit? Or is this effectively an on-profile-open emit (load all extensions' user scripts on profile open)?
https://codereview.chromium.org/1011363005/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1011363005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9884: + The duration for which a network request was delayed while waiting for On 2015/03/19 20:19:01, Mark P wrote: > nit: for->by Done. https://codereview.chromium.org/1011363005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9885: + extension user script load. Recorded once per resource that is delayed, at On 2015/03/19 20:19:01, Mark P wrote: > resource == request? Done. https://codereview.chromium.org/1011363005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9896: + user scripts have loaded. On 2015/03/19 20:19:01, Mark P wrote: > If a user adds a extension with user script (that thus needs to load), does that > cause a new emit? Or is this effectively an on-profile-open emit (load all > extensions' user scripts on profile open)? Initially, I thought it was on-profile-load, but it turns out it could restrict any time a user script is loading. Corrected.
histograms.xml lgtm but the diff shows something odd (a change in an irrelevant histogram description); please make sure you sync and rebase and that this diff no longer appears.
On 2015/03/19 22:08:30, Mark P wrote: > histograms.xml lgtm > but the diff shows something odd (a change in an irrelevant histogram > description); please make sure you sync and rebase and that this diff no longer > appears. Actually that diff is just because someone else forgot to pretty_print histograms.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1011363005/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011363005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2dc9e6c84f233ba1e69db99ae9514cd00ce1dda7 Cr-Commit-Position: refs/heads/master@{#321480} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
