Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
303123006:
Add SBClientDownload.DownloadRequestNetworkDuration and DownloadRequestTimeoutDuration histograms. (Closed)
|
Created:
6 years, 6 months ago by mattm Modified:
6 years, 6 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, jar+watch_chromium.org, moheeb1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd SBClientDownload.DownloadRequestNetworkDuration and DownloadRequestTimeoutDuration histograms.
BUG=378911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275326
Patch Set 1 #
Total comments: 7
Patch Set 2 : rebase #Patch Set 3 : review changes #
Total comments: 2
Patch Set 4 : review changes #
Total comments: 2
Patch Set 5 : review changes #
Messages
Total messages: 15 (0 generated)
lgtm
+mpearson for histograms/ OWNERS
https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23367: + check whether the content of a download is malicious or not, including file Thank you for adding clarifications to a pre-existing histogram. I rarely see anyone do this. https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23387: + includes cancelled and failed pings. What if you don't get a response. I would call that a "failed" ping. What time do you record? And what time do you record for cancelled pings? The time until cancellation? https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23412: + parts that are covered by the timeout. This histogram includes any checks Does the "only including..." add something here? If not, remove it. If so, please make be more precise (i.e., what's included and what's excluded). If you want to be hand-wavy, at least refer to the relevant code. https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23413: + that started the timeout, regardless if they sent a ping or not. I don't know what "includes any checks that started the timeout" means. I would think a timeout timer could only be started once. Are you missing a word?
https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23387: + includes cancelled and failed pings. On 2014/05/30 18:31:27, Mark P wrote: > What if you don't get a response. I would call that a "failed" ping. What time > do you record? Failed meant pings that completed but with a bad response. Not getting a response would be a cancelled ping. Clarified. > And what time do you record for cancelled pings? The time until > cancellation? Yeah, clarified. Do you think there's a better way to record that? https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23412: + parts that are covered by the timeout. This histogram includes any checks On 2014/05/30 18:31:27, Mark P wrote: > Does the "only including..." add something here? If not, remove it. If so, > please make be more precise (i.e., what's included and what's excluded). If you > want to be hand-wavy, at least refer to the relevant code. > Done. https://codereview.chromium.org/303123006/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:23413: + that started the timeout, regardless if they sent a ping or not. On 2014/05/30 18:31:27, Mark P wrote: > I don't know what "includes any checks that started the timeout" means. I would > think a timeout timer could only be started once. Are you missing a word? Some checks complete before getting to the point that starts the timeout.
https://codereview.chromium.org/303123006/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303123006/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23461: + recorded as the time the request was cancelled. On 2014/06/04 03:18:13, mattm wrote: > On 2014/05/30 18:31:27, Mark P wrote: > > What if you don't get a response. I would call that a "failed" ping. What > time > > do you record? > > Failed meant pings that completed but with a bad response. Not getting a > response would be a cancelled ping. Clarified. > > > And what time do you record for cancelled pings? The time until > > cancellation? > > Yeah, clarified. Do you think there's a better way to record that? I would think you want two histograms here, one with two buckets counting the numbers of successfully completed requests versus cancelled requests, and another that shows the time distribution for successful requests. (I assume the time distribution for canceled requests is going pretty much to be a peak at the value of a timeout in your code. If not, then perhaps another histogram showing the distribution of times for canceled requests.) Right now you're adding two separate distribution together and reporting them as one, and I'm not sure how much insight this will get you.
https://codereview.chromium.org/303123006/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303123006/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23461: + recorded as the time the request was cancelled. On 2014/06/04 16:47:34, Mark P wrote: > On 2014/06/04 03:18:13, mattm wrote: > > On 2014/05/30 18:31:27, Mark P wrote: > > > What if you don't get a response. I would call that a "failed" ping. What > > time > > > do you record? > > > > Failed meant pings that completed but with a bad response. Not getting a > > response would be a cancelled ping. Clarified. > > > > > And what time do you record for cancelled pings? The time until > > > cancellation? > > > > Yeah, clarified. Do you think there's a better way to record that? > > I would think you want two histograms here, one with two buckets counting the > numbers of successfully completed requests versus cancelled requests, and > another that shows the time distribution for successful requests. Makes sense. Did that for both the Network and Timeout histograms, hopefully this isn't too much histogram spam. > (I assume the > time distribution for canceled requests is going pretty much to be a peak at the > value of a timeout in your code. If not, then perhaps another histogram showing > the distribution of times for canceled requests.) Yeah, aside from cancellation due to shutdown which should be pretty rare. > Right now you're adding two > separate distribution together and reporting them as one, and I'm not sure how > much insight this will get you.
https://codereview.chromium.org/303123006/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303123006/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23504: + CheckClientDownloadRequest::StartTimeout() call. You might want to clarify that this not recorded at the time of the StartTimeout call, but recorded later once we know if (for instance) a resulting ping received a reply.
https://codereview.chromium.org/303123006/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303123006/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:23504: + CheckClientDownloadRequest::StartTimeout() call. On 2014/06/05 00:28:55, Mark P wrote: > You might want to clarify that this not recorded at the time of the StartTimeout > call, but recorded later once we know if (for instance) a resulting ping > received a reply. Done.
histograms.xml lgtm You may want to have someone re-review the code. It's change since the last code review. --mark
lgtm
The CQ bit was checked by mattm@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mattm@chromium.org/303123006/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 275326 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
