Update ServiceWorker event handled status histogram

Currently ServiceWorker.UnhandledEventRatio is a bit noisy and not
very informative. This CL changes the UMA as following:

- Simplify per-worker event handled status. Probably we don't need to
  record the handled ratio in percentage, but just record status in
  some buckets (e.g. 'all events' or 'no events' are handled etc)
  would be enough.

- Reset the metrics when the SW's stopped, so that the metrics won't be
  skewed by the lifetime of versions.

- Exclude ServiceWorker stats for NTP like scope, as it tends to dominate
  the stats and make the resutls largely skewed.

BUG=483354
TEST=(manually tested locally with chrome://histograms)

Review URL: https://codereview.chromium.org/1178043002

Cr-Commit-Position: refs/heads/master@{#337576}

3 files changed, 72 insertions, 17 deletions

diff --git a/content/browser/service_worker/service_worker_metrics.cc b/content/browser/service_worker/service_worker_metrics.cc
index ec85f02..8709b11 100644
--- a/content/browser/service_worker/service_worker_metrics.cc
+++ b/content/browser/service_worker/service_worker_metrics.cc
@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics_action.h"
+#include "base/strings/string_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/user_metrics.h"
@@ -21,6 +22,25 @@ void RecordURLMetricOnUI(const GURL& url) {
       "ServiceWorker.ControlledPageUrl", url);
 }
 
+bool ShouldExcludeForHistogram(const GURL& scope) {
+  // Exclude NTP scope from UMA for now as it tends to dominate the stats
+  // and makes the results largely skewed.
+  // TOOD(kinuko): This should be temporary, revisit this once we have
+  // better idea about what should be excluded in the UMA.
+  // (UIThreadSearchTermsData::GoogleBaseURLValue() returns the google base
+  // URL, but not available in content layer)
+  const char google_like_scope_prefix[] = "https://www.google.";
+  return base::StartsWith(scope.spec(), google_like_scope_prefix,
+                          base::CompareCase::INSENSITIVE_ASCII);
+}
+
+enum EventHandledRatioType {
+  EVENT_HANDLED_NONE,
+  EVENT_HANDLED_SOME,
+  EVENT_HANDLED_ALL,
+  NUM_EVENT_HANDLED_RATIO_TYPE,
+};
+
 }  // namespace
 
 void ServiceWorkerMetrics::CountInitDiskCacheResult(bool result) {
@@ -122,13 +142,21 @@ void ServiceWorkerMetrics::RecordInstallEventStatus(
                             SERVICE_WORKER_ERROR_MAX_VALUE);
 }
 
