metrics: Move CachingUmaRecorder singleton out of RecordHistogram

In the future it will be used for non-histogram data, namely backing
RecordUserActions.

Bug: 658300
Change-Id: I0a872878466f8cf62847ed10da2011fcdbfb07b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2079841
Auto-Submit: bttk <bttk@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Commit-Queue: bttk <bttk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749424}

base/BUILD.gn

@@ -3360,6 +3360,7 @@ if (is_android) {
       "android/java/src/org/chromium/base/metrics/ScopedSysTraceEvent.java",
       "android/java/src/org/chromium/base/metrics/StatisticsRecorderAndroid.java",
       "android/java/src/org/chromium/base/metrics/UmaRecorder.java",
+      "android/java/src/org/chromium/base/metrics/UmaRecorderHolder.java",
       "android/java/src/org/chromium/base/multidex/ChromiumMultiDexInstaller.java",
       "android/java/src/org/chromium/base/process_launcher/BindService.java",
       "android/java/src/org/chromium/base/process_launcher/ChildConnectionAllocator.java",

base/android/java/src/org/chromium/base/AnimationFrameTimeHistogram.java

@@ -12,7 +12,7 @@ import android.animation.TimeAnimator.TimeListener;
 import android.util.Log;
 
 import org.chromium.base.annotations.NativeMethods;
-import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.base.metrics.UmaRecorderHolder;
 
 /**
  * Record Android animation frame rate and save it to UMA histogram. This is mainly for monitoring
@@ -73,7 +73,7 @@ public class AnimationFrameTimeHistogram {
      * successful.
      */
     public void endRecording() {
-        if (mRecorder.endRecording() && RecordHistogram.sDisabledBy == null) {
+        if (mRecorder.endRecording() && UmaRecorderHolder.sDisabledBy == null) {
             AnimationFrameTimeHistogramJni.get().saveHistogram(
                     mHistogramName, mRecorder.getFrameTimesMs(), mRecorder.getFrameTimesCount());
         }

base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java

@@ -38,6 +38,7 @@ import org.chromium.base.annotations.RemovableInRelease;
 import org.chromium.base.compat.ApiHelperForM;
 import org.chromium.base.metrics.CachedMetrics;
 import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.base.metrics.UmaRecorderHolder;
 
 import java.io.File;
 import java.io.IOException;
@@ -661,7 +662,7 @@ public class LibraryLoader {
             Log.i(TAG, "Loaded native library version number \"%s\"",
                     NativeLibraries.sVersionNumber);
         }
-        RecordHistogram.onLibraryLoaded();
+        UmaRecorderHolder.onLibraryLoaded();
 
         // From now on, keep tracing in sync with native.
         TraceEvent.registerNativeEnabledObserver();

base/android/java/src/org/chromium/base/metrics/RecordHistogram.java

@@ -6,76 +6,24 @@ package org.chromium.base.metrics;
 
 import android.text.format.DateUtils;
 
-import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.MainDex;
 import org.chromium.base.annotations.NativeMethods;
 
-import javax.annotation.concurrent.GuardedBy;
-
 /** Java API for recording UMA histograms. */
 @JNINamespace("base::android")
 @MainDex
 public class RecordHistogram {
-    /** Underlying {@link UmaRecorder} used by methods in this class. */
-    private static final CachingUmaRecorder sRecorder = new CachingUmaRecorder();
-
-    /**
-     * {@code null}, unless recording of histograms is currently disabled for testing. Exposed for
-     * use in peer classes {e.g. AnimationFrameTimeHistogram}.
-     * <p>
-     * Use {@link #setDisabledForTests(boolean)} to set this value.
-     * <p>
-     * TODO(bttk@chromium.org): Rename to sDisabledForTestBy
-     */
-    @VisibleForTesting
-    @Nullable
-    public static Throwable sDisabledBy;
-
     /**
-     * The delegate disabled by {@link #setDisabledForTests(boolean)}.
-     */
-    @GuardedBy("sRecorder")
-    @Nullable
-    private static UmaRecorder sDisabledDelegateForTest;
-
-    /**
-     * Tests may not have native initialized, so they may need to disable metrics. The value should
-     * be reset after the test done, to avoid carrying over state to unrelated tests.
-     * <p>
-     * In JUnit tests this can be done automatically using
-     * {@link org.chromium.base.metrics.test.DisableHistogramsRule}.
+     * Tests may need to disable metrics. The value should be reset after the test done, to avoid
+     * carrying over state to unrelated tests. <p> In JUnit tests this can be done automatically
+     * using {@link org.chromium.base.metrics.test.DisableHistogramsRule}.
      */
     @VisibleForTesting
     public static void setDisabledForTests(boolean disabled) {
-        synchronized (sRecorder) {
-            if (disabled) {
-                if (sDisabledBy != null) {
-                    throw new IllegalStateException(
-                            "Histograms are already disabled.", sDisabledBy);
-                }
-                sDisabledBy = new Throwable();
-                sDisabledDelegateForTest = sRecorder.setDelegate(new NoopUmaRecorder());
-            } else {
-                sDisabledBy = null;
-                sRecorder.setDelegate(sDisabledDelegateForTest);
-                sDisabledDelegateForTest = null;
-            }
-        }
-    }
-
-    /** Starts forwarding metrics to the native code. Returns after the cache has been flushed. */
-    public static void onLibraryLoaded() {
-        synchronized (sRecorder) {
-            if (sDisabledBy == null) {
-                sRecorder.setDelegate(new NativeUmaRecorder());
-            } else {
-                // If metrics are disabled for test, use native when metrics get restored.
-                sDisabledDelegateForTest = new NativeUmaRecorder();
-            }
-        }
+        UmaRecorderHolder.setDisabledForTests(disabled);
     }
 
     /**
@@ -87,7 +35,7 @@ public class RecordHistogram {
      * @param sample sample to be recorded, either true or false
      */
     public static void recordBooleanHistogram(String name, boolean sample) {
-        sRecorder.recordBooleanHistogram(name, sample);
+        UmaRecorderHolder.get().recordBooleanHistogram(name, sample);
     }
 
     /**
@@ -112,7 +60,7 @@ public class RecordHistogram {
      * @param sample sample to be recorded, at least 1 and at most 999999
      */
     public static void recordCountHistogram(String name, int sample) {
-        sRecorder.recordExponentialHistogram(name, sample, 1, 1_000_000, 50);
+        UmaRecorderHolder.get().recordExponentialHistogram(name, sample, 1, 1_000_000, 50);
     }
 
     /**
@@ -123,7 +71,7 @@ public class RecordHistogram {
      * @param sample sample to be recorded, at least 1 and at most 99
      */
     public static void recordCount100Histogram(String name, int sample) {
-        sRecorder.recordExponentialHistogram(name, sample, 1, 100, 50);
+        UmaRecorderHolder.get().recordExponentialHistogram(name, sample, 1, 100, 50);
     }
 
     /**
@@ -134,7 +82,7 @@ public class RecordHistogram {
      * @param sample sample to be recorded, at least 1 and at most 999
      */
     public static void recordCount1000Histogram(String name, int sample) {
-        sRecorder.recordExponentialHistogram(name, sample, 1, 1_000, 50);
+        UmaRecorderHolder.get().recordExponentialHistogram(name, sample, 1, 1_000, 50);
     }
 
     /**
@@ -145,7 +93,7 @@ public class RecordHistogram {
      * @param sample sample to be recorded, at least 1 and at most 99999
      */
     public static void recordCount100000Histogram(String name, int sample) {
-        sRecorder.recordExponentialHistogram(name, sample, 1, 100_000, 50);
+        UmaRecorderHolder.get().recordExponentialHistogram(name, sample, 1, 100_000, 50);
     }
 
     /**
@@ -161,7 +109,7 @@ public class RecordHistogram {
      */
     public static void recordCustomCountHistogram(
             String name, int sample, int min, int max, int numBuckets) {
-        sRecorder.recordExponentialHistogram(name, sample, min, max, numBuckets);
+        UmaRecorderHolder.get().recordExponentialHistogram(name, sample, min, max, numBuckets);
     }
 
     /**
@@ -177,7 +125,7 @@ public class RecordHistogram {
      */
     public static void recordLinearCountHistogram(
             String name, int sample, int min, int max, int numBuckets) {
-        sRecorder.recordLinearHistogram(name, sample, min, max, numBuckets);
+        UmaRecorderHolder.get().recordLinearHistogram(name, sample, min, max, numBuckets);
     }
 
     /**
@@ -200,7 +148,7 @@ public class RecordHistogram {
      *               {@code <= 100} ideally, definitely {@code <= 1000}.
      */
     public static void recordSparseHistogram(String name, int sample) {
-        sRecorder.recordSparseHistogram(name, sample);
+        UmaRecorderHolder.get().recordSparseHistogram(name, sample);
     }
 
     /**
@@ -286,7 +234,7 @@ public class RecordHistogram {
      * @param sizeInkB Sample to record in KB
      */
     public static void recordMemoryKBHistogram(String name, int sizeInKB) {
-        sRecorder.recordExponentialHistogram(name, sizeInKB, 1000, 500000, 50);
+        UmaRecorderHolder.get().recordExponentialHistogram(name, sizeInKB, 1000, 500000, 50);
     }
 
     /**
@@ -302,7 +250,7 @@ public class RecordHistogram {
         final int min = 1;
         // One extra is added for the overflow bucket.
         final int numBuckets = max + 1;
-        sRecorder.recordLinearHistogram(name, sample, min, max, numBuckets);
+        UmaRecorderHolder.get().recordLinearHistogram(name, sample, min, max, numBuckets);
     }
 
     private static int clampToInt(long value) {
@@ -315,7 +263,7 @@ public class RecordHistogram {
 
     private static void recordCustomTimesHistogramMilliseconds(
             String name, long duration, long min, long max, int numBuckets) {
-        sRecorder.recordExponentialHistogram(
+        UmaRecorderHolder.get().recordExponentialHistogram(
                 name, clampToInt(duration), clampToInt(min), clampToInt(max), numBuckets);
     }
 

base/android/java/src/org/chromium/base/metrics/UmaRecorderHolder.java

+// Copyright 2020 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.
+
+package org.chromium.base.metrics;
+
+import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
+
+import javax.annotation.concurrent.GuardedBy;
+
+/** Holds the {@link CachingUmaRecorder} used by {@link RecordHistogram}. */
+public class UmaRecorderHolder {
+    /** The instance held by this class. */
+    private static CachingUmaRecorder sRecorder = new CachingUmaRecorder();
+
+    /**
+     * {@code null}, unless recording is currently disabled for testing. Exposed for use in peer
+     * classes {e.g. AnimationFrameTimeHistogram}.
+     * <p>
+     * Use {@link #setDisabledForTests(boolean)} to set this value.
+     * <p>
+     * TODO(bttk@chromium.org): Fix dependency in AnimationFrameTimeHistogram, make this field
+     *     private and rename to sDisabledForTestBy
+     */
+    @VisibleForTesting
+    @Nullable
+    public static Throwable sDisabledBy;
+
+    /** Lock for {@code sDisabledDelegateForTest}. */
+    private static final Object sLock = new Object();
+
+    /** The delegate disabled by {@link #setDisabledForTests(boolean)}. */
+    @GuardedBy("sLock")
+    @Nullable
+    private static UmaRecorder sDisabledDelegateForTest;
+
+    /** Returns the {@link CachingUmaRecorder}. */
+    /* package */ static CachingUmaRecorder get() {
+        return sRecorder;
+    }
+
+    /** Starts forwarding metrics to the native code. Returns after the cache has been flushed. */
+    public static void onLibraryLoaded() {
+        synchronized (sLock) {
+            if (sDisabledBy == null) {
+                sRecorder.setDelegate(new NativeUmaRecorder());
+            } else {
+                // If metrics are disabled for test, use native when metrics get restored.
+                sDisabledDelegateForTest = new NativeUmaRecorder();
+            }
+        }
+    }
+
+    /**
+     * Tests may need to disable metrics. The value should be reset after the test done, to avoid
+     * carrying over state to unrelated tests. <p> In JUnit tests this can be done automatically
+     * using {@link org.chromium.base.metrics.test.DisableHistogramsRule}.
+     */
+    @VisibleForTesting
+    public static void setDisabledForTests(boolean disabled) {
+        synchronized (sLock) {
+            if (disabled) {
+                if (sDisabledBy != null) {
+                    throw new IllegalStateException(
+                            "Histograms are already disabled.", sDisabledBy);
+                }
+                sDisabledBy = new Throwable();
+                sDisabledDelegateForTest = sRecorder.setDelegate(new NoopUmaRecorder());
+            } else {
+                sDisabledBy = null;
+                sRecorder.setDelegate(sDisabledDelegateForTest);
+                sDisabledDelegateForTest = null;
+            }
+        }
+    }
+}

base/android/junit/src/org/chromium/base/metrics/test/DisableHistogramsRule.java

@@ -6,7 +6,7 @@ package org.chromium.base.metrics.test;
 
 import org.junit.rules.ExternalResource;
 
-import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.base.metrics.UmaRecorderHolder;
 
 /**
  * Disables histogram recording for the duration of the tests.
@@ -14,11 +14,11 @@ import org.chromium.base.metrics.RecordHistogram;
 public class DisableHistogramsRule extends ExternalResource {
     @Override
     protected void before() {
-        RecordHistogram.setDisabledForTests(true);
+        UmaRecorderHolder.setDisabledForTests(true);
     }
 
     @Override
     protected void after() {
-        RecordHistogram.setDisabledForTests(false);
+        UmaRecorderHolder.setDisabledForTests(false);
     }
 }

base/test/android/javatests/src/org/chromium/base/test/DisableNativeTestRule.java

@@ -9,8 +9,8 @@ import org.junit.runner.Description;
 import org.junit.runners.model.Statement;
 
 import org.chromium.base.Log;
-import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
+import org.chromium.base.metrics.UmaRecorderHolder;
 
 /**
  * {@link TestRule} to disable native access for testing.
@@ -26,7 +26,7 @@ public final class DisableNativeTestRule implements TestRule {
                 LoadNative loadNative = description.getAnnotation(LoadNative.class);
                 if (loadNative == null) {
                     Log.i(TAG, "Disable RecordHistogram and RecordUserAction for testing");
-                    RecordHistogram.setDisabledForTests(true);
+                    UmaRecorderHolder.setDisabledForTests(true);
                     RecordUserAction.setDisabledForTests(true);
                 } else {
                     Log.i(TAG, "Test will run with native libraries");
@@ -36,7 +36,7 @@ public final class DisableNativeTestRule implements TestRule {
                 } finally {
                     if (loadNative == null) {
                         Log.i(TAG, "Re-enable RecordHistogram and RecordUserAction after test");
-                        RecordHistogram.setDisabledForTests(false);
+                        UmaRecorderHolder.setDisabledForTests(false);
                         RecordUserAction.setDisabledForTests(false);
                     }
                 }