Merge "Provide more feedback to user on protolog to logcat command" into main

5 files changed, 39 insertions, 20 deletions

diff --git a/core/java/com/android/internal/protolog/ProtoLogCommandHandler.java b/core/java/com/android/internal/protolog/ProtoLogCommandHandler.java
index 82d8d3431a9d..6d4a40899a65 100644
--- a/core/java/com/android/internal/protolog/ProtoLogCommandHandler.java
+++ b/core/java/com/android/internal/protolog/ProtoLogCommandHandler.java
@@ -145,11 +145,11 @@ public class ProtoLogCommandHandler extends ShellCommand {
 
         switch (cmd) {
             case "enable" -> {
-                mProtoLogConfigurationService.enableProtoLogToLogcat(processGroups());
+                mProtoLogConfigurationService.enableProtoLogToLogcat(pw, processGroups());
                 return 0;
             }
             case "disable" -> {
-                mProtoLogConfigurationService.disableProtoLogToLogcat(processGroups());
+                mProtoLogConfigurationService.disableProtoLogToLogcat(pw, processGroups());
                 return 0;
             }
             default -> {
diff --git a/core/java/com/android/internal/protolog/ProtoLogConfigurationService.java b/core/java/com/android/internal/protolog/ProtoLogConfigurationService.java
index d65aaae7deaa..a19690bbd0e4 100644
--- a/core/java/com/android/internal/protolog/ProtoLogConfigurationService.java
+++ b/core/java/com/android/internal/protolog/ProtoLogConfigurationService.java
@@ -18,6 +18,8 @@ package com.android.internal.protolog;
 
 import android.annotation.NonNull;
 
+import java.io.PrintWriter;
+
 public interface ProtoLogConfigurationService extends IProtoLogConfigurationService {
     /**
      * Get the list of groups clients have registered to the protolog service.
@@ -37,11 +39,11 @@ public interface ProtoLogConfigurationService extends IProtoLogConfigurationServ
      * Enable logging target groups to logcat.
      * @param groups we want to enable logging them to logcat for.
      */
-    void enableProtoLogToLogcat(@NonNull String... groups);
+    void enableProtoLogToLogcat(@NonNull PrintWriter pw, @NonNull String... groups);
 
     /**
      * Disable logging target groups to logcat.
      * @param groups we want to disable from being logged to logcat.
      */
-    void disableProtoLogToLogcat(@NonNull String... groups);
+    void disableProtoLogToLogcat(@NonNull PrintWriter pw, @NonNull String... groups);
 }
diff --git a/core/java/com/android/internal/protolog/ProtoLogConfigurationServiceImpl.java b/core/java/com/android/internal/protolog/ProtoLogConfigurationServiceImpl.java
index f83359dddfcc..ac1022ff1e0f 100644
--- a/core/java/com/android/internal/protolog/ProtoLogConfigurationServiceImpl.java
+++ b/core/java/com/android/internal/protolog/ProtoLogConfigurationServiceImpl.java
@@ -44,6 +44,7 @@ import java.io.FileDescriptor;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.io.PrintWriter;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
@@ -183,8 +184,8 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ
      * @param groups we want to enable logging them to logcat for.
      */
     @Override
-    public void enableProtoLogToLogcat(@NonNull String... groups) {
-        toggleProtoLogToLogcat(true, groups);
+    public void enableProtoLogToLogcat(@NonNull PrintWriter pw, @NonNull String... groups) {
+        toggleProtoLogToLogcat(pw, true, groups);
     }
 
     /**
@@ -192,8 +193,8 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ
      * @param groups we want to disable from being logged to logcat.
      */
     @Override
-    public void disableProtoLogToLogcat(@NonNull String... groups) {
-        toggleProtoLogToLogcat(false, groups);
+    public void disableProtoLogToLogcat(@NonNull PrintWriter pw, @NonNull String... groups) {
+        toggleProtoLogToLogcat(pw, false, groups);
     }
 
     /**
@@ -249,7 +250,9 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ
         }
     }
 
-    private void toggleProtoLogToLogcat(boolean enabled, @NonNull String[] groups) {
+    private void toggleProtoLogToLogcat(
+            @NonNull PrintWriter pw, boolean enabled, @NonNull String[] groups
+    ) {
         final var clientToGroups = new HashMap<IProtoLogClient, Set<String>>();
 
         for (String group : groups) {
@@ -257,8 +260,10 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ
 
             if (clients == null) {
                 // No clients associated to this group
-                Log.w(LOG_TAG, "Attempting to toggle log to logcat for group " + group
-                        + " with no registered clients.");
+                var warning = "Attempting to toggle log to logcat for group " + group
+                        + " with no registered clients. This is a no-op.";
+                Log.w(LOG_TAG, warning);
+                pw.println("WARNING: " + warning);
                 continue;
             }
 
@@ -270,8 +275,14 @@ public class ProtoLogConfigurationServiceImpl extends IProtoLogConfigurationServ
 
         for (IProtoLogClient client : clientToGroups.keySet()) {
             try {
-                client.toggleLogcat(enabled, clientToGroups.get(client).toArray(new String[0]));
+                final var clientGroups = clientToGroups.get(client).toArray(new String[0]);
+                pw.println("Toggling logcat logging for client " + client.toString()
+                        + " to " + enabled + " for groups: ["
+                        + String.join(", ", clientGroups) + "]");
+                client.toggleLogcat(enabled, clientGroups);
+                pw.println("- Done");
             } catch (RemoteException e) {
+                pw.println("- Failed");
                 throw new RuntimeException(
                         "Failed to toggle logcat status for groups on client", e);
             }
diff --git a/tests/Tracing/src/com/android/internal/protolog/ProtoLogCommandHandlerTest.java b/tests/Tracing/src/com/android/internal/protolog/ProtoLogCommandHandlerTest.java
index be0c7daebb57..e62a89b17927 100644
--- a/tests/Tracing/src/com/android/internal/protolog/ProtoLogCommandHandlerTest.java
+++ b/tests/Tracing/src/com/android/internal/protolog/ProtoLogCommandHandlerTest.java
@@ -19,6 +19,7 @@ package com.android.internal.protolog;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.contains;
 import static org.mockito.ArgumentMatchers.endsWith;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.atLeast;
 import static org.mockito.Mockito.times;
 
@@ -157,13 +158,15 @@ public class ProtoLogCommandHandlerTest {
 
         cmdHandler.exec(mMockBinder, FileDescriptor.in, FileDescriptor.out,
                 FileDescriptor.err, new String[] { "logcat", "enable", "MY_GROUP" });
-        Mockito.verify(mProtoLogConfigurationService).enableProtoLogToLogcat("MY_GROUP");
+        Mockito.verify(mProtoLogConfigurationService)
+                .enableProtoLogToLogcat(Mockito.any(), eq("MY_GROUP"));
 
         cmdHandler.exec(mMockBinder, FileDescriptor.in, FileDescriptor.out,
                 FileDescriptor.err,
                 new String[] { "logcat", "enable", "MY_GROUP", "MY_OTHER_GROUP" });
         Mockito.verify(mProtoLogConfigurationService)
-                .enableProtoLogToLogcat("MY_GROUP", "MY_OTHER_GROUP");
+                .enableProtoLogToLogcat(Mockito.any(),
+                        eq("MY_GROUP"), eq("MY_OTHER_GROUP"));
     }
 
     @Test
@@ -173,13 +176,15 @@ public class ProtoLogCommandHandlerTest {
 
         cmdHandler.exec(mMockBinder, FileDescriptor.in, FileDescriptor.out,
                 FileDescriptor.err, new String[] { "logcat", "disable", "MY_GROUP" });
-        Mockito.verify(mProtoLogConfigurationService).disableProtoLogToLogcat("MY_GROUP");
+        Mockito.verify(mProtoLogConfigurationService)
+                .disableProtoLogToLogcat(Mockito.any(), eq("MY_GROUP"));
 
         cmdHandler.exec(mMockBinder, FileDescriptor.in, FileDescriptor.out,
                 FileDescriptor.err,
                 new String[] { "logcat", "disable", "MY_GROUP", "MY_OTHER_GROUP" });
         Mockito.verify(mProtoLogConfigurationService)
-                .disableProtoLogToLogcat("MY_GROUP", "MY_OTHER_GROUP");
+                .disableProtoLogToLogcat(Mockito.any(),
+                        eq("MY_GROUP"), eq("MY_OTHER_GROUP"));
     }
 
     @Test
diff --git a/tests/Tracing/src/com/android/internal/protolog/ProtoLogConfigurationServiceTest.java b/tests/Tracing/src/com/android/internal/protolog/ProtoLogConfigurationServiceTest.java
index 3be725101252..1f3f91ebf557 100644
--- a/tests/Tracing/src/com/android/internal/protolog/ProtoLogConfigurationServiceTest.java
+++ b/tests/Tracing/src/com/android/internal/protolog/ProtoLogConfigurationServiceTest.java
@@ -62,6 +62,7 @@ import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.io.PrintWriter;
 import java.util.List;
 
 /**
@@ -234,7 +235,7 @@ public class ProtoLogConfigurationServiceTest {
         service.registerClient(mMockClient, args);
 
         Truth.assertThat(service.isLoggingToLogcat(TEST_GROUP)).isFalse();
-        service.enableProtoLogToLogcat(TEST_GROUP);
+        service.enableProtoLogToLogcat(Mockito.mock(PrintWriter.class), TEST_GROUP);
         Truth.assertThat(service.isLoggingToLogcat(TEST_GROUP)).isTrue();
 
         Mockito.verify(mMockClient).toggleLogcat(eq(true),
@@ -251,7 +252,7 @@ public class ProtoLogConfigurationServiceTest {
         service.registerClient(mMockClient, args);
 
         Truth.assertThat(service.isLoggingToLogcat(TEST_GROUP)).isTrue();
-        service.disableProtoLogToLogcat(TEST_GROUP);
+        service.disableProtoLogToLogcat(Mockito.mock(PrintWriter.class), TEST_GROUP);
         Truth.assertThat(service.isLoggingToLogcat(TEST_GROUP)).isFalse();
 
         Mockito.verify(mMockClient).toggleLogcat(eq(false),
@@ -269,7 +270,7 @@ public class ProtoLogConfigurationServiceTest {
         service.registerClient(mMockClient, args);
 
         Truth.assertThat(service.isLoggingToLogcat(TEST_GROUP)).isFalse();
-        service.enableProtoLogToLogcat(OTHER_TEST_GROUP);
+        service.enableProtoLogToLogcat(Mockito.mock(PrintWriter.class), OTHER_TEST_GROUP);
         Truth.assertThat(service.isLoggingToLogcat(TEST_GROUP)).isFalse();
 
         Mockito.verify(mMockClient, never()).toggleLogcat(anyBoolean(), any());
@@ -280,7 +281,7 @@ public class ProtoLogConfigurationServiceTest {
         final ProtoLogConfigurationService service = new ProtoLogConfigurationServiceImpl();
 
         Truth.assertThat(service.getGroups()).asList().doesNotContain(TEST_GROUP);
-        service.enableProtoLogToLogcat(TEST_GROUP);
+        service.enableProtoLogToLogcat(Mockito.mock(PrintWriter.class), TEST_GROUP);
         Truth.assertThat(service.isLoggingToLogcat(TEST_GROUP)).isTrue();
 
         final RegisterClientArgs args = new RegisterClientArgs();