Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820193002:
Record more download resumption UMAs to analyze the how download is resumed (Closed)
DescriptionRecord more download resumption UMAs to analyze the how download is resumed
This CL adds the metrics to study how many resumption is attempted for a download.
This allows us to study the high download interruption counts.
And choose a better throttling strategy.
BUG=712318
Review-Url: https://codereview.chromium.org/2820193002
Cr-Commit-Position: refs/heads/master@{#465451}
Committed: https://chromium.googlesource.com/chromium/src/+/bbe9ad733bbdf501cc63db9b624cfed076d33c32
Patch Set 1 #
Total comments: 8
Patch Set 2 : use sparse histogram #
Total comments: 6
Patch Set 3 : addressing comments #
Total comments: 2
Patch Set 4 : addressing comments #
Total comments: 4
Patch Set 5 : nit #
Messages
Total messages: 17 (6 generated)
qinmin@chromium.org changed reviewers: + dtrainor@chromium.org, isherman@chromium.org
PTAL
Out of curiosity, why do you store the intermediate values in shared prefs? Is it possible for the Chrome process to die while the download continues? https://codereview.chromium.org/2820193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1833: "MobileDownload.ResumptionsCount.Automatic", count, 1, 10, 10); nit: Mebbe use sparse histograms, with some manual clamping as a sanity-check, rather than preallocating buckets? This should give you a great tradeoff between client-side memory usage and server-side data granularity. https://codereview.chromium.org/2820193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1840: "MobileDownload.ResumptionsCount.Total", count, 1, 200, 20); (For these as well) https://codereview.chromium.org/2820193002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2820193002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:30132: +<histogram name="MobileDownload.ResumptionsCount"> nit: Please add base="true", as this histogram name is not emitted to directly; it's just used for building up suffixes. https://codereview.chromium.org/2820193002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:30136: + type. Please document when this is recorded: On each resumption? When the download completes? (successfully? in failure?)
Yes, the SharedPreference was used because download can be resumed after browser crashes. https://codereview.chromium.org/2820193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1833: "MobileDownload.ResumptionsCount.Automatic", count, 1, 10, 10); On 2017/04/17 22:18:43, Ilya Sherman wrote: > nit: Mebbe use sparse histograms, with some manual clamping as a sanity-check, > rather than preallocating buckets? This should give you a great tradeoff > between client-side memory usage and server-side data granularity. Done. https://codereview.chromium.org/2820193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1840: "MobileDownload.ResumptionsCount.Total", count, 1, 200, 20); On 2017/04/17 22:18:43, Ilya Sherman wrote: > (For these as well) Done. https://codereview.chromium.org/2820193002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2820193002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:30132: +<histogram name="MobileDownload.ResumptionsCount"> On 2017/04/17 22:18:43, Ilya Sherman wrote: > nit: Please add base="true", as this histogram name is not emitted to directly; > it's just used for building up suffixes. Done. https://codereview.chromium.org/2820193002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:30136: + type. On 2017/04/17 22:18:43, Ilya Sherman wrote: > Please document when this is recorded: On each resumption? When the download > completes? (successfully? in failure?) Done.
https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1828: int count = sharedPrefs.getInt(name, 0); Please add some manual clamping as a sanity check, i.e. count = min(count, 200) (and maybe also a DCHECK or clamping for negative values)
https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1205: SharedPreferences sharedPrefs = getAutoRetryCountSharedPreference(mContext); Do we actually store this in two places now? Here and the notification service? https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1812: private String getDownloadRetryCountSharedPrefName( Should we just start moving this all to proto or something more sustainable? Should we just unify all of our persistent storage here? We'll need some auto-resumable download counts for downloads that don't have a visible Java UI as well.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1205: SharedPreferences sharedPrefs = getAutoRetryCountSharedPreference(mContext); On 2017/04/18 16:49:17, David Trainor-ping if over 24h wrote: > Do we actually store this in two places now? Here and the notification service? The SharedPrefs in notification service controls number of browser restarts, so we don't fall into a vicious loop that browser crashes over and over due to resumption. So it is different from the usage of this. Added a TODO here. https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1812: private String getDownloadRetryCountSharedPrefName( On 2017/04/18 16:49:17, David Trainor-ping if over 24h wrote: > Should we just start moving this all to proto or something more sustainable? > Should we just unify all of our persistent storage here? We'll need some > auto-resumable download counts for downloads that don't have a visible Java UI > as well. Added a TODO here https://codereview.chromium.org/2820193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1828: int count = sharedPrefs.getInt(name, 0); On 2017/04/18 00:08:16, Ilya Sherman wrote: > Please add some manual clamping as a sanity check, i.e. count = min(count, 200) > (and maybe also a DCHECK or clamping for negative values) Done. https://codereview.chromium.org/2820193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1841: "MobileDownload.ResumptionsCount.Total", Math.min(count, 500)); Not sure if 500 is too big for a sparse histogram. If so, I can change it back to Count histogram.
lgtm. I think we need to figure out a well organized way to grab all of these metrics eventually. It looks like we're logging metrics in a ton of places and it makes it hard to follow/guarantee what will be logged when. https://codereview.chromium.org/2820193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1810: * TODO(qinmin): introduce a proto for this and consolidate all the UMA metrics stored in These also might make more sense (retry counts) inside DownloadsHistory.
Metrics LGTM % a final nit, thanks. https://codereview.chromium.org/2820193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1841: "MobileDownload.ResumptionsCount.Total", Math.min(count, 500)); On 2017/04/18 18:06:49, qinmin wrote: > Not sure if 500 is too big for a sparse histogram. If so, I can change it back > to Count histogram. Thanks, 500 should be fine. https://codereview.chromium.org/2820193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1843: count = sharedPrefs.getInt(name, 0); nit: Could you please assert that this count is also nonnegative?
https://codereview.chromium.org/2820193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2820193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1810: * TODO(qinmin): introduce a proto for this and consolidate all the UMA metrics stored in On 2017/04/18 22:35:45, David Trainor-ping if over 24h wrote: > These also might make more sense (retry counts) inside DownloadsHistory. added in TODO. https://codereview.chromium.org/2820193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1843: count = sharedPrefs.getInt(name, 0); On 2017/04/18 22:49:11, Ilya Sherman wrote: > nit: Could you please assert that this count is also nonnegative? Done.
The CQ bit was checked by qinmin@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/2820193002/#ps90001 (title: "nit")
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": 90001, "attempt_start_ts": 1492559124719630,
"parent_rev": "c63400e394a50c8b42ef798eaee47245c5febf04", "commit_rev":
"bbe9ad733bbdf501cc63db9b624cfed076d33c32"}
Message was sent while issue was closed.
Description was changed from ========== Record more download resumption UMAs to analyze the how download is resumed This CL adds the metrics to study how many resumption is attempted for a download. This allows us to study the high download interruption counts. And choose a better throttling strategy. BUG=712318 ========== to ========== Record more download resumption UMAs to analyze the how download is resumed This CL adds the metrics to study how many resumption is attempted for a download. This allows us to study the high download interruption counts. And choose a better throttling strategy. BUG=712318 Review-Url: https://codereview.chromium.org/2820193002 Cr-Commit-Position: refs/heads/master@{#465451} Committed: https://chromium.googlesource.com/chromium/src/+/bbe9ad733bbdf501cc63db9b624c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:90001) as https://chromium.googlesource.com/chromium/src/+/bbe9ad733bbdf501cc63db9b624c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
