Add UMA for estimating disk bandwidth and the time saved with parallel download

content/browser/download/download_stats.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_stats.h"
 
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/string_util.h"
@@ skipping 328 matching lines @@
 
 // Maps extensions to their matching UMA histogram int value.
 int GetDangerousFileType(const base::FilePath& file_path) {
   for (size_t i = 0; i < arraysize(kDangerousFileTypes); ++i) {
     if (file_path.MatchesExtension(kDangerousFileTypes[i]))
       return i + 1;
   }
   return 0;  // Unknown extension.
 }
 
+// Helper method to calculate and record the bandwidth for a given metric by
+// dividing the |data_length| by |elapsed_time|.
+int LogBandwidthBytesPerSecond(
+    const std::string& metric,
+    size_t data_length,
+    base::TimeDelta elapsed_time) {

[Ilya Sherman, 2017/03/15 20:54:43]

Optional nit: I find this function a bit hard to think about, because it does
two things -- compute a value, and record it -- but the name only describes one
of those.  I'd suggest splitting it into two functions: one to compute the
value, and one leetle wrapper function for recording it.

[qinmin, 2017/03/15 23:20:11]

Done.

+  size_t elapsed_time_ms = elapsed_time.InMilliseconds();
+  if (0u == elapsed_time_ms)
+    elapsed_time_ms = 1;
+
+  int bandwidth = 1000 * data_length / elapsed_time_ms;
+  UMA_HISTOGRAM_CUSTOM_COUNTS(

[Ilya Sherman, 2017/03/15 20:54:43]

The UMA_HISTOGRAM macros (other than the SPARSE macro) require metric names to
be runtime constants.  Please use base::UmaHistogramCustomCounts() instead.

[qinmin, 2017/03/15 23:20:11]

Done.

+      metric, bandwidth,  1, 50000000, 50);

[Ilya Sherman, 2017/03/15 20:54:43]

nit: Could you please write this as "50 * 1000 * 1000" to help make the constant
easier to read?  Also, are you sure that you want to use 1000 rather than 1024
as a factor here?

[qinmin, 2017/03/15 23:20:11]

Done. still keeping 1000 as the existing UMAs are using 1000 instead of 1024

+  return bandwidth;
+}
+
 } // namespace
 
 void RecordDownloadCount(DownloadCountTypes type) {
   UMA_HISTOGRAM_ENUMERATION(
       "Download.Counts", type, DOWNLOAD_COUNT_TYPES_LAST_ENTRY);
 }
 
 void RecordDownloadSource(DownloadSource source) {
   UMA_HISTOGRAM_ENUMERATION(
       "Download.Sources", source, DOWNLOAD_SOURCE_LAST_ENTRY);
@@ skipping 337 matching lines @@
         resource_handler_blocked_time * 100 / resource_handler_lifetime;
   }
 
   UMA_HISTOGRAM_COUNTS_100("Download.ResourceHandlerBlockedPercentage",
                            percentage);
 }
 
 void RecordFileBandwidth(size_t length,
                          base::TimeDelta disk_write_time,
                          base::TimeDelta elapsed_time) {
-  size_t elapsed_time_ms = elapsed_time.InMilliseconds();
-  if (0u == elapsed_time_ms)
-    elapsed_time_ms = 1;
-  size_t disk_write_time_ms = disk_write_time.InMilliseconds();
-  if (0u == disk_write_time_ms)
-    disk_write_time_ms = 1;
+  LogBandwidthBytesPerSecond(
+      "Download.BandwidthOverallBytesPerSecond", length, elapsed_time);
+  LogBandwidthBytesPerSecond(
+      "Download.BandwidthDiskBytesPerSecond", length, disk_write_time);
+}
 
+void RecordParallelDownloadStats(
+    size_t bytes_downloaded_with_parallel_streams,
+    base::TimeDelta time_with_parallel_streams,
+    size_t bytes_downloaded_without_parallel_streams,
+    base::TimeDelta time_without_parallel_streams) {
+  int bandwidth_without_parallel_streams = LogBandwidthBytesPerSecond(
+      "Download.BandwidthWithoutParallelStreamsBytesPerSecond",
+      bytes_downloaded_without_parallel_streams, time_without_parallel_streams);
+  LogBandwidthBytesPerSecond(
+      "Download.BandwidthWithParallelStreamsBytesPerSecond",
+      bytes_downloaded_with_parallel_streams, time_with_parallel_streams);
+
+  base::TimeDelta time_saved =
+      base::TimeDelta::FromMilliseconds(1000.0 *
+                                        bytes_downloaded_with_parallel_streams /
+                                        bandwidth_without_parallel_streams) -
+      time_with_parallel_streams;
+
+  int microseconds_per_hour =

[Ilya Sherman, 2017/03/15 20:54:43]

nit: I'd name this kMicrosecondsPerHour, and use base::checked_cast for the
cast.

[qinmin, 2017/03/15 23:20:11]

Done.

+      static_cast<int>(base::Time::kMicrosecondsPerHour);
   UMA_HISTOGRAM_CUSTOM_COUNTS(
-      "Download.BandwidthOverallBytesPerSecond",
-      (1000 * length / elapsed_time_ms), 1, 50000000, 50);
-  UMA_HISTOGRAM_CUSTOM_COUNTS(
-      "Download.BandwidthDiskBytesPerSecond",
-      (1000 * length / disk_write_time_ms), 1, 50000000, 50);
+      "Download.TimeSavedWithParallelDownload",
+      time_saved.InMilliseconds(), -microseconds_per_hour,

[Ilya Sherman, 2017/03/15 20:54:43]

UMA histograms do not support negative buckets (again, sparse histograms are the
exception).  You could shift the entire range up by kMicrosecondsPerHour, or
record two separate histograms for gains vs. losses, or various other
workarounds.  Feel free to ping me if you're not sure how to proceed here.

[qinmin, 2017/03/15 23:20:11]

Use 2 separate histogram instead.

+      microseconds_per_hour, 50);
 }
 
 void RecordDownloadFileRenameResultAfterRetry(
     base::TimeDelta time_since_first_failure,
     DownloadInterruptReason interrupt_reason) {
   if (interrupt_reason == DOWNLOAD_INTERRUPT_REASON_NONE) {
     UMA_HISTOGRAM_TIMES("Download.TimeToRenameSuccessAfterInitialFailure",
                         time_since_first_failure);
   } else {
     UMA_HISTOGRAM_TIMES("Download.TimeToRenameFailureAfterInitialFailure",
@@ skipping 66 matching lines @@
   if (!page_transition)
     return;
 
   UMA_HISTOGRAM_ENUMERATION(
       "Download.PageTransition",
       ui::PageTransitionStripQualifier(page_transition.value()),
       ui::PAGE_TRANSITION_LAST_CORE + 1);
 }
 
 }  // namespace content