Replace BugReport.NonFatalBugReport marker interface with an explic…

…it API to report non-fatal crashes.

We recently added a new marker interface to allow distinguishing crashes vs non-fatal exceptions sent to `BugReport`. This leads to adding boilerplate code with classes created solely to carry the marker interface. Add a new `BugReporter::sendNonFatalBugReport` method which does not require special `Exception` class and makes it obvious of the type of bug report at the call site.

PiperOrigin-RevId: 462147374
Change-Id: I0e5ff5dac4d710d439dad5f55a221189cc130072

src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java

@@ -53,6 +53,8 @@ public final class BugReport {
 
   static final BugReporter REPORTER_INSTANCE = new DefaultBugReporter();
 
+  // TODO(b/232094803): Replace the static state with instance variables and allow custom overrides
+  //  for testing.
   private static final BlazeVersionInfo VERSION_INFO = BlazeVersionInfo.instance();
 
   private static BlazeRuntimeInterface runtime = null;
@@ -175,6 +177,16 @@ public static void sendBugReport(Throwable exception) {
    * @param values Additional string values to clarify the exception.
    */
   public static void sendBugReport(Throwable exception, List<String> args, String... values) {
+    sendBugReportInternal(exception, /*isFatal=*/ true, filterArgs(args), values);
+  }
+
+  /** Logs the bug report, indicating it is not a crash. */
+  public static void sendNonFatalBugReport(Throwable exception) {
+    sendBugReportInternal(exception, /*isFatal=*/ false, /*args=*/ ImmutableList.of());
+  }
+
+  private static void sendBugReportInternal(
+      Throwable exception, boolean isFatal, List<String> args, String... values) {
     if (SHOULD_NOT_SEND_BUG_REPORT_BECAUSE_IN_TEST) {
       Throwables.throwIfUnchecked(exception);
       throw new IllegalStateException(
@@ -185,7 +197,7 @@ public static void sendBugReport(Throwable exception, List<String> args, String.
       return;
     }
 
-    logException(exception, filterArgs(args), values);
+    logException(exception, isFatal, filterArgs(args), values);
   }
 
   /**
@@ -359,38 +371,42 @@ private static ImmutableList<String> filterArgs(Iterable<String> args) {
     return filteredArgs.build();
   }
 
-  // Log the exception. Because this method is only called in a blaze release, this will result in a
-  // report being sent to a remote logging service.
+  /**
+   * Logs the exception. Because this method is only called in a blaze release, this will result in
+   * a report being sent to a remote logging service.
+   *
+   * <p>TODO(b/232094803): Make this method private and replace the tests with ones calling public
+   * methods like {@link #sendBugReport(Throwable)} directly.
+   */
   @VisibleForTesting
-  static void logException(Throwable exception, List<String> args, String... values) {
+  static void logException(
+      Throwable exception, boolean isCrash, List<String> args, String... values) {
     logger.atSevere().withCause(exception).log("Exception");
     String preamble = getProductName();
-    Level level = Level.SEVERE;
-    if (exception instanceof OutOfMemoryError) {
-      preamble += " OOMError: ";
-    } else if (exception instanceof NonFatalBugReport) {
+    Level level = isCrash ? Level.SEVERE : Level.WARNING;
+    if (!isCrash) {
       preamble += " had a non fatal error with args: ";
-      level = Level.WARNING;
+    } else if (exception instanceof OutOfMemoryError) {
+      preamble += " OOMError: ";
     } else {
       preamble += " crashed with args: ";
     }
 
     LoggingUtil.logToRemote(level, preamble + Joiner.on(' ').join(args), exception, values);
   }
 
-  /**
-   * NonFatalBugReport is a marker interface for Throwables which should be reported when passed to
-   * sendBugReport, but don't represent a crash.
-   */
-  public static interface NonFatalBugReport {}
-
   private static final class DefaultBugReporter implements BugReporter {
 
     @Override
     public void sendBugReport(Throwable exception, List<String> args, String... values) {
       BugReport.sendBugReport(exception, args, values);
     }
 
+    @Override
+    public void sendNonFatalBugReport(Exception exception) {
+      BugReport.sendNonFatalBugReport(exception);
+    }
+
     @Override
     public void handleCrash(Crash crash, CrashContext ctx) {
       BugReport.handleCrash(crash, ctx);

src/main/java/com/google/devtools/build/lib/bugreport/BugReporter.java

@@ -57,6 +57,9 @@ default void sendBugReport(Throwable exception) {
   /** Reports an exception, see {@link BugReport#sendBugReport(Throwable, List, String...)}. */
   void sendBugReport(Throwable exception, List<String> args, String... values);
 
+  /** Reports a non-fatal exception, see {@link BugReport#sendNonFatalBugReport(Throwable)}. */
+  void sendNonFatalBugReport(Exception exception);
+
   /** See {@link BugReport#handleCrash}. */
   void handleCrash(Crash crash, CrashContext ctx);
 }

src/main/java/com/google/devtools/build/lib/query2/query/TargetEdgeErrorObserver.java

@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.query2.query;
 
 import com.google.devtools.build.lib.bugreport.BugReport;
-import com.google.devtools.build.lib.bugreport.BugReport.NonFatalBugReport;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -97,15 +96,9 @@ public void node(Target node) {
         errorCode.compareAndSet(
             /*expectedValue=*/ null, /*newValue=*/ DetailedExitCode.of(failureDetail));
       } else {
-        BugReport.sendBugReport(new UndetailedErrorFromPackageException(node));
+        BugReport.sendNonFatalBugReport(
+            new IllegalStateException("Undetailed error from package: " + node));
       }
     }
   }
-
-  private static final class UndetailedErrorFromPackageException extends RuntimeException
-      implements NonFatalBugReport {
-    UndetailedErrorFromPackageException(Target node) {
-      super("Undetailed error from package: " + node);
-    }
-  }
 }

src/test/java/com/google/devtools/build/lib/bugreport/BugReportTest.java

@@ -28,7 +28,6 @@
 import com.google.common.collect.Lists;
 import com.google.common.testing.TestLogHandler;
 import com.google.devtools.build.lib.bugreport.BugReport.BlazeRuntimeInterface;
-import com.google.devtools.build.lib.bugreport.BugReport.NonFatalBugReport;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.EventKind;
@@ -113,34 +112,36 @@ Throwable createThrowable() {
     abstract Throwable createThrowable();
   }
 
-  private static final class NonFatalException extends RuntimeException
-      implements NonFatalBugReport {
-    NonFatalException(String message) {
-      super(message);
-    }
-  }
-
   private enum ExceptionType {
     FATAL(
         new RuntimeException("fatal exception"),
+        /*isFatal=*/ true,
         Level.SEVERE,
         "myProductName crashed with args: arg foo"),
     NONFATAL(
-        new NonFatalException("bug report"),
+        new IllegalStateException("bug report"),
+        /*isFatal=*/ false,
         Level.WARNING,
         "myProductName had a non fatal error with args: arg foo"),
-    OOM(new OutOfMemoryError("Java heap space"), Level.SEVERE, "myProductName OOMError: arg foo");
+    OOM(
+        new OutOfMemoryError("Java heap space"),
+        /*isFatal=*/ true,
+        Level.SEVERE,
+        "myProductName OOMError: arg foo");
 
     @SuppressWarnings("ImmutableEnumChecker") // I'm pretty sure no one will mutate this Throwable.
     private final Throwable throwable;
 
     @SuppressWarnings("ImmutableEnumChecker") // Same here.
     private final Level level;
 
+    private final boolean isFatal;
+
     private final String expectedMessage;
 
-    ExceptionType(Throwable throwable, Level level, String expectedMessage) {
+    ExceptionType(Throwable throwable, boolean isFatal, Level level, String expectedMessage) {
       this.throwable = throwable;
+      this.isFatal = isFatal;
       this.level = level;
       this.expectedMessage = expectedMessage;
     }
@@ -178,7 +179,8 @@ public void logException(@TestParameter ExceptionType exceptionType) throws Exce
     logger.addHandler(handler);
     LoggingUtil.installRemoteLoggerForTesting(immediateFuture(logger));
 
-    BugReport.logException(exceptionType.throwable, ImmutableList.of("arg", "foo"));
+    BugReport.logException(
+        exceptionType.throwable, exceptionType.isFatal, ImmutableList.of("arg", "foo"));
 
     LogRecord got = handler.getStoredLogRecords().get(0);
     assertThat(got.getThrown()).isSameInstanceAs(exceptionType.throwable);

src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java

@@ -1018,6 +1018,11 @@ public synchronized void sendBugReport(
       exceptions.add(exception);
     }
 
+    @Override
+    public synchronized void sendNonFatalBugReport(Exception exception) {
+      exceptions.add(exception);
+    }
+
     @FormatMethod
     @Override
     public void logUnexpected(String message, Object... args) {

src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java

@@ -550,6 +550,11 @@ public synchronized void sendBugReport(
       exceptions.add(exception);
     }
 
+    @Override
+    public synchronized void sendNonFatalBugReport(Exception exception) {
+      exceptions.add(exception);
+    }
+
     @Override
     public void handleCrash(Crash crash, CrashContext ctx) {
       // Unexpected: try to crash JVM.

src/test/java/com/google/devtools/build/lib/testutil/TestInterruptingBugReporter.java

@@ -133,6 +133,11 @@ public void sendBugReport(Throwable exception, List<String> args, String... valu
     handle(exception, "call to sendBugReport", Thread.currentThread());
   }
 
+  @Override
+  public void sendNonFatalBugReport(Exception exception) {
+    handle(exception, "call to sendNonFatalBugReport", Thread.currentThread());
+  }
+
   @Override
   public void handleCrash(Crash crash, CrashContext ctx) {
     handle(crash.getThrowable(), "call to handleCrash", Thread.currentThread());

src/test/java/com/google/devtools/build/lib/util/ThreadUtilsTest.java

@@ -55,6 +55,11 @@ public void sendBugReport(Throwable exception, List<String> args, String... valu
             reportedException.set(exception);
           }
 
+          @Override
+          public void sendNonFatalBugReport(Exception exception) {
+            throw new UnsupportedOperationException();
+          }
+
           @Override
           public void handleCrash(Crash crash, CrashContext ctx) {
             BugReporter.defaultInstance().handleCrash(crash, ctx);