X-Chrome-Variations logic refactoring

chrome/browser/chrome_metrics_helper.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 "chrome/browser/chrome_metrics_helper.h"
+
+#include "base/base64.h"
+#include "base/memory/singleton.h"
+#include "chrome/browser/google/google_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_io_data.h"
+#include "chrome/common/metrics/proto/chrome_experiments.pb.h"
+#include "chrome/common/metrics/variations/variations_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/http/http_request_headers.h"
+
+using content::BrowserThread;
+
+ChromeMetricsHelper* ChromeMetricsHelper::GetInstance() {
+  return Singleton<ChromeMetricsHelper>::get();
+}
+
+void ChromeMetricsHelper::AppendHeaders(const GURL& url,
+                                        bool incognito,
+                                        bool uma_enabled,
+                                        net::HttpRequestHeaders* headers) {
+  // Note the criteria for attaching Chrome experiment headers:
+  // 1. We only transmit to *.google.<TLD> domains. NOTE that this use of
+  //    google_util helpers to check this does not guarantee that the URL is
+  //    Google-owned, only that it is of the form *.google.<TLD>. In the future
+  //    we may choose to reinforce this check.
+  // 2. Only transmit for non-Incognito profiles.

[Peter Kasting, 2012/12/17 19:51:57]

Why this bullet?  Incognito is to protect your local disk, not to anonymize you
to servers.  I don't think we should be checking incognito state here.

[SteveT, 2012/12/17 20:30:46]

We didn't have very strong reasons for removing this from incognito mode in our
discussions way back, but I think our immediate instinct was to shy away from
studying incognito sessions too much.

I don't think we lose too much from NOT receiving this data from incognito
sessions, nor would we gain much if we were to include it.

I've cc'd Tyler who was also involved in the discussions about this decision,
and we can debate further if you wish.

+  // 3. For the X-Chrome-UMA-Enabled bit, only set it if UMA is in fact enabled
+  //    for this install of Chrome.
+  // 4. For the X-Chrome-Variations, only include non-empty variation IDs.
+  if (incognito ||
+      !google_util::IsGoogleDomainUrl(url.spec(),
+                                      google_util::ALLOW_SUBDOMAIN,
+                                      google_util::ALLOW_NON_STANDARD_PORTS)) {
+    return;
+  }
+
+  if (uma_enabled)
+    headers->SetHeaderIfMissing("X-Chrome-UMA-Enabled", "1");
+
+  // Lazily initialize the header, if not already done, before attempting to
+  // transmit it.
+  InitVariationIDsCacheIfNeeded();
+  base::AutoLock scoped_lock(lock_);
+  if (!variation_ids_header_.empty())
+    headers->SetHeaderIfMissing("X-Chrome-Variations", variation_ids_header_);
+}
+
+ChromeMetricsHelper::ChromeMetricsHelper()
+    : variation_ids_cache_initialized_(false) {
+}
+
+ChromeMetricsHelper::~ChromeMetricsHelper() {
+}
+
+void ChromeMetricsHelper::OnFieldTrialGroupFinalized(
+    const std::string& trial_name,
+    const std::string& group_name) {
+  chrome_variations::VariationID new_id =
+      chrome_variations::GetGoogleVariationID(
+          chrome_variations::GOOGLE_WEB_PROPERTIES, trial_name, group_name);
+  if (new_id == chrome_variations::kEmptyID)
+    return;
+  base::AutoLock scoped_lock(lock_);
+  variation_ids_set_.insert(new_id);
+  UpdateVariationIDsHeaderValue();
+}
+
+void ChromeMetricsHelper::InitVariationIDsCacheIfNeeded() {
+  base::AutoLock scoped_lock(lock_);
+  if (variation_ids_cache_initialized_)
+    return;
+
+  // Register for additional cache updates. This is done first to avoid a race
+  // that could cause registered FieldTrials to be missed.
+  DCHECK(MessageLoop::current());
+  base::FieldTrialList::AddObserver(this);
+
+  base::FieldTrial::ActiveGroups initial_groups;
+  base::FieldTrialList::GetActiveFieldTrialGroups(&initial_groups);
+  for (base::FieldTrial::ActiveGroups::const_iterator it =
+       initial_groups.begin(); it != initial_groups.end(); ++it) {
+    const chrome_variations::VariationID id =
+        chrome_variations::GetGoogleVariationID(
+            chrome_variations::GOOGLE_WEB_PROPERTIES, it->trial_name,
+            it->group_name);
+    if (id != chrome_variations::kEmptyID)
+      variation_ids_set_.insert(id);
+  }
+  UpdateVariationIDsHeaderValue();
+
+  variation_ids_cache_initialized_ = true;
+}
+
+void ChromeMetricsHelper::UpdateVariationIDsHeaderValue() {
+  // The header value is a serialized protobuffer of Variation IDs which is
+  // base64 encoded before transmitting as a string.
+  if (variation_ids_set_.empty())
+    return;
+
+  // This is the bottleneck for the creation of the header, so validate the size
+  // here. Force a hard maximum on the ID count in case the Variations server
+  // returns too many IDs and DOSs receiving servers with large requests.
+  DCHECK_LE(variation_ids_set_.size(), 10U);
+  if (variation_ids_set_.size() > 20) {

[Peter Kasting, 2012/12/17 19:51:57]

I read Steve's explanation for this, but the style guide bans what's going on
here.

We can't use DCHECK as a "this probably shouldn't be happening, notify
developers" function, and we can't write subsequent handler code for if the
DCHECKed condition fails.

I suggest simply removing the DCHECK.  I don't think its value is high.  There
are also other options, such as converting it to a DVLOG or something, but in
general we don't tend to collect or read log data, so I would only log if the
logging is definitively going to get noticed were this condition to happen in
the real world.

[SteveT, 2012/12/17 20:30:46]

Again, I believe I mentioned this before, but we (myself + the GWS guys) chose
10 and 20 as almost arbitrary thresholds. It's hard to tell what value might be
considered "too high". If you're interested, we can try to measure the lengths
of the header string at these levels and figure out what might be actually "too
high" to transmit to Google.com all the time.

But you're right in that if the style guide says "a DCHECK means this should
never happen", then we shouldn't be addressing cases that go beyond that point.
I think we still want a DVLOG here to warn developers if we go over, though,
since the <= 10 check was meant to be a warning.

Does that sound fair?

+    variation_ids_header_.clear();
+    return;
+  }
+
+  metrics::ChromeVariations proto;
+  for (std::set<chrome_variations::VariationID>::const_iterator it =
+      variation_ids_set_.begin(); it != variation_ids_set_.end(); ++it)
+    proto.add_variation_id(*it);
+
+  std::string serialized;
+  proto.SerializeToString(&serialized);
+
+  std::string hashed;
+  if (base::Base64Encode(serialized, &hashed)) {
+    // If successful, swap the header value with the new one.
+    // Note that the list of IDs and the header could be temporarily out of sync
+    // if IDs are added as the header is recreated. The receiving servers are OK
+    // with such descrepancies.
+    variation_ids_header_ = hashed;
+  } else {
+    NOTREACHED() << "Failed to base64 encode Variation IDs value: "
+                 << serialized;
+  }
+}