Metrics fix for data reduction proxy.

chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc

 // 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/net/spdyproxy/data_reduction_proxy_settings.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
@@ skipping 19 matching lines @@
 #include "net/http/http_auth_cache.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_status.h"
 #include "url/gurl.h"
 
 using base::FieldTrialList;
 using base::StringPrintf;
+using spdyproxy::ProbeURLFetchResult;
+using spdyproxy::ProxyStartupState;
 
 namespace {
 
 // Key of the UMA DataReductionProxy.StartupState histogram.
 const char kUMAProxyStartupStateHistogram[] =
     "DataReductionProxy.StartupState";
-// Values of the UMA DataReductionProxy.StartupState histogram.
-enum ProxyStartupState {
-  PROXY_NOT_AVAILABLE = 0,
-  PROXY_DISABLED,
-  PROXY_ENABLED,
-  PROXY_STARTUP_STATE_COUNT,
-};
 
 // Key of the UMA DataReductionProxy.ProbeURL histogram.
 const char kUMAProxyProbeURL[] = "DataReductionProxy.ProbeURL";
-// Values of the UMA DataReductionProxy.ProbeURL histogram.
-// This enum must remain synchronized with DataReductionProxyProbeURLFetchResult
-// in metrics/histograms/histograms.xml.
-enum ProbeURLFetchResult {
-  // The probe failed because the internet was disconnected.
-  INTERNET_DISCONNECTED = 0,
 
-  // The probe failed for any other reason, and as a result, the proxy was
-  // disabled.
-  FAILED_PROXY_DISABLED,
-
-  // The probe failed, but the proxy was already disabled.
-  FAILED_PROXY_ALREADY_DISABLED,
-
-  // THe probe succeeded, and as a result the proxy was enabled.
-  SUCCEEDED_PROXY_ENABLED,
-
-  // The probe succeeded, but the proxy was already enabled.
-  SUCCEEDED_PROXY_ALREADY_ENABLED,
-
-  // This must always be last.
-  FETCH_RESULT_COUNT
-};
-
-void RecordProbeURLFetchResult(ProbeURLFetchResult result) {
-  UMA_HISTOGRAM_ENUMERATION(kUMAProxyProbeURL, result, FETCH_RESULT_COUNT);
-}
 
 const char kEnabled[] = "Enabled";
 
 // TODO(marq): Factor this string out into a constant here and in
 //             http_auth_handler_spdyproxy.
 const char kAuthenticationRealmName[] = "SpdyProxy";
 
 int64 GetInt64PrefValue(const ListValue& list_value, size_t index) {
   int64 val = 0;
   std::string pref_value;
@@ skipping 26 matching lines @@
 void DataReductionProxySettings::InitPrefMembers() {
   spdy_proxy_auth_enabled_.Init(
       prefs::kSpdyProxyAuthEnabled,
       GetOriginalProfilePrefs(),
       base::Bind(&DataReductionProxySettings::OnProxyEnabledPrefChange,
                  base::Unretained(this)));
 }
 
 void DataReductionProxySettings::InitDataReductionProxySettings() {
   InitPrefMembers();
+  RecordDataReductionInit();

[bengr, 2014/01/02 17:30:40]

Did you move this here so we record something even if the data reduction proxy
is not supported on the platform? Does this mean that every platform (Windows,
etc) will have UMA for the data reduction proxy?

[marq (ping after 24h), 2014/01/02 20:45:47]

No, because this code only exists for the mobile platforms because of how we
have it set up in .gyp, and because there's only code on those platforms to
create and init a DataReductionProxySettings object.

This change is the actual bug fix -- without it, we will never record the
PROXY_NOT_AVAILABLE startup state.

 
   // Disable the proxy if it is not allowed to be used.
   if (!IsDataReductionProxyAllowed())
     return;
 
   AddDefaultProxyBypassRules();
   net::NetworkChangeNotifier::AddIPAddressObserver(this);
 
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
 
   // Setting the kEnableSpdyProxyAuth switch has the same effect as enabling
   // the feature via settings, in that once set, the preference will be sticky
   // across instances of Chrome. Disabling the feature can only be done through
   // the settings menu.
-  RecordDataReductionInit();
   if (spdy_proxy_auth_enabled_.GetValue() ||
       command_line.HasSwitch(switches::kEnableSpdyProxyAuth)) {
     MaybeActivateDataReductionProxy(true);
   } else {
     // This is logged so we can use this information in user feedback.
     LogProxyState(false /* enabled */,
                   false /* restricted */,
                   true /* at startup */);
   }
 }
@@ skipping 218 matching lines @@
 DataReductionProxySettings::ContentLengthList
 DataReductionProxySettings::GetDailyReceivedContentLengths() {
   return GetDailyContentLengths(prefs::kDailyHttpReceivedContentLength);
 }
 
 void DataReductionProxySettings::OnURLFetchComplete(
     const net::URLFetcher* source) {
   net::URLRequestStatus status = source->GetStatus();
   if (status.status() == net::URLRequestStatus::FAILED &&
       status.error() == net::ERR_INTERNET_DISCONNECTED) {
-    RecordProbeURLFetchResult(INTERNET_DISCONNECTED);
+    RecordProbeURLFetchResult(spdyproxy::INTERNET_DISCONNECTED);
     return;
   }
 
   std::string response;
   source->GetResponseAsString(&response);
 
   if ("OK" == response.substr(0, 2)) {
     DVLOG(1) << "The data reduction proxy is unrestricted.";
 
     if (enabled_by_user_) {
       if (restricted_by_carrier_) {
         // The user enabled the proxy, but sometime previously in the session,
         // the network operator had blocked the canary and restricted the user.
         // The current network doesn't block the canary, so don't restrict the
         // proxy configurations.
         SetProxyConfigs(true /* enabled */,
                         false /* restricted */,
                         false /* at_startup */);
-        RecordProbeURLFetchResult(SUCCEEDED_PROXY_ENABLED);
+        RecordProbeURLFetchResult(spdyproxy::SUCCEEDED_PROXY_ENABLED);
       } else {
-        RecordProbeURLFetchResult(SUCCEEDED_PROXY_ALREADY_ENABLED);
+        RecordProbeURLFetchResult(spdyproxy::SUCCEEDED_PROXY_ALREADY_ENABLED);
       }
     }
     restricted_by_carrier_ = false;
     return;
   }
   DVLOG(1) << "The data reduction proxy is restricted to the configured "
            << "fallback proxy.";
 
   if (enabled_by_user_) {
     if (!restricted_by_carrier_) {
       // Restrict the proxy.
       SetProxyConfigs(true /* enabled */,
                       true /* restricted */,
                       false /* at_startup */);
-      RecordProbeURLFetchResult(FAILED_PROXY_DISABLED);
+      RecordProbeURLFetchResult(spdyproxy::FAILED_PROXY_DISABLED);
     } else {
-      RecordProbeURLFetchResult(FAILED_PROXY_ALREADY_DISABLED);
+      RecordProbeURLFetchResult(spdyproxy::FAILED_PROXY_ALREADY_DISABLED);
     }
   }
   restricted_by_carrier_ = true;
 }
 
 void DataReductionProxySettings::OnIPAddressChanged() {
   if (enabled_by_user_) {
     DCHECK(IsDataReductionProxyAllowed());
     ProbeWhetherDataReductionProxyIsAvailable();
   }
@@ skipping 113 matching lines @@
     dict->SetString("bypass_list", JoinString(bypass_rules_, ", "));
   } else {
     dict->SetString("mode", ProxyModeToString(ProxyPrefs::MODE_SYSTEM));
     dict->SetString("server", "");
     dict->SetString("bypass_list", "");
   }
 }
 
 // Metrics methods
 void DataReductionProxySettings::RecordDataReductionInit() {
-  ProxyStartupState state = PROXY_NOT_AVAILABLE;
+  ProxyStartupState state = spdyproxy::PROXY_NOT_AVAILABLE;
   if (IsDataReductionProxyAllowed())
-    state = IsDataReductionProxyEnabled() ? PROXY_ENABLED : PROXY_DISABLED;
+    state = IsDataReductionProxyEnabled() ? spdyproxy::PROXY_ENABLED

[bengr, 2014/01/02 17:30:40]

It's probably cleaner now to write this as:

if (IsDataReductionProxyExabled())
  state = spdyproxy::PROXY_ENABLED;
else
  state = spdyproxy::PROXY_DISABLED;

[marq (ping after 24h), 2014/01/02 20:45:47]

Done.

+                                          : spdyproxy::PROXY_DISABLED;
+  RecordStartupState(state);
+}
+
+void DataReductionProxySettings::RecordProbeURLFetchResult(
+    ProbeURLFetchResult result) {

[bengr, 2014/01/02 17:30:40]

Can this fit on the line above?

[marq (ping after 24h), 2014/01/02 20:45:47]

Not by nine characters.

+  UMA_HISTOGRAM_ENUMERATION(kUMAProxyProbeURL,
+                            result,
+                            spdyproxy::FETCH_RESULT_COUNT);
+}
+
+void DataReductionProxySettings::RecordStartupState(ProxyStartupState state) {
   UMA_HISTOGRAM_ENUMERATION(kUMAProxyStartupStateHistogram,
                             state,
-                            PROXY_STARTUP_STATE_COUNT);
+                            spdyproxy::PROXY_STARTUP_STATE_COUNT);
 }
 
 DataReductionProxySettings::ContentLengthList
 DataReductionProxySettings::GetDailyContentLengths(const char* pref_name) {
   DataReductionProxySettings::ContentLengthList content_lengths;
   const ListValue* list_value = GetLocalStatePrefs()->GetList(pref_name);
   if (list_value->GetSize() == spdyproxy::kNumDaysInHistory) {
     for (size_t i = 0; i < spdyproxy::kNumDaysInHistory; ++i) {
       content_lengths.push_back(GetInt64PrefValue(*list_value, i));
     }
@@ skipping 107 matching lines @@
 }
 
 void
 DataReductionProxySettings::ProbeWhetherDataReductionProxyIsAvailable() {
   net::URLFetcher* fetcher = GetURLFetcher();
   if (!fetcher)
     return;
   fetcher_.reset(fetcher);
   fetcher_->Start();
 }