Change UMA proto product field to be an int32.

This makes it easier for downstream clients of the
component to add new product types without needing
to update the component source.

Also, adds a client method to return the product, so
that different MetricsService clients can provide
different values.

Also cleans up some unnecessary metrics:: namespace
prefixes.

BUG=415581

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

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

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -179,6 +179,10 @@ bool ChromeMetricsServiceClient::IsOffTheRecordSessionActive() {
   return chrome::IsOffTheRecordSessionActive();
 }
 
+int32 ChromeMetricsServiceClient::GetProduct() {
+  return metrics::ChromeUserMetricsExtension::CHROME;
+}
+
 std::string ChromeMetricsServiceClient::GetApplicationLocale() {
   return g_browser_process->GetApplicationLocale();
 }

chrome/browser/metrics/chrome_metrics_service_client.h

@@ -58,6 +58,7 @@ class ChromeMetricsServiceClient
   // metrics::MetricsServiceClient:
   virtual void SetMetricsClientId(const std::string& client_id) OVERRIDE;
   virtual bool IsOffTheRecordSessionActive() OVERRIDE;
+  virtual int32 GetProduct() OVERRIDE;
   virtual std::string GetApplicationLocale() OVERRIDE;
   virtual bool GetBrand(std::string* brand_code) OVERRIDE;
   virtual metrics::SystemProfileProto::Channel GetChannel() OVERRIDE;

chromecast/metrics/cast_metrics_service_client.cc

@@ -54,6 +54,11 @@ bool CastMetricsServiceClient::IsOffTheRecordSessionActive() {
   return false;
 }
 
+int32_t CastMetricsServiceClient::GetProduct() {
+  // Chromecast currently uses the same product identifier as Chrome.
+  return metrics::ChromeUserMetricsExtension::CHROME;
+}
+
 std::string CastMetricsServiceClient::GetApplicationLocale() {
   return base::i18n::GetConfiguredLocale();
 }

chromecast/metrics/cast_metrics_service_client.h

@@ -43,6 +43,7 @@ class CastMetricsServiceClient : public ::metrics::MetricsServiceClient {
   // metrics::MetricsServiceClient implementation:
   virtual void SetMetricsClientId(const std::string& client_id) OVERRIDE;
   virtual bool IsOffTheRecordSessionActive() OVERRIDE;
+  virtual int32_t GetProduct() OVERRIDE;
   virtual std::string GetApplicationLocale() OVERRIDE;
   virtual bool GetBrand(std::string* brand_code) OVERRIDE;
   virtual ::metrics::SystemProfileProto::Channel GetChannel() OVERRIDE;

components/metrics/metrics_log.cc

@@ -62,7 +62,7 @@ std::string GetMetricsEnabledDate(PrefService* pref) {
     return "0";
   }
 
-  return pref->GetString(metrics::prefs::kMetricsReportingEnabledTimestamp);
+  return pref->GetString(prefs::kMetricsReportingEnabledTimestamp);
 }
 
 // Computes a SHA-1 hash of |data| and returns it as a hex string.
