Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769933003:
Add more UMA to record whether parallel download is completed/interrupted/cancelled (Closed)
DescriptionAdd more UMA to record whether parallel download is completed/interrupted/cancelled
This CL adds more UMA to record parallel download status,
so that we can study how parallel requests will impact download.
BUG=644352
Review-Url: https://codereview.chromium.org/2769933003
Cr-Commit-Position: refs/heads/master@{#460436}
Committed: https://chromium.googlesource.com/chromium/src/+/75184f444eb8f9460f6d4ee36a90706eab762564
Patch Set 1 #
Total comments: 6
Patch Set 2 : report interrupted UMA for all downloads #
Total comments: 6
Patch Set 3 : use suffix for histogram #Patch Set 4 : rebase #
Messages
Total messages: 28 (11 generated)
qinmin@chromium.org changed reviewers: + dtrainor@chromium.org, isherman@chromium.org, xingliu@chromium.org
PTAL. isherman@: since the feature is not enabled in any channel, can we remove the last enum in DownloadCountType as this is introduced recently? I currently marked it as obsolete. Also, some of the stats are similar to those used by regular download. So I am wondering whether using a suffix would be ok. However, is it possible to add suffix to an existing histogram?
https://codereview.chromium.org/2769933003/diff/1/content/browser/download/do... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2769933003/diff/1/content/browser/download/do... content/browser/download/download_stats.h:96: USES_PARALLEL_REQUESTS, DOWNLOAD_COUNT_UNUSED_18? https://codereview.chromium.org/2769933003/diff/1/content/browser/download/do... content/browser/download/download_stats.h:104: PARALLEL_DOWNLOAD_COMPLETED_COUNT, How many of these will double count with the non-parallel case? Will we be able to easily distinguish stats for normal vs. parallel downloads? https://codereview.chromium.org/2769933003/diff/1/content/browser/download/do... content/browser/download/download_stats.h:167: void RecordDownloadCompleted(const base::TimeTicks& start, Should we call this?
https://codereview.chromium.org/2769933003/diff/1/content/browser/download/do... File content/browser/download/download_stats.h (right): https://codereview.chromium.org/2769933003/diff/1/content/browser/download/do... content/browser/download/download_stats.h:96: USES_PARALLEL_REQUESTS, On 2017/03/23 21:43:07, David Trainor-ping if over 24h wrote: > DOWNLOAD_COUNT_UNUSED_18? i am wondering whether i can remove this, this is not used since the finch trial hasn't started yet. https://codereview.chromium.org/2769933003/diff/1/content/browser/download/do... content/browser/download/download_stats.h:104: PARALLEL_DOWNLOAD_COMPLETED_COUNT, On 2017/03/23 21:43:07, David Trainor-ping if over 24h wrote: > How many of these will double count with the non-parallel case? Will we be able > to easily distinguish stats for normal vs. parallel downloads? Not sure exact percentage, but we should expect similar stats for Completed, cancelled. The started count will tell us the double count case. I would expect interrupted_count to go up a little bit, but not too much. https://codereview.chromium.org/2769933003/diff/1/content/browser/download/do... content/browser/download/download_stats.h:167: void RecordDownloadCompleted(const base::TimeTicks& start, On 2017/03/23 21:43:07, David Trainor-ping if over 24h wrote: > Should we call this? No, we don't need the time reporting. We already have the bandwidth and time saved stats in another histogram
For interrupted downloads, new CL should report stats for both parallel download and all downloads
On 2017/03/23 21:31:27, qinmin wrote: > PTAL. > > isherman@: > since the feature is not enabled in any channel, can we remove the last enum in > DownloadCountType as this is introduced recently? I currently marked it as > obsolete. If the value was never recorded, then it's fine to remove. OTOH, if it's been recorded for some users, it's safer to keep around. Basically, you don't want to end up in a situation where data uploaded by different users has different meanings, and old versions pretty much never fully drop to zero usage. With that in mind, I trust you to make an informed decision. > Also, some of the stats are similar to those used by regular download. So I am > wondering whether using a suffix would be ok. However, is it possible to add > suffix to an existing histogram? If I'm understanding your question correctly, then yes, it's perfectly fine to add a suffix to an existing histogram. Suffixes aren't really special in any particular way, other than being a bit easier to work with in some contexts. https://codereview.chromium.org/2769933003/diff/20001/content/browser/downloa... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2769933003/diff/20001/content/browser/downloa... content/browser/download/download_stats.cc:423: "Download.ParallelDownloadInterruptedReceivedSizeK", received_kb, 1, Optional: You might want to move "ParallelDownload" to be a suffix, so that related histograms are grouped near each other. (Well, related in terms of the thing being measured; currently, all ParallelDownload histograms would be alphabetized together, which is maybe more important for you?) https://codereview.chromium.org/2769933003/diff/20001/content/browser/downloa... content/browser/download/download_stats.cc:423: "Download.ParallelDownloadInterruptedReceivedSizeK", received_kb, 1, nit: I'd use an extra dot to separate "ParallelDownload" from the rest of the name. https://codereview.chromium.org/2769933003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2769933003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13693: +</histogram> Could you use a histogram_suffixes element to reduce repeated text in this file?
https://codereview.chromium.org/2769933003/diff/20001/content/browser/downloa... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2769933003/diff/20001/content/browser/downloa... content/browser/download/download_stats.cc:423: "Download.ParallelDownloadInterruptedReceivedSizeK", received_kb, 1, On 2017/03/24 00:52:59, Ilya Sherman wrote: > Optional: You might want to move "ParallelDownload" to be a suffix, so that > related histograms are grouped near each other. (Well, related in terms of the > thing being measured; currently, all ParallelDownload histograms would be > alphabetized together, which is maybe more important for you?) Done. https://codereview.chromium.org/2769933003/diff/20001/content/browser/downloa... content/browser/download/download_stats.cc:423: "Download.ParallelDownloadInterruptedReceivedSizeK", received_kb, 1, On 2017/03/24 00:52:59, Ilya Sherman wrote: > nit: I'd use an extra dot to separate "ParallelDownload" from the rest of the > name. uses .ParallelDownload suffix now https://codereview.chromium.org/2769933003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2769933003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13693: +</histogram> On 2017/03/24 00:52:59, Ilya Sherman wrote: > Could you use a histogram_suffixes element to reduce repeated text in this file? Done.
Metrics LGTM, thanks.
lgtm
The CQ bit was checked by qinmin@chromium.org
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
lgtm
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xingliu@chromium.org, isherman@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2769933003/#ps60001 (title: "rebase")
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
CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by qinmin@chromium.org
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by qinmin@chromium.org
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": 60001, "attempt_start_ts": 1490807845948780,
"parent_rev": "74c9463920f15cebcf2e90ba5a6b613fc03bcdd8", "commit_rev":
"75184f444eb8f9460f6d4ee36a90706eab762564"}
Message was sent while issue was closed.
Description was changed from ========== Add more UMA to record whether parallel download is completed/interrupted/cancelled This CL adds more UMA to record parallel download status, so that we can study how parallel requests will impact download. BUG=644352 ========== to ========== Add more UMA to record whether parallel download is completed/interrupted/cancelled This CL adds more UMA to record parallel download status, so that we can study how parallel requests will impact download. BUG=644352 Review-Url: https://codereview.chromium.org/2769933003 Cr-Commit-Position: refs/heads/master@{#460436} Committed: https://chromium.googlesource.com/chromium/src/+/75184f444eb8f9460f6d4ee36a90... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/75184f444eb8f9460f6d4ee36a90... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
