Merge "more testable MetricsLogger interface"

4 files changed, 177 insertions, 70 deletions

diff --git a/core/java/com/android/internal/logging/MetricsLogger.java b/core/java/com/android/internal/logging/MetricsLogger.java
index 949e7ac50a7b..a48292925830 100644
--- a/core/java/com/android/internal/logging/MetricsLogger.java
+++ b/core/java/com/android/internal/logging/MetricsLogger.java
@@ -31,109 +31,188 @@ public class MetricsLogger {
     // define metric categories in frameworks/base/proto/src/metrics_constants.proto.
     // mirror changes in native version at system/core/libmetricslogger/metrics_logger.cpp
 
+    private static MetricsLogger sMetricsLogger;
+
+    private static MetricsLogger getLogger() {
+        if (sMetricsLogger == null) {
+            sMetricsLogger = new MetricsLogger();
+        }
+        return sMetricsLogger;
+    }
+
+    protected void saveLog(Object[] rep) {
+        EventLogTags.writeSysuiMultiAction(rep);
+    }
+
     public static final int VIEW_UNKNOWN = MetricsEvent.VIEW_UNKNOWN;
     public static final int LOGTAG = EventLogTags.SYSUI_MULTI_ACTION;
 
-    public static void visible(Context context, int category) throws IllegalArgumentException {
+    public void write(LogMaker content) {
+        if (content.getType() == MetricsEvent.TYPE_UNKNOWN) {
+            content.setType(MetricsEvent.TYPE_ACTION);
+        }
+        saveLog(content.serialize());
+    }
+
+    public void visible(int category) throws IllegalArgumentException {
         if (Build.IS_DEBUGGABLE && category == VIEW_UNKNOWN) {
             throw new IllegalArgumentException("Must define metric category");
         }
         EventLogTags.writeSysuiViewVisibility(category, 100);
-        EventLogTags.writeSysuiMultiAction(
-                new LogMaker(category)
+        saveLog(new LogMaker(category)
                         .setType(MetricsEvent.TYPE_OPEN)
                         .serialize());
     }
 
-    public static void hidden(Context context, int category) throws IllegalArgumentException {
+    public void hidden(int category) throws IllegalArgumentException {
         if (Build.IS_DEBUGGABLE && category == VIEW_UNKNOWN) {
             throw new IllegalArgumentException("Must define metric category");
         }
         EventLogTags.writeSysuiViewVisibility(category, 0);
-        EventLogTags.writeSysuiMultiAction(
-                new LogMaker(category)
+        saveLog(new LogMaker(category)
                         .setType(MetricsEvent.TYPE_CLOSE)
                         .serialize());
     }
 
-    public static void visibility(Context context, int category, boolean visibile)
+    public void visibility(int category, boolean visibile)
             throws IllegalArgumentException {
         if (visibile) {
-            visible(context, category);
+            visible(category);
         } else {
-            hidden(context, category);
+            hidden(category);
         }
     }
 
-    public static void visibility(Context context, int category, int vis)
+    public void visibility(int category, int vis)
             throws IllegalArgumentException {
-        visibility(context, category, vis == View.VISIBLE);
+        visibility(category, vis == View.VISIBLE);
     }
 
-    public static void action(Context context, int category) {
+    public void action(int category) {
         EventLogTags.writeSysuiAction(category, "");
-        EventLogTags.writeSysuiMultiAction(
-                new LogMaker(category)
+        saveLog(new LogMaker(category)
                         .setType(MetricsEvent.TYPE_ACTION)
                         .serialize());
     }
 
-    public static void action(Context context, int category, int value) {
+    public void action(int category, int value) {
         EventLogTags.writeSysuiAction(category, Integer.toString(value));
-        EventLogTags.writeSysuiMultiAction(
-                new LogMaker(category)
+        saveLog(new LogMaker(category)
                         .setType(MetricsEvent.TYPE_ACTION)
                         .setSubtype(value)
                         .serialize());
     }
 
-    public static void action(Context context, int category, boolean value) {
+    public void action(int category, boolean value) {
         EventLogTags.writeSysuiAction(category, Boolean.toString(value));
-        EventLogTags.writeSysuiMultiAction(
-                new LogMaker(category)
+        saveLog(new LogMaker(category)
                         .setType(MetricsEvent.TYPE_ACTION)
                         .setSubtype(value ? 1 : 0)
                         .serialize());
     }
 
-    public static void action(LogMaker content) {
-        if (content.getType() == MetricsEvent.TYPE_UNKNOWN) {
-            content.setType(MetricsEvent.TYPE_ACTION);
-        }
-        EventLogTags.writeSysuiMultiAction(content.serialize());
-    }
-
-
-    public static void action(Context context, int category, String pkg) {
+    public void action(int category, String pkg) {
         if (Build.IS_DEBUGGABLE && category == VIEW_UNKNOWN) {
             throw new IllegalArgumentException("Must define metric category");
         }
         EventLogTags.writeSysuiAction(category, pkg);
-        EventLogTags.writeSysuiMultiAction(new LogMaker(category)
+        saveLog(new LogMaker(category)
                 .setType(MetricsEvent.TYPE_ACTION)
                 .setPackageName(pkg)
                 .serialize());
     }
 
     /** Add an integer value to the monotonically increasing counter with the given name. */
-    public static void count(Context context, String name, int value) {
+    public void count(String name, int value) {
         EventLogTags.writeSysuiCount(name, value);
-        EventLogTags.writeSysuiMultiAction(
-                new LogMaker(MetricsEvent.RESERVED_FOR_LOGBUILDER_COUNTER)
+        saveLog(new LogMaker(MetricsEvent.RESERVED_FOR_LOGBUILDER_COUNTER)
                         .setCounterName(name)
                         .setCounterValue(value)
                         .serialize());
     }
 
     /** Increment the bucket with the integer label on the histogram with the given name. */
-    public static void histogram(Context context, String name, int bucket) {
+    public void histogram(String name, int bucket) {
         // see LogHistogram in system/core/libmetricslogger/metrics_logger.cpp
         EventLogTags.writeSysuiHistogram(name, bucket);
-        EventLogTags.writeSysuiMultiAction(
-                new LogMaker(MetricsEvent.RESERVED_FOR_LOGBUILDER_HISTOGRAM)
+        saveLog(new LogMaker(MetricsEvent.RESERVED_FOR_LOGBUILDER_HISTOGRAM)
                         .setCounterName(name)
                         .setCounterBucket(bucket)
                         .setCounterValue(1)
                         .serialize());
     }
+
+    /** @deprecated use {@link #visible(int)} */
+    @Deprecated
+    public static void visible(Context context, int category) throws IllegalArgumentException {
+        getLogger().visible(category);
+    }
+
+    /** @deprecated use {@link #hidden(int)} */
+    @Deprecated
+    public static void hidden(Context context, int category) throws IllegalArgumentException {
+        getLogger().hidden(category);
+    }
+
+    /** @deprecated use {@link #visibility(int, boolean)} */
+    @Deprecated
+    public static void visibility(Context context, int category, boolean visibile)
+            throws IllegalArgumentException {
+        getLogger().visibility(category, visibile);
+    }
+
+    /** @deprecated use {@link #visibility(int, int)} */
+    @Deprecated
+    public static void visibility(Context context, int category, int vis)
+            throws IllegalArgumentException {
+        visibility(context, category, vis == View.VISIBLE);
+    }
+
+    /** @deprecated use {@link #action(int)} */
+    @Deprecated
+    public static void action(Context context, int category) {
+        getLogger().action(category);
+    }
+
+    /** @deprecated use {@link #action(int, int)} */
+    @Deprecated
+    public static void action(Context context, int category, int value) {
+        getLogger().action(category, value);
+    }
+
+    /** @deprecated use {@link #action(int, boolean)} */
+    @Deprecated
+    public static void action(Context context, int category, boolean value) {
+        getLogger().action(category, value);
+    }
+
+    /** @deprecated use {@link #write(LogMaker)} */
+    @Deprecated
+    public static void action(LogMaker content) {
+        getLogger().write(content);
+    }
+
+    /** @deprecated use {@link #action(int, String)} */
+    @Deprecated
+    public static void action(Context context, int category, String pkg) {
+        getLogger().action(category, pkg);
+    }
+
+    /**
+     * Add an integer value to the monotonically increasing counter with the given name.
+     * @deprecated use {@link #count(String, int)}
+     */
+    @Deprecated
+    public static void count(Context context, String name, int value) {
+        getLogger().count(name, value);
+    }
+
+    /**
+     * Increment the bucket with the integer label on the histogram with the given name.
+     * @deprecated use {@link #histogram(String, int)}
+     */
+    @Deprecated
+    public static void histogram(Context context, String name, int bucket) {
+        getLogger().histogram(name, bucket);
+    }
 }
diff --git a/core/java/com/android/internal/logging/testing/FakeMetricsLogger.java b/core/java/com/android/internal/logging/testing/FakeMetricsLogger.java
new file mode 100644
index 000000000000..fbaf87a30b24
--- /dev/null
+++ b/core/java/com/android/internal/logging/testing/FakeMetricsLogger.java
@@ -0,0 +1,30 @@
+package com.android.internal.logging.testing;
+
+import android.metrics.LogMaker;
+
+import com.android.internal.logging.MetricsLogger;
+
+import java.util.LinkedList;
+import java.util.Queue;
+
+/**
+ * Fake logger that queues up logged events for inspection.
+ *
+ * @hide.
+ */
+public class FakeMetricsLogger extends MetricsLogger {
+    private Queue<LogMaker> logs = new LinkedList<>();
+
+    @Override
+    protected void saveLog(Object[] rep) {
+        logs.offer(new LogMaker(rep));
+    }
+
+    public Queue<LogMaker> getLogs() {
+        return logs;
+    }
+
+    public void reset() {
+        logs.clear();
+    }
+}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBar.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBar.java
index 005b701c0d52..6c729dc31c01 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBar.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBar.java
@@ -487,6 +487,8 @@ public class StatusBar extends SystemUI implements DemoMode,
 
     private ScreenPinningRequest mScreenPinningRequest;
 
+    MetricsLogger mMetricsLogger = new MetricsLogger();
+
     // ensure quick settings is disabled until the current user makes it through the setup wizard
     private boolean mUserSetup = false;
     private DeviceProvisionedListener mUserSetupObserver = new DeviceProvisionedListener() {
@@ -1360,7 +1362,7 @@ public class StatusBar extends SystemUI implements DemoMode,
         mDismissView.setOnButtonClickListener(new View.OnClickListener() {
             @Override
             public void onClick(View v) {
-                MetricsLogger.action(mContext, MetricsEvent.ACTION_DISMISS_ALL_NOTES);
+                mMetricsLogger.action(MetricsEvent.ACTION_DISMISS_ALL_NOTES);
                 clearAllNotifications();
             }
         });
@@ -1539,7 +1541,7 @@ public class StatusBar extends SystemUI implements DemoMode,
             } else {
                 EventBus.getDefault().send(new UndockingTaskEvent());
                 if (metricsUndockAction != -1) {
-                    MetricsLogger.action(mContext, metricsUndockAction);
+                    mMetricsLogger.action(metricsUndockAction);
                 }
             }
         }
@@ -1597,7 +1599,7 @@ public class StatusBar extends SystemUI implements DemoMode,
                             notification.getKey());
                     notification.getNotification().fullScreenIntent.send();
                     shadeEntry.notifyFullScreenIntentLaunched();
-                    MetricsLogger.count(mContext, "note_fullscreen", 1);
+                    mMetricsLogger.count("note_fullscreen", 1);
                 } catch (PendingIntent.CanceledException e) {
                 }
             }
@@ -2801,16 +2803,16 @@ public class StatusBar extends SystemUI implements DemoMode,
         if (!mUserSetup) return;
 
         if (KeyEvent.KEYCODE_SYSTEM_NAVIGATION_UP == key) {
-            MetricsLogger.action(mContext, MetricsEvent.ACTION_SYSTEM_NAVIGATION_KEY_UP);
+            mMetricsLogger.action(MetricsEvent.ACTION_SYSTEM_NAVIGATION_KEY_UP);
             mNotificationPanel.collapse(false /* delayed */, 1.0f /* speedUpFactor */);
         } else if (KeyEvent.KEYCODE_SYSTEM_NAVIGATION_DOWN == key) {
-            MetricsLogger.action(mContext, MetricsEvent.ACTION_SYSTEM_NAVIGATION_KEY_DOWN);
+            mMetricsLogger.action(MetricsEvent.ACTION_SYSTEM_NAVIGATION_KEY_DOWN);
             if (mNotificationPanel.isFullyCollapsed()) {
                 mNotificationPanel.expand(true /* animate */);
-                MetricsLogger.count(mContext, NotificationPanelView.COUNTER_PANEL_OPEN, 1);
+                mMetricsLogger.count(NotificationPanelView.COUNTER_PANEL_OPEN, 1);
             } else if (!mNotificationPanel.isInSettings() && !mNotificationPanel.isExpanding()){
                 mNotificationPanel.flingSettings(0 /* velocity */, true /* expand */);
-                MetricsLogger.count(mContext, NotificationPanelView.COUNTER_PANEL_OPEN_QS, 1);
+                mMetricsLogger.count(NotificationPanelView.COUNTER_PANEL_OPEN_QS, 1);
             }
         }
 
@@ -3689,7 +3691,7 @@ public class StatusBar extends SystemUI implements DemoMode,
                 if (pinnedHeadsUp && isPanelFullyCollapsed())  {
                     notificationLoad = 1;
                 } else {
-                    MetricsLogger.histogram(mContext, "note_load", notificationLoad);
+                    mMetricsLogger.histogram("note_load", notificationLoad);
                 }
                 mBarService.onPanelRevealed(clearNotificationEffects, notificationLoad);
             } else {
@@ -3772,7 +3774,7 @@ public class StatusBar extends SystemUI implements DemoMode,
             if (mStatusBarStateLog == null) {
                 mStatusBarStateLog = new LogMaker(MetricsEvent.VIEW_UNKNOWN);
             }
-            MetricsLogger.action(mStatusBarStateLog
+            mMetricsLogger.write(mStatusBarStateLog
                     .setCategory(isBouncerShowing ? MetricsEvent.BOUNCER : MetricsEvent.LOCKSCREEN)
                     .setType(isShowing ? MetricsEvent.TYPE_OPEN : MetricsEvent.TYPE_CLOSE)
                     .setSubtype(isSecure ? 1 : 0));
@@ -5776,7 +5778,7 @@ public class StatusBar extends SystemUI implements DemoMode,
             NotificationInfo info = (NotificationInfo) item.gutsContent;
             final NotificationInfo.OnSettingsClickListener onSettingsClick = (View v,
                     int appUid) -> {
-                MetricsLogger.action(mContext, MetricsEvent.ACTION_NOTE_INFO);
+                mMetricsLogger.action(MetricsEvent.ACTION_NOTE_INFO);
                 guts.resetFalsingCheck();
                 startAppNotificationSettingsActivity(pkg, appUid, channel.getId());
             };
@@ -5848,7 +5850,7 @@ public class StatusBar extends SystemUI implements DemoMode,
                     return false;
                 }
 
-                MetricsLogger.action(mContext, MetricsEvent.ACTION_NOTE_CONTROLS);
+                mMetricsLogger.action(MetricsEvent.ACTION_NOTE_CONTROLS);
 
                 // ensure that it's laid but not visible until actually laid out
                 guts.setVisibility(View.INVISIBLE);
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarTest.java
index 9f56da78c3bd..09f6b554064b 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarTest.java
@@ -23,14 +23,15 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import android.metrics.LogMaker;
-import android.metrics.MetricsReader;
 import android.support.test.filters.FlakyTest;
 import android.support.test.filters.SmallTest;
 import android.support.test.metricshelper.MetricsAsserts;
 import android.support.test.runner.AndroidJUnit4;
 import android.util.DisplayMetrics;
 
+import com.android.internal.logging.MetricsLogger;
 import com.android.internal.logging.nano.MetricsProto.MetricsEvent;
+import com.android.internal.logging.testing.FakeMetricsLogger;
 import com.android.keyguard.KeyguardHostView.OnDismissAction;
 import com.android.systemui.SysuiTestCase;
 import com.android.systemui.statusbar.ActivatableNotificationView;
@@ -43,9 +44,6 @@ import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-// TODO(gpitsch): We have seen some flakes in these tests, needs some investigation.
-// Q: How is mMetricsReader being used by the tested code?
-// A: StatusBar uses MetricsLogger to write to the event log, then read back by MetricsReader
 @SmallTest
 @RunWith(AndroidJUnit4.class)
 public class StatusBarTest extends SysuiTestCase {
@@ -55,8 +53,8 @@ public class StatusBarTest extends SysuiTestCase {
     KeyguardIndicationController mKeyguardIndicationController;
     NotificationStackScrollLayout mStackScroller;
     StatusBar mStatusBar;
+    FakeMetricsLogger mMetricsLogger;
 
-    private MetricsReader mMetricsReader;
     private DisplayMetrics mDisplayMetrics = new DisplayMetrics();
 
     @Before
@@ -65,8 +63,9 @@ public class StatusBarTest extends SysuiTestCase {
         mUnlockMethodCache = mock(UnlockMethodCache.class);
         mKeyguardIndicationController = mock(KeyguardIndicationController.class);
         mStackScroller = mock(NotificationStackScrollLayout.class);
+        mMetricsLogger = new FakeMetricsLogger();
         mStatusBar = new TestableStatusBar(mStatusBarKeyguardViewManager, mUnlockMethodCache,
-                mKeyguardIndicationController, mStackScroller);
+                mKeyguardIndicationController, mStackScroller, mMetricsLogger);
 
         doAnswer(invocation -> {
             OnDismissAction onDismissAction = (OnDismissAction) invocation.getArguments()[0];
@@ -81,15 +80,6 @@ public class StatusBarTest extends SysuiTestCase {
         }).when(mStatusBarKeyguardViewManager).addAfterKeyguardGoneRunnable(any());
 
         when(mStackScroller.getActivatedChild()).thenReturn(null);
-
-        mMetricsReader = new MetricsReader();
-        mMetricsReader.checkpoint(); // clear out old logs
-        try {
-            // pause so that no new events arrive in the rest of this millisecond.
-            Thread.sleep(2);
-        } catch (InterruptedException e) {
-            // pass
-        }
     }
 
     @Test
@@ -127,10 +117,10 @@ public class StatusBarTest extends SysuiTestCase {
         when(mStatusBarKeyguardViewManager.isShowing()).thenReturn(false);
         when(mStatusBarKeyguardViewManager.isBouncerShowing()).thenReturn(false);
         when(mUnlockMethodCache.isMethodSecure()).thenReturn(false);
-
         mStatusBar.onKeyguardViewManagerStatesUpdated();
 
-        MetricsAsserts.assertHasLog("missing hidden insecure lockscreen log", mMetricsReader,
+        MetricsAsserts.assertHasLog("missing hidden insecure lockscreen log",
+                mMetricsLogger.getLogs(),
                 new LogMaker(MetricsEvent.LOCKSCREEN)
                         .setType(MetricsEvent.TYPE_CLOSE)
                         .setSubtype(0));
@@ -150,7 +140,8 @@ public class StatusBarTest extends SysuiTestCase {
 
         mStatusBar.onKeyguardViewManagerStatesUpdated();
 
-        MetricsAsserts.assertHasLog("missing hidden secure lockscreen log", mMetricsReader,
+        MetricsAsserts.assertHasLog("missing hidden secure lockscreen log",
+                mMetricsLogger.getLogs(),
                 new LogMaker(MetricsEvent.LOCKSCREEN)
                         .setType(MetricsEvent.TYPE_CLOSE)
                         .setSubtype(1));
@@ -170,7 +161,8 @@ public class StatusBarTest extends SysuiTestCase {
 
         mStatusBar.onKeyguardViewManagerStatesUpdated();
 
-        MetricsAsserts.assertHasLog("missing insecure lockscreen showing", mMetricsReader,
+        MetricsAsserts.assertHasLog("missing insecure lockscreen showing",
+                mMetricsLogger.getLogs(),
                 new LogMaker(MetricsEvent.LOCKSCREEN)
                         .setType(MetricsEvent.TYPE_OPEN)
                         .setSubtype(0));
@@ -190,7 +182,8 @@ public class StatusBarTest extends SysuiTestCase {
 
         mStatusBar.onKeyguardViewManagerStatesUpdated();
 
-        MetricsAsserts.assertHasLog("missing secure lockscreen showing log", mMetricsReader,
+        MetricsAsserts.assertHasLog("missing secure lockscreen showing log",
+                mMetricsLogger.getLogs(),
                 new LogMaker(MetricsEvent.LOCKSCREEN)
                         .setType(MetricsEvent.TYPE_OPEN)
                         .setSubtype(1));
@@ -210,7 +203,8 @@ public class StatusBarTest extends SysuiTestCase {
 
         mStatusBar.onKeyguardViewManagerStatesUpdated();
 
-        MetricsAsserts.assertHasLog("missing bouncer log", mMetricsReader,
+        MetricsAsserts.assertHasLog("missing bouncer log",
+                mMetricsLogger.getLogs(),
                 new LogMaker(MetricsEvent.BOUNCER)
                         .setType(MetricsEvent.TYPE_OPEN)
                         .setSubtype(1));
@@ -223,7 +217,8 @@ public class StatusBarTest extends SysuiTestCase {
         ActivatableNotificationView view =  mock(ActivatableNotificationView.class);
         mStatusBar.onActivated(view);
 
-        MetricsAsserts.assertHasLog("missing lockscreen note tap log", mMetricsReader,
+        MetricsAsserts.assertHasLog("missing lockscreen note tap log",
+                mMetricsLogger.getLogs(),
                 new LogMaker(MetricsEvent.ACTION_LS_NOTE)
                         .setType(MetricsEvent.TYPE_ACTION));
     }
@@ -231,11 +226,12 @@ public class StatusBarTest extends SysuiTestCase {
     static class TestableStatusBar extends StatusBar {
         public TestableStatusBar(StatusBarKeyguardViewManager man,
                 UnlockMethodCache unlock, KeyguardIndicationController key,
-                NotificationStackScrollLayout stack) {
+                NotificationStackScrollLayout stack, MetricsLogger logger) {
             mStatusBarKeyguardViewManager = man;
             mUnlockMethodCache = unlock;
             mKeyguardIndicationController = key;
             mStackScroller = stack;
+            mMetricsLogger = logger;
         }
 
         @Override