Enable all errorprone checks (#3155)

* Enable all errorprone checks

* Fixes

* Finish

* Finish

* Add flag to disable error prone

benchmark-e2e/src/main/java/io/opentelemetry/e2ebenchmark/E2EAgentBenchmark.java

@@ -51,11 +51,11 @@ void tearDown() {
   }
 
   @Test
-  void run() throws Exception {
+  void run() throws InterruptedException {
     runBenchmark();
   }
 
-  private void runBenchmark() throws Exception {
+  private void runBenchmark() throws InterruptedException {
     String agentPath = System.getProperty("io.opentelemetry.smoketest.agent.shadowJar.path");
 
     // otlp collector container

benchmark-integration/jetty-perftest/src/main/java/io/opentelemetry/perftest/Worker.java

@@ -11,7 +11,7 @@
 import io.opentelemetry.context.Scope;
 import java.util.concurrent.TimeUnit;
 
-public class Worker {
+public final class Worker {
 
   private static final Tracer tracer = GlobalOpenTelemetry.getTracer("test");
 
@@ -32,4 +32,6 @@ public static void doWork(long workTimeMillis) {
       span.end();
     }
   }
+
+  private Worker() {}
 }

benchmark-integration/jetty-perftest/src/main/java/io/opentelemetry/perftest/jetty/JettyPerftest.java

@@ -18,7 +18,7 @@
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.servlet.ServletContextHandler;
 
-public class JettyPerftest {
+public final class JettyPerftest {
 
   private static final int PORT = 8080;
   private static final String PATH = "/work";
@@ -53,7 +53,7 @@ public static class PerfServlet extends HttpServlet {
     protected void doGet(HttpServletRequest request, HttpServletResponse response)
         throws IOException {
       if (request.getParameter("error") != null) {
-        throw new RuntimeException("some sync error");
+        throw new IllegalStateException("some sync error");
       }
       String workVal = request.getParameter("workTimeMillis");
       long workTimeMillis = 0L;
@@ -64,7 +64,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response)
       response.getWriter().print("Did " + workTimeMillis + "ms of work.");
     }
 
-    private void scheduleWork(long workTimeMillis) {
+    private static void scheduleWork(long workTimeMillis) {
       Span span = tracer.spanBuilder("work").startSpan();
       try (Scope scope = span.makeCurrent()) {
         if (span != null) {
@@ -79,4 +79,6 @@ private void scheduleWork(long workTimeMillis) {
       }
     }
   }
+
+  private JettyPerftest() {}
 }

benchmark-integration/play-perftest/app/controllers/HomeController.scala

@@ -29,7 +29,7 @@ class HomeController @Inject()(cc: ControllerComponents) extends AbstractControl
   def doGet(workTimeMillis: Option[Long], error: Option[String]) = Action {
     implicit request: Request[AnyContent] =>
       error match {
-        case Some(x) => throw new RuntimeException("some sync error")
+        case Some(x) => throw new IllegalStateException("some sync error")
         case None => {
           var workTime = workTimeMillis.getOrElse(0L)
           scheduleWork(workTime)

benchmark/src/jmh/java/io/opentelemetry/benchmark/HttpBenchmark.java

@@ -34,8 +34,11 @@ public void doSetup() {
         while (!AbstractLifeCycle.STARTED.equals(jettyServer.getState())) {
           Thread.sleep(500);
         }
+      } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+        throw new IllegalStateException(e);
       } catch (Exception e) {
-        throw new RuntimeException(e);
+        throw new IllegalStateException(e);
       }
     }
 

benchmark/src/jmh/java/io/opentelemetry/benchmark/InstrumenterBenchmark.java

@@ -55,7 +55,8 @@ public Context startEnd() {
   }
 
   static class ConstantHttpAttributesExtractor extends HttpAttributesExtractor<Void, Void> {
-    static HttpAttributesExtractor<Void, Void> INSTANCE = new ConstantHttpAttributesExtractor();
+    static final HttpAttributesExtractor<Void, Void> INSTANCE =
+        new ConstantHttpAttributesExtractor();
 
     @Override
     protected @Nullable String method(Void unused) {

benchmark/src/jmh/java/io/opentelemetry/benchmark/TypeMatchingBenchmark.java

@@ -50,7 +50,7 @@ public class TypeMatchingBenchmark {
           }
         }
       } catch (IOException e) {
-        throw new RuntimeException(e);
+        throw new IllegalStateException(e);
       }
     }
   }

benchmark/src/jmh/java/io/opentelemetry/benchmark/classes/A.java

@@ -5,6 +5,7 @@
 
 package io.opentelemetry.benchmark.classes;
 
+@SuppressWarnings("ClassNamedLikeTypeParameter")
 public interface A {
   void a();
 }

benchmark/src/jmh/java/io/opentelemetry/benchmark/classes/B.java

@@ -5,6 +5,7 @@
 
 package io.opentelemetry.benchmark.classes;
 
+@SuppressWarnings("ClassNamedLikeTypeParameter")
 public interface B extends A {
   void b();
 }

benchmark/src/jmh/java/io/opentelemetry/benchmark/classes/C.java

@@ -5,6 +5,7 @@
 
 package io.opentelemetry.benchmark.classes;
 
+@SuppressWarnings("ClassNamedLikeTypeParameter")
 public interface C extends A, B {
   void c();
 }

benchmark/src/jmh/java/io/opentelemetry/benchmark/classes/D.java

@@ -5,6 +5,7 @@
 
 package io.opentelemetry.benchmark.classes;
 
+@SuppressWarnings("ClassNamedLikeTypeParameter")
 public interface D extends A, B, C {
   void d();
 }

benchmark/src/jmh/java/io/opentelemetry/benchmark/classes/E.java

@@ -5,6 +5,7 @@
 
 package io.opentelemetry.benchmark.classes;
 
+@SuppressWarnings("ClassNamedLikeTypeParameter")
 public interface E extends B, C, D {
   void e();
 }

benchmark/src/jmh/java/io/opentelemetry/benchmark/classes/F.java

@@ -5,6 +5,7 @@
 
 package io.opentelemetry.benchmark.classes;
 
+@SuppressWarnings("ClassNamedLikeTypeParameter")
 public abstract class F implements E {
   public abstract void f();
 

benchmark/src/jmh/java/io/opentelemetry/benchmark/classes/HttpClass.java

@@ -19,8 +19,8 @@
 import org.eclipse.jetty.servlet.ServletContextHandler;
 
 public class HttpClass {
-  private final String contextPath = "/path";
-  private final Integer port = 18888;
+  private static final String contextPath = "/path";
+  private static final Integer port = 18888;
 
   public Server buildJettyServer() {
     System.setProperty("org.eclipse.jetty.util.log.class", "org.eclipse.jetty.util.log.StdErrLog");

build.gradle

@@ -79,7 +79,10 @@ allprojects {
 
     tasks.withType(JavaCompile).configureEach {
       options.errorprone {
+        enabled = rootProject.findProperty("disableErrorProne") != "true"
         disableWarningsInGeneratedCode = true
+        allDisabledChecksAsWarnings = true
+
         excludedPaths = ".*/build/generated/.*"
 
         // Doesn't work well with Java 8
@@ -89,6 +92,15 @@ allprojects {
         disable("AutoValueImmutableFields")
         disable("StringSplitter")
 
+        // Don't currently use this (to indicate a local variable that's mutated) but could
+        // consider for future.
+        disable("Var")
+
+        // Don't support Android without desugar
+        disable("AndroidJdkLibsChecker")
+        disable("Java7ApiChecker")
+        disable("StaticOrDefaultInterfaceMethod")
+
         // Great check, but for bytecode manipulation it's too common to separate over
         // onEnter / onExit
         // TODO(anuraaga): Only disable for auto instrumentation project.
@@ -101,13 +113,29 @@ allprojects {
         // We end up using obsolete types if a library we're instrumenting uses them.
         disable("JdkObsolete")
 
+        // Limits API possibilities
+        disable("NoFunctionalReturnType")
+
         // Storing into a variable in onEnter triggers this unfortunately.
         // TODO(anuraaga): Only disable for auto instrumentation project.
         disable("UnusedVariable")
 
         // TODO(anuraaga): Remove this, we use this pattern in several tests and it will mean
         // some moving.
         disable("DefaultPackage")
+
+        // TODO(anuraaga): Remove this, all our advice classes miss constructors but probably should
+        // address this.
+        disable("PrivateConstructorForUtilityClass")
+
+        // TODO(anuraaga): Remove this, probably after instrumenter API migration instead of dealing
+        // with older APIs.
+        disable("InconsistentOverloads")
+        disable("TypeParameterNaming")
+
+        if (name.contains("Jmh") || name.contains("Test")) {
+          disable("FieldMissingNullable")
+        }
       }
     }
   }

buildSrc/src/main/java/io/opentelemetry/instrumentation/gradle/bytebuddy/ClasspathByteBuddyPlugin.java

@@ -64,7 +64,7 @@ private static Plugin pluginFromClassPath(
       Class<?> clazz = Class.forName(className, false, classLoader);
       return (Plugin) clazz.getDeclaredConstructor().newInstance();
     } catch (Exception e) {
-      throw new RuntimeException("Failed to create ByteBuddy plugin instance", e);
+      throw new IllegalStateException("Failed to create ByteBuddy plugin instance", e);
     }
   }
 
@@ -84,7 +84,7 @@ private static URL fileAsUrl(File file) {
     try {
       return file.toURI().toURL();
     } catch (MalformedURLException e) {
-      throw new RuntimeException("Cannot resolve " + file + " as URL", e);
+      throw new IllegalStateException("Cannot resolve " + file + " as URL", e);
     }
   }
 }

buildSrc/src/main/java/io/opentelemetry/instrumentation/gradle/muzzle/MuzzlePlugin.java

@@ -105,7 +105,7 @@ public void apply(Project project) {
                               .getMethod("printMuzzleReferences", ClassLoader.class);
                       assertionMethod.invoke(null, instrumentationCL);
                     } catch (Exception e) {
-                      throw new RuntimeException(e);
+                      throw new IllegalStateException(e);
                     }
                   });
             });
@@ -191,7 +191,7 @@ private static ClassLoader classpathLoader(
                   try {
                     return file.toURI().toURL();
                   } catch (MalformedURLException e) {
-                    throw new RuntimeException(e);
+                    throw new IllegalStateException(e);
                   }
                 })
             .toArray(URL[]::new);
@@ -305,7 +305,7 @@ public String toString() {
                               userCL,
                               muzzleDirective.getAssertPass().get());
                         } catch (Exception e) {
-                          throw new RuntimeException(e);
+                          throw new IllegalStateException(e);
                         } finally {
                           Thread.currentThread().setContextClassLoader(ccl);
                         }
@@ -366,7 +366,7 @@ private static Set<Artifact> muzzleDirectiveToArtifacts(
     try {
       rangeResult = system.resolveVersionRange(session, rangeRequest);
     } catch (VersionRangeResolutionException e) {
-      throw new RuntimeException(e);
+      throw new IllegalStateException(e);
     }
 
     Set<Artifact> allVersionArtifacts =
@@ -426,7 +426,7 @@ private static Set<MuzzleDirective> inverseOf(
     try {
       allRangeResult = system.resolveVersionRange(session, allRangeRequest);
     } catch (VersionRangeResolutionException e) {
-      throw new RuntimeException(e);
+      throw new IllegalStateException(e);
     }
 
     VersionRangeRequest rangeRequest = new VersionRangeRequest();
@@ -436,7 +436,7 @@ private static Set<MuzzleDirective> inverseOf(
     try {
       rangeResult = system.resolveVersionRange(session, rangeRequest);
     } catch (VersionRangeResolutionException e) {
-      throw new RuntimeException(e);
+      throw new IllegalStateException(e);
     }
 
     allRangeResult.getVersions().removeAll(rangeResult.getVersions());

dependencyManagement/dependencyManagement.gradle.kts

@@ -84,7 +84,7 @@ val DEPENDENCIES = listOf(
   "info.solidsoft.spock:spock-global-unroll:0.5.1",
   "org.assertj:assertj-core:3.19.0",
   "org.awaitility:awaitility:4.0.3",
-  "org.checkerframework:checker-qual:3.6.1",
+  "org.checkerframework:checker-qual:3.13.0",
   "org.codehaus.groovy:groovy-all:${groovyVersion}",
   "org.objenesis:objenesis:3.1",
   "org.spockframework:spock-core:1.3-groovy-2.5",

gradle/enforcement/checkstyle.xml

@@ -133,7 +133,6 @@
     <module name="OneStatementPerLine"/>
     <module name="MultipleVariableDeclarations"/>
     <module name="ArrayTypeStyle"/>
-    <module name="MissingSwitchDefault"/>
     <module name="FallThrough"/>
     <module name="UpperEll"/>
     <module name="ModifierOrder"/>

instrumentation-api-caching/src/main/java/io/opentelemetry/instrumentation/api/caching/CacheBuilder.java

@@ -7,6 +7,7 @@
 
 import com.github.benmanes.caffeine.cache.Caffeine;
 import java.util.concurrent.Executor;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 /** A builder of {@link Cache}. */
 public final class CacheBuilder {
@@ -16,7 +17,7 @@ public final class CacheBuilder {
   private boolean weakKeys;
   private boolean weakValues;
   private long maximumSize = UNSET;
-  private Executor executor = null;
+  @Nullable private Executor executor = null;
 
   /** Sets the maximum size of the cache. */
   public CacheBuilder setMaximumSize(long maximumSize) {

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/InstrumentationVersion.java

@@ -5,7 +5,9 @@
 
 package io.opentelemetry.instrumentation.api;
 
-public class InstrumentationVersion {
+public final class InstrumentationVersion {
   public static final String VERSION =
       InstrumentationVersion.class.getPackage().getImplementationVersion();
+
+  private InstrumentationVersion() {}
 }

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/Config.java

@@ -23,29 +23,29 @@ public abstract class Config {
 
   // lazy initialized, so that javaagent can set it, and library instrumentation can fall back and
   // read system properties
-  private static volatile Config INSTANCE = null;
+  @Nullable private static volatile Config instance = null;
 
   /**
    * Sets the agent configuration singleton. This method is only supposed to be called once, from
    * the agent classloader just before the first instrumentation is loaded (and before {@link
    * Config#get()} is used for the first time).
    */
   public static void internalInitializeConfig(Config config) {
-    if (INSTANCE != null) {
+    if (instance != null) {
       log.warn("Config#INSTANCE was already set earlier");
       return;
     }
-    INSTANCE = requireNonNull(config);
+    instance = requireNonNull(config);
   }
 
   public static Config get() {
-    if (INSTANCE == null) {
+    if (instance == null) {
       // this should only happen in library instrumentation
       //
       // no need to synchronize because worst case is creating INSTANCE more than once
-      INSTANCE = new ConfigBuilder().readEnvironmentVariables().readSystemProperties().build();
+      instance = new ConfigBuilder().readEnvironmentVariables().readSystemProperties().build();
     }
-    return INSTANCE;
+    return instance;
   }
 
   public static Config create(Map<String, String> allProperties) {

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/db/RedisCommandSanitizer.java

@@ -423,4 +423,6 @@ public String sanitize(String command, List<?> args) {
       }
     }
   }
+
+  private RedisCommandSanitizer() {}
 }

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

@@ -58,8 +58,8 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> newBuil
   private final List<? extends AttributesExtractor<? super REQUEST, ? super RESPONSE>> extractors;
   private final List<? extends RequestListener> requestListeners;
   private final ErrorCauseExtractor errorCauseExtractor;
-  private final StartTimeExtractor<REQUEST> startTimeExtractor;
-  private final EndTimeExtractor<RESPONSE> endTimeExtractor;
+  @Nullable private final StartTimeExtractor<REQUEST> startTimeExtractor;
+  @Nullable private final EndTimeExtractor<RESPONSE> endTimeExtractor;
 
   Instrumenter(InstrumenterBuilder<REQUEST, RESPONSE> builder) {
     this.instrumentationName = builder.instrumentationName;