Avoid some pref writes in MetricsLog. Ensure some stability metrics are cleared.

components/metrics/metrics_log.cc

 // Copyright 2014 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 "components/metrics/metrics_log.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 186 matching lines @@
   WriteRealtimeStabilityAttributes(pref, incremental_uptime, uptime);
 
   SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
   for (size_t i = 0; i < metrics_providers.size(); ++i) {
     if (log_type() == INITIAL_STABILITY_LOG)
       metrics_providers[i]->ProvideInitialStabilityMetrics(system_profile);
     metrics_providers[i]->ProvideStabilityMetrics(system_profile);
   }
 
   // Omit some stats unless this is the initial stability log.
   if (log_type() != INITIAL_STABILITY_LOG)

[manzagop (departed), 2016/08/31 16:20:47]

What is the reason for skipping these stats? Is it storage impact? 

Not skipping them would shorten the reporting delay in the case of an early
crash (because sometimes there is no initial stability report (when there is no
system profile which can happen with an early crash)).

[Alexei Svitkine (slow), 2016/08/31 16:40:00]

I think the logic was that it was not necessary to log them outside of the
initial stability log context. As you point out, that reasoning doesn't make
sense in the case when there's no previous system profile saved. (I don't think
it was considering that case.)

[manzagop (departed), 2016/08/31 19:26:52]

Ok, removing. Won't cause a storage size increase because also addressing the
TODO to not set 0s in the proto.

     return;
 
+  SystemProfileProto::Stability* stability =
+      system_profile->mutable_stability();
+
+  // TODO(jar): The following are all optional in SystemProfileProto::Stability,

[manzagop (departed), 2016/08/31 16:20:47]

This todo is still valid because it's about not setting the value in the proto.
I'm guessing it requires some server side changes to ensure a missing value is
interpreted as a 0. 

I'm guessing the motivation for doing this is for server side disk use. Not
setting each value would save ~3bytes (tag+value), for a total of 15 bytes per
initial stability report.

[Alexei Svitkine (slow), 2016/08/31 16:40:00]

No server-side change is needed, so the TODO can be removed if you don't set the
fields when the values are 0. Values default to 0 already if not present (unless
there's special custom logic to do something else, of course).

Yeah, this is mainly about saving the bandwidth of transmitting 0's.

[manzagop (departed), 2016/08/31 19:26:51]

Great! Done.

+  // so we *could* optimize them for values of zero (and not include them).
+
   int incomplete_shutdown_count =
       pref->GetInteger(prefs::kStabilityIncompleteSessionEndCount);
-  pref->SetInteger(prefs::kStabilityIncompleteSessionEndCount, 0);
+  if (incomplete_shutdown_count)
+    pref->SetInteger(prefs::kStabilityIncompleteSessionEndCount, 0);
+  stability->set_incomplete_shutdown_count(incomplete_shutdown_count);
+
   int breakpad_registration_success_count =

[manzagop (departed), 2016/08/31 16:20:47]

These last 4 metrics are never cleared. Should they be cleared here?
https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?l=521

[Alexei Svitkine (slow), 2016/08/31 16:40:00]

Yes, I think so.

Although, I wonder if anyone ever uses these metrics. Maybe we should just
remove them entirely.

[manzagop (departed), 2016/08/31 19:26:52]

Not feeling comfortable making that call / do the cleanup, so I'll stick with
just clearing the values for now if that's ok with you.

       pref->GetInteger(prefs::kStabilityBreakpadRegistrationSuccess);
-  pref->SetInteger(prefs::kStabilityBreakpadRegistrationSuccess, 0);
+  if (breakpad_registration_success_count)
+    pref->SetInteger(prefs::kStabilityBreakpadRegistrationSuccess, 0);
+  stability->set_breakpad_registration_success_count(
+      breakpad_registration_success_count);
+
   int breakpad_registration_failure_count =
       pref->GetInteger(prefs::kStabilityBreakpadRegistrationFail);
-  pref->SetInteger(prefs::kStabilityBreakpadRegistrationFail, 0);
+  if (breakpad_registration_failure_count)
+    pref->SetInteger(prefs::kStabilityBreakpadRegistrationFail, 0);
+  stability->set_breakpad_registration_failure_count(
+      breakpad_registration_failure_count);
+
   int debugger_present_count =
       pref->GetInteger(prefs::kStabilityDebuggerPresent);
-  pref->SetInteger(prefs::kStabilityDebuggerPresent, 0);
+  if (debugger_present_count)
+    pref->SetInteger(prefs::kStabilityDebuggerPresent, 0);
+  stability->set_debugger_present_count(debugger_present_count);
+
   int debugger_not_present_count =
       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).
