Enabling sampling of UMA and crash reports on Android.

chrome/browser/android/metrics/uma_session_stats.cc

 // Copyright 2015 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/android/metrics/uma_session_stats.h"
 
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
@@ skipping 86 matching lines @@
 // There are three possible states:
 // * Logs are being recorded and being uploaded to the server.
 // * Logs are being recorded, but not being uploaded to the server.
 //   This happens when we've got permission to upload on Wi-Fi but we're on a
 //   mobile connection (for example).
 // * Logs are neither being recorded or uploaded.
 static void UpdateMetricsServiceState(JNIEnv* env,
                                       const JavaParamRef<jobject>& obj,
                                       jboolean may_record,
                                       jboolean may_upload) {
-  metrics::MetricsService* metrics = g_browser_process->metrics_service();
-  DCHECK(metrics);
+  // Used to detect when consent to record changes. Initiallized to false to
+  // treat the startup path as a change when consent has been given.

[Alexei Svitkine (slow), 2016/08/18 06:16:39]

Hmm, I am bit confused by this.

In my CL that added UpdateMetricsPrefsOnPermissionChange(), I was assuming this
would only be called on a user initiated change of the pref. But this comment
implies this actually changes in a startup case.

If so, I think the UpdateMetricsPrefsOnPermissionChange() is not correct. As it
assumes the pref is changing (and for example, when enabling, clears "old" crash
counts).

If that is not the case, then (my) logic is wrong. :(

How can we detect that the pref is actually changing here. Should we have a
separate param from the Java side?

(Also, separately, it's not obvious to me how why you're changing the logic here
- and why the change to making it static makes a difference to the old logic.
Can you explain?)

[jwd, 2016/08/18 15:47:56]

Shoot, yeah, should have caught that. But I do believe this is on the startup
path.
Gayane, can maybe confirm. 
So, the problem is that the UpdateMetricsServiceState function is called with no
arguments from the java side (on connection change, on startNewSession,
onResume, and NOT when the setting is changed), and is strictly independent of
the function that syncs android and chrome prefs. 

The only way we currently have of knowing if the update is the first since
consent change is maybe from the GoogleUpdateSettings::GetCollectStatsConsent,
which does file ops. I could have some initialization that checks that once a
session. It'll be annoying since it'll need to be a posted task, but it should
work. I think it only means an extra file op on first run and whenever the pref
changes, since I think the old code was doing extra file ops anyway.

idea (1) is do that

idea (2):
Create an "is dirty" android pref, whenever we change the consent pref, we mark
the is dirty pref as true, whenever we call UpdateMetricsServiceState we pass in
the is dirty value no the native version, and set it to false after.

idea (3):
Only synchronize the the android and chrome prefs in the
UpdateMtricsServiceState call, that way we know when there's a change, since
they disagree.

idea (4):
Rework a lot of the Java code to be more similar to the native code, i.e.
Something similar to ChangeMetricsReportingState in metrics_reporting_state.
That would be called from the settings change and the first run flow (or
something would be from there, since SetCollectStatsConsent needs to be called
before ForceClientIdCreation, otherwise the consent file won't get written...)
It's different from the old logic in the case of sampling. may_record can be
true, while metrics aren't recording. With the old logic, it would always look
like it's switching on. Not VERY wrong, since the client isn't recording and we
probably don't care about the "old" crash counts, but it would do extra file
ops.

[Alexei Svitkine (slow), 2016/08/18 16:00:30]

I think we should try to get to (4) eventually - making Java and native code
more similar.

In the mean time, how hard would it be to simply add a new param to the Java &
JNI API that specifies whether the pref is actually changing as a result of a
user toggling the pref? Then, only do the metrics prefs stuff when that is true.

[jwd, 2016/08/18 21:00:47]

Currently, non-trivial, since the Update call is done independently of pref
changes. Either we'd need to call it when we make a pref change and pass
is_change=true there, and pass false at all the other call sites; or we'd need
to do (2) so that the Java side Update method can know.

[jwd, 2016/08/23 18:40:03]

Added the is_change parameter, and made it so that this gets called when the
prefs are changed.

+  static bool last_may_record = false;
 
-  if (metrics->recording_active() != may_record) {
+  if (last_may_record != may_record) {
+    last_may_record = may_record;
+
     UpdateMetricsPrefsOnPermissionChange(may_record);
 
-    // This function puts a consent file with the ClientID in the
-    // data directory. The ID is passed to the renderer for crash
-    // reporting when things go wrong.
+    // This function ensures a consent file in the data directory is either
+    // created, or deleted, depending on may_record. Starting up metrics
+    // services will ensure that the consent file contains the ClientID. The ID
+    // is passed to the renderer for crash reporting when things go wrong.
     content::BrowserThread::GetBlockingPool()->PostTask(FROM_HERE,
         base::Bind(
             base::IgnoreResult(GoogleUpdateSettings::SetCollectStatsConsent),
             may_record));
   }
 
-  g_browser_process->GetMetricsServicesManager()->UpdatePermissions(
-      may_record, may_upload);
+  // This will also apply the consent state, taken from Chrome Local State
+  // prefs.
+  g_browser_process->GetMetricsServicesManager()->UpdateUploadPermissions(
+      may_upload);
 }
 
 // Renderer process crashed in the foreground.
 static void LogRendererCrash(JNIEnv*, const JavaParamRef<jclass>&) {
   DCHECK(g_browser_process);
   // Increment the renderer crash count in stability metrics.
   PrefService* pref = g_browser_process->local_state();
   DCHECK(pref);
   int value = pref->GetInteger(metrics::prefs::kStabilityRendererCrashCount);
   pref->SetInteger(metrics::prefs::kStabilityRendererCrashCount, value + 1);
@@ skipping 83 matching lines @@
   // We should have only one UmaSessionStats instance.
   DCHECK(!g_uma_session_stats);
   g_uma_session_stats = new UmaSessionStats();
   return reinterpret_cast<intptr_t>(g_uma_session_stats);
 }
 
 // Register native methods
 bool RegisterUmaSessionStats(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }