Metrics fix for data reduction proxy.

Refactors histogram increments into virtual methods for mocking.

Adds unit tests that expect the correct metrics calls.

Fixes timing of RecordDataReductionInit() so that it happens when the
proxy is unavailable.


BUG=328579

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243201 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc

@@ -37,49 +37,18 @@
 
 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";
 
@@ -126,6 +95,7 @@ void DataReductionProxySettings::InitPrefMembers() {
 
 void DataReductionProxySettings::InitDataReductionProxySettings() {
   InitPrefMembers();
+  RecordDataReductionInit();
 
   // Disable the proxy if it is not allowed to be used.
   if (!IsDataReductionProxyAllowed())
@@ -140,7 +110,6 @@ void DataReductionProxySettings::InitDataReductionProxySettings() {
   // 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);
@@ -381,7 +350,7 @@ void DataReductionProxySettings::OnURLFetchComplete(
   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;
   }
 
@@ -400,9 +369,9 @@ void DataReductionProxySettings::OnURLFetchComplete(
         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;
@@ -417,9 +386,9 @@ void DataReductionProxySettings::OnURLFetchComplete(
       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;
@@ -553,12 +522,28 @@ void DataReductionProxySettings::SetProxyConfigs(
 
 // Metrics methods
 void DataReductionProxySettings::RecordDataReductionInit() {
-  ProxyStartupState state = PROXY_NOT_AVAILABLE;
-  if (IsDataReductionProxyAllowed())
-    state = IsDataReductionProxyEnabled() ? PROXY_ENABLED : PROXY_DISABLED;
+  ProxyStartupState state = spdyproxy::PROXY_NOT_AVAILABLE;
+  if (IsDataReductionProxyAllowed()) {
+    if (IsDataReductionProxyEnabled())
+      state = spdyproxy::PROXY_ENABLED;
+    else
+      state = spdyproxy::PROXY_DISABLED;
+  }
+
+  RecordStartupState(state);
+}
+
+void DataReductionProxySettings::RecordProbeURLFetchResult(
+    ProbeURLFetchResult result) {
+  UMA_HISTOGRAM_ENUMERATION(kUMAProxyProbeURL,
+                            result,
+                            spdyproxy::PROBE_URL_FETCH_RESULT_COUNT);
+}
+
+void DataReductionProxySettings::RecordStartupState(ProxyStartupState state) {
   UMA_HISTOGRAM_ENUMERATION(kUMAProxyStartupStateHistogram,
                             state,
-                            PROXY_STARTUP_STATE_COUNT);
+                            spdyproxy::PROXY_STARTUP_STATE_COUNT);
 }
 
 DataReductionProxySettings::ContentLengthList

chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h

@@ -37,6 +37,42 @@ const unsigned int kNumDaysInHistorySummary = 30;
 COMPILE_ASSERT(kNumDaysInHistorySummary <= kNumDaysInHistory,
                DataReductionProxySettings_summary_too_long);
 
+  // Values of the UMA DataReductionProxy.StartupState histogram.
+  // This enum must remain synchronized with DataReductionProxyStartupState
+  // in metrics/histograms/histograms.xml.
+  enum ProxyStartupState {
+    PROXY_NOT_AVAILABLE = 0,
+    PROXY_DISABLED,
+    PROXY_ENABLED,
+    PROXY_STARTUP_STATE_COUNT,
+  };
+
+  // Values of the UMA DataReductionProxy.ProbeURL histogram.
+  // This enum must remain synchronized with
+  // DataReductionProxyProbeURLFetchResult in metrics/histograms/histograms.xml.
+  // TODO(marq): Rename these histogram buckets with s/DISABLED/RESTRICTED/, so
+  //     their names match the behavior they track.
+  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 restricted.
+    FAILED_PROXY_ALREADY_DISABLED,
+
+    // THe probe succeeded, and as a result the proxy was restricted.
+    SUCCEEDED_PROXY_ENABLED,
+
+    // The probe succeeded, but the proxy was already restricted.
+    SUCCEEDED_PROXY_ALREADY_ENABLED,
+
+    // This must always be last.
+    PROBE_URL_FETCH_RESULT_COUNT
+  };
+
 }  // namespace spdyproxy
 
 // Central point for configuring the data reduction proxy.
@@ -172,6 +208,10 @@ class DataReductionProxySettings
   // Accessor for unit tests.
   std::vector<std::string> BypassRules() { return bypass_rules_;}
 
+  // Virtualized for mocking
+  virtual void RecordProbeURLFetchResult(spdyproxy::ProbeURLFetchResult result);
+  virtual void RecordStartupState(spdyproxy::ProxyStartupState state);
+
  private:
   friend class DataReductionProxySettingsTestBase;
   friend class DataReductionProxySettingsTest;

chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc

@@ -7,6 +7,8 @@
 #include "base/command_line.h"
 #include "base/md5.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_samples.h"
+#include "base/metrics/statistics_recorder.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
@@ -51,6 +53,20 @@ void HeadersToRaw(std::string* headers) {
     *headers += '\0';
 }
 
+ProbeURLFetchResult FetchResult(bool enabled, bool success) {
+  if (enabled) {
+    if (success)
+      return spdyproxy::SUCCEEDED_PROXY_ALREADY_ENABLED;
+    else
+      return spdyproxy::FAILED_PROXY_DISABLED;
+  } else {
+    if (success)
+      return spdyproxy::SUCCEEDED_PROXY_ENABLED;
+    else
+      return spdyproxy::FAILED_PROXY_ALREADY_DISABLED;
+  }
+}
+
 DataReductionProxySettingsTestBase::DataReductionProxySettingsTestBase()
     : testing::Test() {
 }
@@ -119,13 +135,16 @@ template <class C>
 void DataReductionProxySettingsTestBase::SetProbeResult(
     const std::string& test_url,
     const std::string& response,
+    ProbeURLFetchResult result,
     bool success,
     int expected_calls)  {
   MockDataReductionProxySettings<C>* settings =
       static_cast<MockDataReductionProxySettings<C>*>(settings_.get());
   if (0 == expected_calls) {
     EXPECT_CALL(*settings, GetURLFetcher()).Times(0);
+    EXPECT_CALL(*settings, RecordProbeURLFetchResult(_)).Times(0);
   } else {
+    EXPECT_CALL(*settings, RecordProbeURLFetchResult(result)).Times(1);
     EXPECT_CALL(*settings, GetURLFetcher())
         .Times(expected_calls)
         .WillRepeatedly(Return(new net::FakeURLFetcher(
@@ -143,12 +162,14 @@ template void
 DataReductionProxySettingsTestBase::SetProbeResult<DataReductionProxySettings>(
     const std::string& test_url,
     const std::string& response,
+    ProbeURLFetchResult result,
     bool success,
     int expected_calls);
 template void
 DataReductionProxySettingsTestBase::SetProbeResult<
     DataReductionProxySettingsAndroid>(const std::string& test_url,
                                        const std::string& response,
+                                       ProbeURLFetchResult result,
                                        bool success,
                                        int expected_calls);
 
@@ -184,13 +205,19 @@ void DataReductionProxySettingsTestBase::CheckProbe(
     bool initially_enabled,
     const std::string& probe_url,
     const std::string& response,
-    bool request_success,
+    bool request_succeeded,
     bool expected_enabled,
     bool expected_restricted) {
   pref_service_.SetBoolean(prefs::kSpdyProxyAuthEnabled, initially_enabled);
+  if (initially_enabled)
+    settings_->enabled_by_user_ = true;
   settings_->restricted_by_carrier_ = false;
-  SetProbeResult(
-      probe_url, response, request_success, initially_enabled ? 1 : 0);
+  SetProbeResult(probe_url,
+                 response,
+                 FetchResult(initially_enabled,
+                             request_succeeded && (response == "OK")),
+                 request_succeeded,
+                 initially_enabled ? 1 : 0);
   settings_->MaybeActivateDataReductionProxy(false);
   base::MessageLoop::current()->RunUntilIdle();
   CheckProxyConfigs(expected_enabled, expected_restricted);
@@ -199,9 +226,14 @@ void DataReductionProxySettingsTestBase::CheckProbe(
 void DataReductionProxySettingsTestBase::CheckProbeOnIPChange(
     const std::string& probe_url,
     const std::string& response,
-    bool request_success,
+    bool request_succeeded,
     bool expected_restricted) {
-  SetProbeResult(probe_url, response, request_success, 1);
+  SetProbeResult(probe_url,
+                 response,
+                 FetchResult(!settings_->restricted_by_carrier_,
+                             request_succeeded && (response == "OK")),
+                 request_succeeded,
+                 1);
   settings_->OnIPAddressChanged();
   base::MessageLoop::current()->RunUntilIdle();
   CheckProxyConfigs(true, expected_restricted);
@@ -211,8 +243,11 @@ void DataReductionProxySettingsTestBase::CheckOnPrefChange(
     bool enabled,
     bool expected_enabled) {
   // Always have a sucessful probe for pref change tests.
-  SetProbeResult(
-      kProbeURLWithOKResponse, "OK", true, expected_enabled ? 1 : 0);
+  SetProbeResult(kProbeURLWithOKResponse,
+                 "OK",
+                 FetchResult(enabled, true),
+                 true,
+                 expected_enabled ? 1 : 0);
   pref_service_.SetBoolean(prefs::kSpdyProxyAuthEnabled, enabled);
   base::MessageLoop::current()->RunUntilIdle();
   // Never expect the proxy to be restricted for pref change tests.
@@ -224,8 +259,11 @@ void DataReductionProxySettingsTestBase::CheckInitDataReductionProxy(
   AddProxyToCommandLine();
   base::MessageLoopForUI loop;
   pref_service_.SetBoolean(prefs::kSpdyProxyAuthEnabled, enabled_at_startup);
-  SetProbeResult(
-      kProbeURLWithOKResponse, "OK", true, enabled_at_startup ? 1 : 0);
+  SetProbeResult(kProbeURLWithOKResponse,
+                 "OK",
+                 FetchResult(enabled_at_startup, true),
+                 true,
+                 enabled_at_startup ? 1 : 0);
   settings_->InitDataReductionProxySettings();
   base::MessageLoop::current()->RunUntilIdle();
   if (enabled_at_startup) {
@@ -497,6 +535,10 @@ TEST_F(DataReductionProxySettingsTest, TestOnIPAddressChanged) {
   AddProxyToCommandLine();
   base::MessageLoopForUI loop;
   // The proxy is enabled initially.
+  pref_service_.SetBoolean(prefs::kSpdyProxyAuthEnabled, true);
+  settings_->spdy_proxy_auth_enabled_.Init(
+      prefs::kSpdyProxyAuthEnabled,
+      settings_->GetOriginalProfilePrefs());
   settings_->enabled_by_user_ = true;
   settings_->restricted_by_carrier_ = false;
   settings_->SetProxyConfigs(true, false, true);
@@ -525,6 +567,8 @@ TEST_F(DataReductionProxySettingsTest, TestOnProxyEnabledPrefChange) {
 }
 
 TEST_F(DataReductionProxySettingsTest, TestInitDataReductionProxyOn) {
+  MockSettings* settings = static_cast<MockSettings*>(settings_.get());
+  EXPECT_CALL(*settings, RecordStartupState(spdyproxy::PROXY_ENABLED));
   CheckInitDataReductionProxy(true);
 }
 
@@ -533,6 +577,7 @@ TEST_F(DataReductionProxySettingsTest, TestInitDataReductionProxyOff) {
   // LogProxyState.
   MockSettings* settings = static_cast<MockSettings*>(settings_.get());
   EXPECT_CALL(*settings, LogProxyState(false, false, true)).Times(1);
+  EXPECT_CALL(*settings, RecordStartupState(spdyproxy::PROXY_DISABLED));
   CheckInitDataReductionProxy(false);
 }
 
@@ -616,3 +661,12 @@ TEST_F(DataReductionProxySettingsTest, WasFetchedViaProxy) {
               DataReductionProxySettings::WasFetchedViaProxy(parsed));
   }
 }
+
+TEST_F(DataReductionProxySettingsTest, CheckInitMetricsWhenNotAllowed) {
+  // No call to |AddProxyToCommandLine()| was made, so the proxy feature
+  // should be unavailable.
+  EXPECT_FALSE(DataReductionProxySettings::IsDataReductionProxyAllowed());
+  MockSettings* settings = static_cast<MockSettings*>(settings_.get());
+  EXPECT_CALL(*settings, RecordStartupState(spdyproxy::PROXY_NOT_AVAILABLE));
+  settings_->InitDataReductionProxySettings();
+}

chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.h

@@ -7,6 +7,7 @@
 
 
 #include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_samples.h"
 #include "base/prefs/testing_pref_service.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h"
 #include "net/url_request/test_url_fetcher_factory.h"
@@ -16,6 +17,9 @@
 class PrefService;
 class TestingPrefServiceSimple;
 
+using spdyproxy::ProbeURLFetchResult;
+using spdyproxy::ProxyStartupState;
+
 template <class C>
 class MockDataReductionProxySettings : public C {
  public:
@@ -24,6 +28,10 @@ class MockDataReductionProxySettings : public C {
   MOCK_METHOD0(GetLocalStatePrefs, PrefService*());
   MOCK_METHOD3(LogProxyState, void(
       bool enabled, bool restricted, bool at_startup));
+  MOCK_METHOD1(RecordProbeURLFetchResult,
+               void(ProbeURLFetchResult result));
+  MOCK_METHOD1(RecordStartupState,
+               void(ProxyStartupState state));
 
   // SetProxyConfigs should always call LogProxyState exactly once.
   virtual void SetProxyConfigs(
@@ -48,10 +56,12 @@ class DataReductionProxySettingsTestBase : public testing::Test {
   template <class C> void SetProbeResult(
       const std::string& test_url,
       const std::string& response,
+      ProbeURLFetchResult state,
       bool success,
       int expected_calls);
   virtual void SetProbeResult(const std::string& test_url,
                               const std::string& response,
+                              ProbeURLFetchResult result,
                               bool success,
                               int expected_calls) = 0;
 
@@ -92,10 +102,11 @@ class ConcreteDataReductionProxySettingsTest
 
   virtual void SetProbeResult(const std::string& test_url,
                               const std::string& response,
+                              ProbeURLFetchResult result,
                               bool success,
                               int expected_calls) OVERRIDE {
     return DataReductionProxySettingsTestBase::SetProbeResult<C>(
-  test_url, response, success, expected_calls);
+  test_url, response, result, success, expected_calls);
   }
 };