@@ -94,7 +94,7 @@ int64 RoundSecondsToHour(int64 time_in_seconds) {
 MetricsLog::MetricsLog(const std::string& client_id,
                        int session_id,
                        LogType log_type,
-                       metrics::MetricsServiceClient* client,
+                       MetricsServiceClient* client,
                        PrefService* local_state)
     : closed_(false),
       log_type_(log_type),
@@ -108,6 +108,11 @@ MetricsLog::MetricsLog(const std::string& client_id,
 
   uma_proto_.set_session_id(session_id);
 
+  const int32 product = client_->GetProduct();
+  // Only set the product if it differs from the default value.
+  if (product != uma_proto_.product())
+    uma_proto_.set_product(product);
+
   SystemProfileProto* system_profile = uma_proto_.mutable_system_profile();
   system_profile->set_build_timestamp(GetBuildTime());
   system_profile->set_app_version(client_->GetVersionString());
@@ -119,26 +124,23 @@ MetricsLog::~MetricsLog() {
 
 // static
 void MetricsLog::RegisterPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterIntegerPref(metrics::prefs::kStabilityLaunchCount, 0);
-  registry->RegisterIntegerPref(metrics::prefs::kStabilityCrashCount, 0);
-  registry->RegisterIntegerPref(
-      metrics::prefs::kStabilityIncompleteSessionEndCount, 0);
-  registry->RegisterIntegerPref(
-      metrics::prefs::kStabilityBreakpadRegistrationFail, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityLaunchCount, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityCrashCount, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityIncompleteSessionEndCount, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityBreakpadRegistrationFail, 0);
   registry->RegisterIntegerPref(
-      metrics::prefs::kStabilityBreakpadRegistrationSuccess, 0);
-  registry->RegisterIntegerPref(metrics::prefs::kStabilityDebuggerPresent, 0);
-  registry->RegisterIntegerPref(metrics::prefs::kStabilityDebuggerNotPresent,
-                                0);
-  registry->RegisterStringPref(metrics::prefs::kStabilitySavedSystemProfile,
+      prefs::kStabilityBreakpadRegistrationSuccess, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityDebuggerPresent, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityDebuggerNotPresent, 0);
+  registry->RegisterStringPref(prefs::kStabilitySavedSystemProfile,
                                std::string());
-  registry->RegisterStringPref(metrics::prefs::kStabilitySavedSystemProfileHash,
+  registry->RegisterStringPref(prefs::kStabilitySavedSystemProfileHash,
                                std::string());
 }
 
 // static
 uint64 MetricsLog::Hash(const std::string& value) {
-  uint64 hash = metrics::HashMetricName(value);
+  uint64 hash = HashMetricName(value);
 
   // The following log is VERY helpful when folks add some named histogram into
   // the code, but forgot to update the descriptive list of histograms.  When
@@ -213,7 +215,7 @@ void MetricsLog::RecordHistogramDelta(const std::string& histogram_name,
 }
 
 void MetricsLog::RecordStabilityMetrics(
-    const std::vector<metrics::MetricsProvider*>& metrics_providers,
+    const std::vector<MetricsProvider*>& metrics_providers,
     base::TimeDelta incremental_uptime,
     base::TimeDelta uptime) {
   DCHECK(!closed_);
@@ -244,20 +246,20 @@ void MetricsLog::RecordStabilityMetrics(
     return;
 
   int incomplete_shutdown_count =
-      pref->GetInteger(metrics::prefs::kStabilityIncompleteSessionEndCount);
-  pref->SetInteger(metrics::prefs::kStabilityIncompleteSessionEndCount, 0);
+      pref->GetInteger(prefs::kStabilityIncompleteSessionEndCount);
+  pref->SetInteger(prefs::kStabilityIncompleteSessionEndCount, 0);
   int breakpad_registration_success_count =
-      pref->GetInteger(metrics::prefs::kStabilityBreakpadRegistrationSuccess);
-  pref->SetInteger(metrics::prefs::kStabilityBreakpadRegistrationSuccess, 0);
+      pref->GetInteger(prefs::kStabilityBreakpadRegistrationSuccess);
+  pref->SetInteger(prefs::kStabilityBreakpadRegistrationSuccess, 0);
   int breakpad_registration_failure_count =
-      pref->GetInteger(metrics::prefs::kStabilityBreakpadRegistrationFail);
-  pref->SetInteger(metrics::prefs::kStabilityBreakpadRegistrationFail, 0);
+      pref->GetInteger(prefs::kStabilityBreakpadRegistrationFail);
+  pref->SetInteger(prefs::kStabilityBreakpadRegistrationFail, 0);
   int debugger_present_count =
-      pref->GetInteger(metrics::prefs::kStabilityDebuggerPresent);
-  pref->SetInteger(metrics::prefs::kStabilityDebuggerPresent, 0);
+      pref->GetInteger(prefs::kStabilityDebuggerPresent);
+  pref->SetInteger(prefs::kStabilityDebuggerPresent, 0);
   int debugger_not_present_count =
-      pref->GetInteger(metrics::prefs::kStabilityDebuggerNotPresent);
-  pref->SetInteger(metrics::prefs::kStabilityDebuggerNotPresent, 0);
+      pref->GetInteger(prefs::kStabilityDebuggerNotPresent);
+  pref->SetInteger(prefs::kStabilityDebuggerNotPresent, 0);
 
   // TODO(jar): The following are all optional, so we *could* optimize them for
   // values of zero (and not include them).
@@ -273,7 +275,7 @@ void MetricsLog::RecordStabilityMetrics(
 }
 
 void MetricsLog::RecordGeneralMetrics(
-    const std::vector<metrics::MetricsProvider*>& metrics_providers) {
+    const std::vector<MetricsProvider*>& metrics_providers) {
   for (size_t i = 0; i < metrics_providers.size(); ++i)
     metrics_providers[i]->ProvideGeneralMetrics(uma_proto());
 }
@@ -296,10 +298,10 @@ bool MetricsLog::HasStabilityMetrics() const {
 // TODO(isherman): Stop writing these attributes specially once the migration to
 // protobufs is complete.
 void MetricsLog::WriteRequiredStabilityAttributes(PrefService* pref) {
-  int launch_count = pref->GetInteger(metrics::prefs::kStabilityLaunchCount);
-  pref->SetInteger(metrics::prefs::kStabilityLaunchCount, 0);
-  int crash_count = pref->GetInteger(metrics::prefs::kStabilityCrashCount);
-  pref->SetInteger(metrics::prefs::kStabilityCrashCount, 0);
+  int launch_count = pref->GetInteger(prefs::kStabilityLaunchCount);
+  pref->SetInteger(prefs::kStabilityLaunchCount, 0);
+  int crash_count = pref->GetInteger(prefs::kStabilityCrashCount);
+  pref->SetInteger(prefs::kStabilityCrashCount, 0);
 
   SystemProfileProto::Stability* stability =
       uma_proto()->mutable_system_profile()->mutable_stability();
@@ -327,7 +329,7 @@ void MetricsLog::WriteRealtimeStabilityAttributes(
 }
 
 void MetricsLog::RecordEnvironment(
-    const std::vector<metrics::MetricsProvider*>& metrics_providers,
+    const std::vector<MetricsProvider*>& metrics_providers,
     const std::vector<variations::ActiveGroupId>& synthetic_trials,
     int64 install_date) {
   DCHECK(!HasEnvironment());
@@ -398,24 +400,24 @@ void MetricsLog::RecordEnvironment(
   if (system_profile->SerializeToString(&serialied_system_profile)) {
     base::Base64Encode(serialied_system_profile, &base64_system_profile);
     PrefService* local_state = local_state_;
-    local_state->SetString(metrics::prefs::kStabilitySavedSystemProfile,
+    local_state->SetString(prefs::kStabilitySavedSystemProfile,
                            base64_system_profile);
-    local_state->SetString(metrics::prefs::kStabilitySavedSystemProfileHash,
+    local_state->SetString(prefs::kStabilitySavedSystemProfileHash,
                            ComputeSHA1(serialied_system_profile));
   }
 }
 
 bool MetricsLog::LoadSavedEnvironmentFromPrefs() {
   PrefService* local_state = local_state_;
   const std::string base64_system_profile =
-      local_state->GetString(metrics::prefs::kStabilitySavedSystemProfile);
+      local_state->GetString(prefs::kStabilitySavedSystemProfile);
   if (base64_system_profile.empty())
     return false;
 
   const std::string system_profile_hash =
-      local_state->GetString(metrics::prefs::kStabilitySavedSystemProfileHash);
-  local_state->ClearPref(metrics::prefs::kStabilitySavedSystemProfile);
-  local_state->ClearPref(metrics::prefs::kStabilitySavedSystemProfileHash);
+      local_state->GetString(prefs::kStabilitySavedSystemProfileHash);
+  local_state->ClearPref(prefs::kStabilitySavedSystemProfile);
+  local_state->ClearPref(prefs::kStabilitySavedSystemProfileHash);
 
   SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
   std::string serialied_system_profile;

components/metrics/metrics_log_unittest.cc

@@ -46,26 +46,26 @@ class TestMetricsLog : public MetricsLog {
   TestMetricsLog(const std::string& client_id,
                  int session_id,
                  LogType log_type,
-                 metrics::MetricsServiceClient* client,
+                 MetricsServiceClient* client,
                  TestingPrefServiceSimple* prefs)
       : MetricsLog(client_id, session_id, log_type, client, prefs),
         prefs_(prefs) {
     InitPrefs();
- }
+  }
 
   virtual ~TestMetricsLog() {}
 
-  const metrics::ChromeUserMetricsExtension& uma_proto() const {
+  const ChromeUserMetricsExtension& uma_proto() const {
     return *MetricsLog::uma_proto();
   }
 
-  const metrics::SystemProfileProto& system_profile() const {
+  const SystemProfileProto& system_profile() const {
     return uma_proto().system_profile();
   }
 
  private:
   void InitPrefs() {
-    prefs_->SetString(metrics::prefs::kMetricsReportingEnabledTimestamp,
+    prefs_->SetString(prefs::kMetricsReportingEnabledTimestamp,
                       base::Int64ToString(kEnabledDate));
   }
 
@@ -91,7 +91,7 @@ class MetricsLogTest : public testing::Test {
  public:
   MetricsLogTest() {
     MetricsLog::RegisterPrefs(prefs_.registry());
-    metrics::MetricsStateManager::RegisterPrefs(prefs_.registry());
+    MetricsStateManager::RegisterPrefs(prefs_.registry());
   }
 
   virtual ~MetricsLogTest() {
@@ -100,30 +100,30 @@ class MetricsLogTest : public testing::Test {
  protected:
   // Check that the values in |system_values| correspond to the test data
   // defined at the top of this file.
-  void CheckSystemProfile(const metrics::SystemProfileProto& system_profile) {
+  void CheckSystemProfile(const SystemProfileProto& system_profile) {
     EXPECT_EQ(kInstallDateExpected, system_profile.install_date());
     EXPECT_EQ(kEnabledDateExpected, system_profile.uma_enabled_date());
 
     ASSERT_EQ(arraysize(kFieldTrialIds) + arraysize(kSyntheticTrials),
               static_cast<size_t>(system_profile.field_trial_size()));
     for (size_t i = 0; i < arraysize(kFieldTrialIds); ++i) {
-      const metrics::SystemProfileProto::FieldTrial& field_trial =
+      const SystemProfileProto::FieldTrial& field_trial =
           system_profile.field_trial(i);
       EXPECT_EQ(kFieldTrialIds[i].name, field_trial.name_id());
       EXPECT_EQ(kFieldTrialIds[i].group, field_trial.group_id());
     }
     // Verify the right data is present for the synthetic trials.
     for (size_t i = 0; i < arraysize(kSyntheticTrials); ++i) {
-      const metrics::SystemProfileProto::FieldTrial& field_trial =
+      const SystemProfileProto::FieldTrial& field_trial =
           system_profile.field_trial(i + arraysize(kFieldTrialIds));
       EXPECT_EQ(kSyntheticTrials[i].name, field_trial.name_id());
       EXPECT_EQ(kSyntheticTrials[i].group, field_trial.group_id());
     }
 
-    EXPECT_EQ(metrics::TestMetricsServiceClient::kBrandForTesting,
+    EXPECT_EQ(TestMetricsServiceClient::kBrandForTesting,
               system_profile.brand_code());
 
-    const metrics::SystemProfileProto::Hardware& hardware =
+    const SystemProfileProto::Hardware& hardware =
         system_profile.hardware();
 
     EXPECT_TRUE(hardware.has_cpu());
@@ -378,8 +378,32 @@ TEST_F(MetricsLogTest, OngoingLogStabilityMetrics) {
 TEST_F(MetricsLogTest, ChromeChannelWrittenToProtobuf) {
   TestMetricsServiceClient client;
   TestMetricsLog log(
-      "user@test.com", kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
+      kClientId, kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
   EXPECT_TRUE(log.uma_proto().system_profile().has_channel());
 }
 
+TEST_F(MetricsLogTest, ProductNotSetIfDefault) {
+  TestMetricsServiceClient client;
+  EXPECT_EQ(ChromeUserMetricsExtension::CHROME, client.GetProduct());
+  TestMetricsLog log(
+      kClientId, kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
+  // Check that the product isn't set, since it's default and also verify the
+  // default value is indeed equal to Chrome.
+  EXPECT_FALSE(log.uma_proto().has_product());
+  EXPECT_EQ(ChromeUserMetricsExtension::CHROME, log.uma_proto().product());
+}
+
+TEST_F(MetricsLogTest, ProductSetIfNotDefault) {
+  const int32_t kTestProduct = 100;
+  EXPECT_NE(ChromeUserMetricsExtension::CHROME, kTestProduct);
+
+  TestMetricsServiceClient client;
+  client.set_product(kTestProduct);
+  TestMetricsLog log(
+      kClientId, kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
+  // Check that the product is set to |kTestProduct|.
+  EXPECT_TRUE(log.uma_proto().has_product());
+  EXPECT_EQ(kTestProduct, log.uma_proto().product());
+}
+
 }  // namespace metrics

components/metrics/metrics_service_client.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_METRICS_METRICS_SERVICE_CLIENT_H_
 #define COMPONENTS_METRICS_METRICS_SERVICE_CLIENT_H_
 
+#include <stdint.h>
 #include <string>
 
 #include "base/basictypes.h"
@@ -30,6 +31,11 @@ class MetricsServiceClient {
   // Whether there's an "off the record" (aka "Incognito") session active.
   virtual bool IsOffTheRecordSessionActive() = 0;
 
+  // Returns the product value to use in uploaded reports, which will be used to
+  // set the ChromeUserMetricsExtension.product field. See comments on that
+  // field on why it's an int32 rather than an enum.
+  virtual int32_t GetProduct() = 0;
+
   // Returns the current application locale (e.g. "en-US").
   virtual std::string GetApplicationLocale() = 0;
 

components/metrics/proto/chrome_user_metrics_extension.proto

@@ -25,9 +25,13 @@ message ChromeUserMetricsExtension {
     // Google Chrome product family.
     CHROME = 0;
   }
-  // The product corresponding to this log. Note: The default value is Chrome,
-  // so Chrome products will not transmit this field.
-  optional Product product = 10 [default = CHROME];
+  // The product corresponding to this log. The field type is int32 instead of
+  // Product so that downstream users of the Chromium metrics component can
+  // introduce products without needing to make changes to the Chromium code
+  // (though they still need to add the new product to the server-side enum).
+  // Note: The default value is Chrome, so Chrome products will not transmit
+  // this field.
+  optional int32 product = 10 [default = 0];
 
   // The id of the client install that generated these events.
   //

components/metrics/test_metrics_service_client.cc

@@ -6,14 +6,16 @@
 
 #include "base/callback.h"
 #include "components/metrics/metrics_log_uploader.h"
+#include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
 
 namespace metrics {
 
 // static
 const char TestMetricsServiceClient::kBrandForTesting[] = "brand_for_testing";
 
 TestMetricsServiceClient::TestMetricsServiceClient()
-    : version_string_("5.0.322.0-64-devel") {
+    : version_string_("5.0.322.0-64-devel"),
+      product_(ChromeUserMetricsExtension::CHROME) {
 }
 
 TestMetricsServiceClient::~TestMetricsServiceClient() {
@@ -28,6 +30,10 @@ bool TestMetricsServiceClient::IsOffTheRecordSessionActive() {
   return false;
 }
 
+int32_t TestMetricsServiceClient::GetProduct() {
+  return product_;
+}
+
 std::string TestMetricsServiceClient::GetApplicationLocale() {
   return "en-US";
 }