-void ServiceWorkerMetrics::RecordEventStatus(size_t fired_events,
-                                             size_t handled_events) {
-  if (!fired_events)
+void ServiceWorkerMetrics::RecordEventHandledRatio(const GURL& scope,
+                                                   EventType event,
+                                                   size_t handled_events,
+                                                   size_t fired_events) {
+  if (!fired_events || ShouldExcludeForHistogram(scope))
     return;
-  int unhandled_ratio = (fired_events - handled_events) * 100 / fired_events;
-  UMA_HISTOGRAM_PERCENTAGE("ServiceWorker.UnhandledEventRatio",
-                           unhandled_ratio);
+  EventHandledRatioType type = EVENT_HANDLED_SOME;
+  if (fired_events == handled_events)
+    type = EVENT_HANDLED_ALL;
+  else if (handled_events == 0)
+    type = EVENT_HANDLED_NONE;
+  // For now Fetch is the only type that is recorded.
+  DCHECK_EQ(EVENT_TYPE_FETCH, event);
+  UMA_HISTOGRAM_ENUMERATION("ServiceWorker.EventHandledRatioType.Fetch", type,
+                            NUM_EVENT_HANDLED_RATIO_TYPE);
 }
 
 void ServiceWorkerMetrics::RecordFetchEventStatus(
diff --git a/content/browser/service_worker/service_worker_metrics.h b/content/browser/service_worker/service_worker_metrics.h
index 52f471c..9a6eb56 100644
--- a/content/browser/service_worker/service_worker_metrics.h
+++ b/content/browser/service_worker/service_worker_metrics.h
@@ -73,6 +73,13 @@ class ServiceWorkerMetrics {
     NUM_STOP_STATUS_TYPES
   };
 
+  enum EventType {
+    EVENT_TYPE_FETCH,
+    // Add new events to record here.
+
+    NUM_EVENT_TYPES
+  };
+
   // Used for ServiceWorkerDiskCache.
   static void CountInitDiskCacheResult(bool result);
   static void CountReadResponseResult(ReadResponseResult result);
@@ -110,9 +117,12 @@ class ServiceWorkerMetrics {
   static void RecordActivateEventStatus(ServiceWorkerStatusCode status);
   static void RecordInstallEventStatus(ServiceWorkerStatusCode status);
 
-  // Records the ratio of unhandled events to the all events fired during
-  // the lifetime of ServiceWorker.
-  static void RecordEventStatus(size_t fired_events, size_t handled_events);
+  // Records how much of dispatched events are handled while a Service
+  // Worker is awake (i.e. after it is woken up until it gets stopped).
+  static void RecordEventHandledRatio(const GURL& scope,
+                                      EventType event,
+                                      size_t handled_events,
+                                      size_t fired_events);
 
   // Records the result of dispatching a fetch event to a service worker.
   static void RecordFetchEventStatus(bool is_main_resource,
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 802ed2e..fad1b56 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -4,6 +4,9 @@
 
 #include "content/browser/service_worker/service_worker_version.h"
 
+#include <map>
+#include <string>
+
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
@@ -395,15 +398,20 @@ const int ServiceWorkerVersion::kRequestTimeoutMinutes = 5;
 
 class ServiceWorkerVersion::Metrics {
  public:
+  using EventType = ServiceWorkerMetrics::EventType;
   explicit Metrics(ServiceWorkerVersion* owner) : owner_(owner) {}
   ~Metrics() {
-    ServiceWorkerMetrics::RecordEventStatus(fired_events_, handled_events_);
+    for (const auto& ev : event_stats_) {
+      ServiceWorkerMetrics::RecordEventHandledRatio(owner_->scope(), ev.first,
+                                                    ev.second.handled_events,
+                                                    ev.second.fired_events);
+    }
   }
 
-  void RecordEventStatus(bool handled) {
-    ++fired_events_;
+  void RecordEventHandledStatus(EventType event, bool handled) {
+    event_stats_[event].fired_events++;
     if (handled)
-      ++handled_events_;
+      event_stats_[event].handled_events++;
   }
 
   void NotifyStopping() {
@@ -438,9 +446,13 @@ class ServiceWorkerVersion::Metrics {
   }
 
  private:
+  struct EventStat {
+    size_t fired_events = 0;
+    size_t handled_events = 0;
+  };
+
   ServiceWorkerVersion* owner_;
-  size_t fired_events_ = 0;
-  size_t handled_events_ = 0;
+  std::map<EventType, EventStat> event_stats_;
   ServiceWorkerMetrics::StopWorkerStatus stop_status_ =
       ServiceWorkerMetrics::STOP_STATUS_STOPPING;
 
@@ -516,7 +528,6 @@ ServiceWorkerVersion::ServiceWorkerVersion(
       context_(context),
       script_cache_map_(this, context),
       ping_controller_(new PingController(this)),
-      metrics_(new Metrics(this)),
       weak_factory_(this) {
   DCHECK(context_);
   DCHECK(registration);
@@ -1344,7 +1355,8 @@ void ServiceWorkerVersion::OnFetchEventFinished(
 
   // TODO(kinuko): Record other event statuses too.
   const bool handled = (result == SERVICE_WORKER_FETCH_EVENT_RESULT_RESPONSE);
-  metrics_->RecordEventStatus(handled);
+  metrics_->RecordEventHandledStatus(ServiceWorkerMetrics::EVENT_TYPE_FETCH,
+                                     handled);
 
   scoped_refptr<ServiceWorkerVersion> protect(this);
   callback->Run(SERVICE_WORKER_OK, result, response);
@@ -1747,6 +1759,8 @@ void ServiceWorkerVersion::DidEnsureLiveRegistrationForStartWorker(
 }
 
 void ServiceWorkerVersion::StartWorkerInternal(bool pause_after_download) {
+  if (!metrics_)
+    metrics_.reset(new Metrics(this));
   if (!timeout_timer_.IsRunning())
     StartTimeoutTimer();
   if (running_status() == STOPPED) {
@@ -2119,6 +2133,9 @@ void ServiceWorkerVersion::OnStoppedInternal(
   DCHECK_EQ(STOPPED, running_status());
   scoped_refptr<ServiceWorkerVersion> protect(this);
 
+  DCHECK(metrics_);
+  metrics_.reset();
+
   bool should_restart = !is_redundant() && !start_callbacks_.empty() &&
                         (old_status != EmbeddedWorkerInstance::STARTING);