Added of new UMA signals in order to be able to discover early if the "save password" feature gets …

chrome/browser/password_manager/password_manager_metrics_util.cc

Index: chrome/browser/password_manager/password_manager_metrics_util.cc
diff --git a/chrome/browser/password_manager/password_manager_metrics_util.cc b/chrome/browser/password_manager/password_manager_metrics_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e1ade33f3e90837033391a751c75d23dc95af27c
--- /dev/null
+++ b/chrome/browser/password_manager/password_manager_metrics_util.cc
@@ -0,0 +1,96 @@
+// Copyright 2013 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 "chrome/browser/password_manager/password_manager_metrics_util.h"
+
+#include "base/basictypes.h"
+#include "base/metrics/histogram.h"
+#include "base/process/process_handle.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/time/time.h"
+#include "url/gurl.h"
+
+namespace password_manager_metrics_util {
+
+std::string IsDomainNameMonitored(const std::string& url_host) {
+  static const size_t kBucketsPerDomain = 10u;

[Ilya Sherman, 2013/09/10 22:30:54]

This terminology is a little confusing, as "buckets" already has a meaning
within the context of histograms -- a single histogram has multiple buckets. 
I'd recommend using a different term to describe the association of domains with
ids -- perhaps "groups" or "sets" or something like that.

[jdomingos, 2013/09/11 15:50:30]

Done.

+  // This array contains each monitored websites together with all id of
+  // buckets which contain the website.
+  static const struct DomainBucketsPair kInfos[] = {

[Ilya Sherman, 2013/09/10 22:30:54]

nit: Extraneous "struct"

[Ilya Sherman, 2013/09/10 22:30:54]

nit: Perhaps |kDomainMapping|?

[jdomingos, 2013/09/11 15:50:30]

Done.

[jdomingos, 2013/09/11 15:50:30]

Done.

+      {"google.com", {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}},
+      {"yahoo.com", {1, 2, 3, 4, 5, 11, 12, 13, 14, 15}},
+      {"baidu.com", {1, 2, 3, 4, 6, 7, 11, 12, 16, 17}},
+      {"wikipedia.org", {1, 2, 3, 4, 5, 6, 11, 12, 16, 18}},
+      {"linkedin.com", {1, 6, 8, 11, 13, 14, 15, 16, 17, 19}},
+      {"twitter.com", {5, 6, 7, 8, 9, 11, 13, 17, 19, 20}},
+      {"live.com", {7, 8, 9, 10, 13, 14, 16, 17, 18, 20}},
+      {"amazon.com", {2, 5, 9, 10, 12, 14, 15, 18, 19, 20}},
+      {"ebay.com", {13, 7, 9, 10, 14, 15, 17, 18, 19, 20}},
+      {"tumblr.com", {4, 8, 10, 12, 13, 15, 16, 18, 19, 20}},

[Ilya Sherman, 2013/09/10 22:30:54]

This bucket distribution doesn't really seem well balanced -- I'd expect each
bucket to appear an equal number of times, but '1' appears 5 times, whereas '3'
only appears 4 times.

[vabr (Chromium), 2013/09/11 07:25:43]

Good catch, Ilya!
ebay.com should have started with 3, not 13.
Jordy, can you please double-check for other typos? The way we generated this,
each bucket should have appeared 5 times (in other words, each bucket contains
exactly 5 hosts).

[jdomingos, 2013/09/11 15:50:30]

Thanks Ilya and Vaclav.
I am going to double-check for other typos.

+  };
+  const size_t kInfosLength = arraysize(kInfos);

[Ilya Sherman, 2013/09/10 22:30:54]

This needs a lot more documentation.  Please explain the algorithm: what the
bucket ids mean, how they were chosen, etc.

[Ilya Sherman, 2013/09/10 22:30:54]

Suppose we wanted to add tracking for an additional website -- say, facebook
(incidentally, why isn't that already included?).  Would there be a way to add
the new website while keeping the bucket ids stable?  If not, that seems like a
pretty serious flaw with this design, as the histogram meanings will need to
change every time we change which set of websites we're interested in tracking.

[vabr (Chromium), 2013/09/11 07:25:43]

Jordy is putting all of it in the design doc (currently
https://docs.google.com/a/google.com/document/d/1AEeaQGxKy0sKhZENTH1WWf8K7sfk...,
but the algorithm is not yet there), and we should put a short link here before
landing it.

Jordy, one technical thing to do: you need to start a new document in your
chromium.org account and add view rights for everyone with that link. Then we
should copy the current design doc over and mark the old one as obsolete. This
is because you cannot share a google.com-owned document outside google.com,
which would be a pita for non-Google Chrome contributors. Also the URL shortener
needs to be public, e.g., goo.gl.

[vabr (Chromium), 2013/09/11 07:25:43]

Unfortunately, there is no way to add websites while keeping bucket ids stable,
unless we just make a new independent group of those websites.
How often do we expect to add sites, though? Is the site's popularity changing
that often that we won't be interested in how the heuristics work on the above
soon?
If we are about to change that once a year or less frequently, which is what I
expect, then we should be fine, because a couple of weeks of history on those
histograms should be OK to detect drastic changes due to breakage.

[jdomingos, 2013/09/11 15:50:30]

Done.

[jdomingos, 2013/09/11 15:50:30]

Facebook is not already included because the save password prompt is not working
properly on it. I will try to see what is the problem and fix it if possible.

+
+  GURL url(url_host);
+  for (size_t i = 0; i < kInfosLength; ++i) {
+    std::string domain = kInfos[i].domain_name;
+    // For each session, the user uses only one bucket for each website.
+    if (url.DomainIs(domain.c_str())) {
+      // Use GetCurrentProcId % kBucketsPerDomain to get a number which:
+      // 1. Is stable during the lifetime of the browser.
+      // 2. Is sufficiently random, in both the sense that
+      //   2a. it's not easy to predict for a specific user, and
+      //   2b. all kBucketsPerDomain possible values are roughly uniformly
+      //       used across all users.

[Ilya Sherman, 2013/09/10 22:30:54]

Why not just compute a truly random number once per run, by caching the number
in a static variable?

[vabr (Chromium), 2013/09/11 07:25:43]

Seems like a good idea!

[jdomingos, 2013/09/11 15:50:30]

Done.

+      return "bucket_" +
+             base::IntToString(
+                 kInfos[i]
+                     .bucket_ids[base::GetCurrentProcId() % kBucketsPerDomain]);

[Ilya Sherman, 2013/09/10 22:30:54]

Rather than splitting this statement over four lines, please introduce a
temporary named variable and write it as two statements.

[jdomingos, 2013/09/11 15:50:30]

Done.

[jdomingos, 2013/09/11 15:50:30]

Done.

+    }
+  }
+  return std::string();
+}
+
+void LogUMAHistogramEnumeration(const std::string& name,
+                                int sample,
+                                int boundary_value) {
+  DCHECK_LT(sample, boundary_value);
+
+  // Note: This leaks memory, which is expected behavior.
+  base::HistogramBase* histogram =
+      base::LinearHistogram::FactoryGet(
+          name,
+          1,
+          boundary_value,
+          boundary_value + 1,
+          base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram->Add(sample);
+}
+
+void LogUMAHistogramTimes(const std::string& name,
+                          const base::TimeDelta& duration) {
+  // Note: This leaks memory, which is expected behavior.
+  base::HistogramBase* histogram =
+      base::Histogram::FactoryTimeGet(
+          name,
+          base::TimeDelta::FromMilliseconds(1),
+          base::TimeDelta::FromSeconds(10),
+          50,
+          base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram->AddTime(duration);
+}
+
+void LogUMAHistogramCount(const std::string& name, int sample) {
+  // Note: This leaks memory, which is expected behavior.
+  base::HistogramBase* histogram =
+      base::Histogram::FactoryGet(
+        name,
+        1,
+        1000000,
+        50,
+        base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram->Add(sample);
+}
+
+}  // namespace password_manager_metrics_util