Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806653002:
Add UMA to track why a download is non-parallel when enabled parallel (Closed)
DescriptionAdd UMA to track why a download is non-parallel when enabled parallel
downloading.
Move the function to determine parallel download back to
DownloadJobFactory, since now it contains UMA code and should be only
called in one place.
BUG=709295
Review-Url: https://codereview.chromium.org/2806653002
Cr-Commit-Position: refs/heads/master@{#463452}
Committed: https://chromium.googlesource.com/chromium/src/+/c97ad191cb505a2e58a726a0aa5457668ae2477d
Patch Set 1 #
Total comments: 12
Patch Set 2 : Work on feedbacks. #Patch Set 3 : Minor comment polish. #
Total comments: 14
Patch Set 4 : Polish on histogram naming. #
Total comments: 2
Patch Set 5 : Polish on the comment. #
Messages
Total messages: 31 (17 generated)
Description was changed from ========== Add UMA to track why a download is non-parallel when enabled parallel downloading. Move the function to determine parallel download back to DownloadJobFactory, since now it contains UMA code and should be only called in one place. Add the UMA tracking for parallel downloading fail reason. BUG=709295 ========== to ========== Add UMA to track why a download is non-parallel when enabled parallel downloading. Move the function to determine parallel download back to DownloadJobFactory, since now it contains UMA code and should be only called in one place. BUG=709295 ==========
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, qinmin@chromium.org
Hi, PTAL, thanks.
The CQ bit was checked by xingliu@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.
xingliu@chromium.org changed reviewers: + isherman@chromium.org
https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... content/browser/download/download_stats.h:252: void RecordParallelDownloadFailReason(ParallelDownloadFailReason reason); This sounds more like a download failed. what about RecordParallelRequestCreationFailureReason or sth like that https://codereview.chromium.org/2806653002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2806653002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13868: +<histogram name="Download.ParallelDownloadFailReason" Download.ParallelDownload.CreationFailureReason?
https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... content/browser/download/download_stats.h:150: enum ParallelDownloadFailReason { nit: Could this be an enum class? https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... content/browser/download/download_stats.h:156: FAIL_COUNT, What does "FAIL_COUNT" mean? "EXCEEDED_MAX_ALLOWED_FAILURES"? https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... content/browser/download/download_stats.h:157: MAX_COUNT, nit: I find "MAX_COUNT" to be a confusing name. Could this just be "COUNT"? (Assuming the name above doesn't need "count" in it.) https://codereview.chromium.org/2806653002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2806653002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13872: + Records why the download is not created as parallel download. I'd recommend including "success" as bucket zero in this histogram, so that you can see the overall failure rate as well. (If you make this change, you'll likely want to also rename the histogram and update the description.) https://codereview.chromium.org/2806653002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13872: + Records why the download is not created as parallel download. Please document when this metric is recorded. Is it on download start, for every download, unless the download was created as a parallel download? Is there any pre-filtering?
The CQ bit was checked by xingliu@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 for the feedback. PTAL. https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... content/browser/download/download_stats.h:150: enum ParallelDownloadFailReason { On 2017/04/07 22:13:08, Ilya Sherman wrote: > nit: Could this be an enum class? Done. https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... content/browser/download/download_stats.h:156: FAIL_COUNT, On 2017/04/07 22:13:08, Ilya Sherman wrote: > What does "FAIL_COUNT" mean? "EXCEEDED_MAX_ALLOWED_FAILURES"? Changed to FALLBACK_TO_NORMAL_DOWNLOAD_COUNT. https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... content/browser/download/download_stats.h:157: MAX_COUNT, On 2017/04/07 22:13:08, Ilya Sherman wrote: > nit: I find "MAX_COUNT" to be a confusing name. Could this just be "COUNT"? > (Assuming the name above doesn't need "count" in it.) Done. Reworked the naming and the comments. https://codereview.chromium.org/2806653002/diff/1/content/browser/download/do... content/browser/download/download_stats.h:252: void RecordParallelDownloadFailReason(ParallelDownloadFailReason reason); On 2017/04/07 21:00:52, qinmin wrote: > This sounds more like a download failed. what about > RecordParallelRequestCreationFailureReason or sth like that Done. https://codereview.chromium.org/2806653002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2806653002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13868: +<histogram name="Download.ParallelDownloadFailReason" On 2017/04/07 21:00:52, qinmin wrote: > Download.ParallelDownload.CreationFailureReason? Done.
Metrics LGTM % nits: https://codereview.chromium.org/2806653002/diff/40001/content/browser/downloa... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2806653002/diff/40001/content/browser/downloa... content/browser/download/download_stats.h:152: enum class ParallelDownloadCreationEvent { nit: Please document that this enum is used to back an UMA histogram and should therefore be treated as append-only. https://codereview.chromium.org/2806653002/diff/40001/content/browser/downloa... content/browser/download/download_stats.h:154: PARALLEL_DOWNLOAD_COUNT = 0, Optional nit: I still think it's weird to use "COUNT" in the name of some buckets but not others. I'd name this something like "STARTED_PARALLEL_DOWNLOAD" and "FELL_BACK_TO_NORMAL_DOWNLOAD" below. https://codereview.chromium.org/2806653002/diff/40001/content/browser/downloa... content/browser/download/download_stats.h:176: MAX, Optional nit: IMO "max" makes sense to use as an alias for the last item in an enum, and "count" makes sense to count the number of items in an enum. So, this is currently named "max" but is behaving more like "count". I'd rename it to "count". https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13864: + parallel download or fall back to normal download based on various factor. nit: s/factor/factors https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13865: + Records the total number of parallel and non-parallel download creation when nit: s/download creation/downloads created https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13867: + parallel download falls back to normal download. The reasons are not mutual nit: s/mutual/mutually https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:105732: + <int value="1" label="Fallback to normal download count"/> Optional nit: Again, I'd drop "count" from both of these buckets, since all of the buckets are counts.
The CQ bit was checked by xingliu@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.
Thanks for the feedback. https://codereview.chromium.org/2806653002/diff/40001/content/browser/downloa... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2806653002/diff/40001/content/browser/downloa... content/browser/download/download_stats.h:152: enum class ParallelDownloadCreationEvent { On 2017/04/08 01:22:46, Ilya Sherman wrote: > nit: Please document that this enum is used to back an UMA histogram and should > therefore be treated as append-only. Done. https://codereview.chromium.org/2806653002/diff/40001/content/browser/downloa... content/browser/download/download_stats.h:154: PARALLEL_DOWNLOAD_COUNT = 0, On 2017/04/08 01:22:46, Ilya Sherman wrote: > Optional nit: I still think it's weird to use "COUNT" in the name of some > buckets but not others. I'd name this something like > "STARTED_PARALLEL_DOWNLOAD" and "FELL_BACK_TO_NORMAL_DOWNLOAD" below. Done. https://codereview.chromium.org/2806653002/diff/40001/content/browser/downloa... content/browser/download/download_stats.h:176: MAX, On 2017/04/08 01:22:46, Ilya Sherman wrote: > Optional nit: IMO "max" makes sense to use as an alias for the last item in an > enum, and "count" makes sense to count the number of items in an enum. So, this > is currently named "max" but is behaving more like "count". I'd rename it to > "count". Done. https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13864: + parallel download or fall back to normal download based on various factor. On 2017/04/08 01:22:47, Ilya Sherman wrote: > nit: s/factor/factors Done. https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13865: + Records the total number of parallel and non-parallel download creation when On 2017/04/08 01:22:47, Ilya Sherman wrote: > nit: s/download creation/downloads created Done. https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13867: + parallel download falls back to normal download. The reasons are not mutual On 2017/04/08 01:22:47, Ilya Sherman wrote: > nit: s/mutual/mutually Done. https://codereview.chromium.org/2806653002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:105732: + <int value="1" label="Fallback to normal download count"/> On 2017/04/08 01:22:47, Ilya Sherman wrote: > Optional nit: Again, I'd drop "count" from both of these buckets, since all of > the buckets are counts. Done.
lgtm
https://codereview.chromium.org/2806653002/diff/60001/content/browser/downloa... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2806653002/diff/60001/content/browser/downloa... content/browser/download/download_stats.h:152: // Used in histogram "Download.ParallelDownload.CreationEvent". nit: Please also document that "and therefore, should be treated as append-only". Metrics data is persisted, so it's important that semantics remain consistent over time.
(Still LGTM % that nit.)
https://codereview.chromium.org/2806653002/diff/60001/content/browser/downloa... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2806653002/diff/60001/content/browser/downloa... content/browser/download/download_stats.h:152: // Used in histogram "Download.ParallelDownload.CreationEvent". On 2017/04/10 21:54:15, Ilya Sherman wrote: > nit: Please also document that "and therefore, should be treated as > append-only". Metrics data is persisted, so it's important that semantics > remain consistent over time. Done, sorry I probably forgot this one.
lgtm
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2806653002/#ps80001 (title: "Polish on the comment.")
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": 80001, "attempt_start_ts": 1491863984104750,
"parent_rev": "af3c577c74d641ba49887e27a54595cd04ef7108", "commit_rev":
"c97ad191cb505a2e58a726a0aa5457668ae2477d"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA to track why a download is non-parallel when enabled parallel downloading. Move the function to determine parallel download back to DownloadJobFactory, since now it contains UMA code and should be only called in one place. BUG=709295 ========== to ========== Add UMA to track why a download is non-parallel when enabled parallel downloading. Move the function to determine parallel download back to DownloadJobFactory, since now it contains UMA code and should be only called in one place. BUG=709295 Review-Url: https://codereview.chromium.org/2806653002 Cr-Commit-Position: refs/heads/master@{#463452} Committed: https://chromium.googlesource.com/chromium/src/+/c97ad191cb505a2e58a726a0aa54... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/c97ad191cb505a2e58a726a0aa54... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