-  SystemProfileProto::Stability* stability =
-      system_profile->mutable_stability();
-  stability->set_incomplete_shutdown_count(incomplete_shutdown_count);
-  stability->set_breakpad_registration_success_count(
-      breakpad_registration_success_count);
-  stability->set_breakpad_registration_failure_count(
-      breakpad_registration_failure_count);
-  stability->set_debugger_present_count(debugger_present_count);
+  if (debugger_not_present_count)
+    pref->SetInteger(prefs::kStabilityDebuggerNotPresent, 0);
   stability->set_debugger_not_present_count(debugger_not_present_count);
 }
 
 void MetricsLog::RecordGeneralMetrics(
     const std::vector<MetricsProvider*>& metrics_providers) {
   for (size_t i = 0; i < metrics_providers.size(); ++i)
     metrics_providers[i]->ProvideGeneralMetrics(uma_proto());
 }
 
 void MetricsLog::GetFieldTrialIds(
@@ skipping 32 matching lines @@
 bool MetricsLog::HasStabilityMetrics() const {
   return uma_proto()->system_profile().stability().has_launch_count();
 }
 
 // The server refuses data that doesn't have certain values.  crashcount and
 // launchcount are currently "required" in the "stability" group.
 // TODO(isherman): Stop writing these attributes specially once the migration to
 // protobufs is complete.
 void MetricsLog::WriteRequiredStabilityAttributes(PrefService* pref) {
   int launch_count = pref->GetInteger(prefs::kStabilityLaunchCount);
-  pref->SetInteger(prefs::kStabilityLaunchCount, 0);
+  if (launch_count)
+    pref->SetInteger(prefs::kStabilityLaunchCount, 0);
   int crash_count = pref->GetInteger(prefs::kStabilityCrashCount);
-  pref->SetInteger(prefs::kStabilityCrashCount, 0);
+  if (crash_count)
+    pref->SetInteger(prefs::kStabilityCrashCount, 0);
 
   SystemProfileProto::Stability* stability =
       uma_proto()->mutable_system_profile()->mutable_stability();
   stability->set_launch_count(launch_count);
   stability->set_crash_count(crash_count);
 }
 
 void MetricsLog::WriteRealtimeStabilityAttributes(
     PrefService* pref,
     base::TimeDelta incremental_uptime,
@@ skipping 71 matching lines @@
   cpu->set_num_cores(base::SysInfo::NumberOfProcessors());
 
   std::vector<ActiveGroupId> field_trial_ids;
   GetFieldTrialIds(&field_trial_ids);
   WriteFieldTrials(field_trial_ids, system_profile);
   WriteFieldTrials(synthetic_trials, system_profile);
 
   for (size_t i = 0; i < metrics_providers.size(); ++i)
     metrics_providers[i]->ProvideSystemProfileMetrics(system_profile);
 
-  std::string serialied_system_profile;
+  std::string serialized_system_profile;
   std::string base64_system_profile;
-  if (system_profile->SerializeToString(&serialied_system_profile)) {
-    base::Base64Encode(serialied_system_profile, &base64_system_profile);
+  if (system_profile->SerializeToString(&serialized_system_profile)) {
+    base::Base64Encode(serialized_system_profile, &base64_system_profile);
     PrefService* local_state = local_state_;
     local_state->SetString(prefs::kStabilitySavedSystemProfile,
                            base64_system_profile);
     local_state->SetString(prefs::kStabilitySavedSystemProfileHash,
-                           ComputeSHA1(serialied_system_profile));
+                           ComputeSHA1(serialized_system_profile));
   }
 }
 
 bool MetricsLog::LoadSavedEnvironmentFromPrefs() {
   PrefService* local_state = local_state_;
   const std::string base64_system_profile =
       local_state->GetString(prefs::kStabilitySavedSystemProfile);
   if (base64_system_profile.empty())
     return false;
 
   const std::string system_profile_hash =
       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;
-  return base::Base64Decode(base64_system_profile, &serialied_system_profile) &&
-         ComputeSHA1(serialied_system_profile) == system_profile_hash &&
-         system_profile->ParseFromString(serialied_system_profile);
+  std::string serialized_system_profile;
+  return base::Base64Decode(base64_system_profile,
+                            &serialized_system_profile) &&
+         ComputeSHA1(serialized_system_profile) == system_profile_hash &&
+         system_profile->ParseFromString(serialized_system_profile);
 }
 
 void MetricsLog::CloseLog() {
   DCHECK(!closed_);
   closed_ = true;
 }
 
 void MetricsLog::GetEncodedLog(std::string* encoded_log) {
   DCHECK(closed_);
   uma_proto_.SerializeToString(encoded_log);
 }
 
 }  // namespace metrics