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

components/metrics/metrics_log.cc

Index: components/metrics/metrics_log.cc
diff --git a/components/metrics/metrics_log.cc b/components/metrics/metrics_log.cc
index 3d8cc1886a891d566bc3e42237c97ff41b385618..10bfcaef5bcc6aa03defabadd2b92834ed12edc6 100644
--- a/components/metrics/metrics_log.cc
+++ b/components/metrics/metrics_log.cc
@@ -207,32 +207,42 @@ void MetricsLog::RecordStabilityMetrics(
   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);
 }
 
@@ -285,9 +295,11 @@ bool MetricsLog::HasStabilityMetrics() const {
 // 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();
@@ -379,15 +391,15 @@ void MetricsLog::RecordEnvironment(
   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));
   }
 }
 
@@ -404,10 +416,11 @@ bool MetricsLog::LoadSavedEnvironmentFromPrefs() {
   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() {