Add error prone checks for internal javadoc and private constructors (#…

…6844)

buildSrc/src/main/kotlin/otel.errorprone-conventions.gradle.kts

@@ -10,6 +10,7 @@ plugins {
 dependencies {
   errorprone("com.google.errorprone:error_prone_core")
   errorprone("com.uber.nullaway:nullaway")
+  errorprone(project(":custom-checks"))
 }
 
 val disableErrorProne = properties["disableErrorProne"]?.toString()?.toBoolean() ?: false
@@ -86,9 +87,11 @@ tasks {
         // cognitive load is dubious.
         disable("YodaCondition")
 
-        if (name.contains("Jmh") || name.contains("Test")) {
+        if ((name.contains("Jmh") || name.contains("Test") || project.name.contains("testing-internal")) && !project.name.equals("custom-checks")) {
           // Allow underscore in test-type method names
           disable("MemberName")
+          // Internal javadoc not needed for test or jmh classes
+          disable("OtelInternalJavadoc")
         }
 
         option("NullAway:CustomContractAnnotations", "io.opentelemetry.api.internal.Contract")

custom-checks/build.gradle.kts

@@ -0,0 +1,82 @@
+plugins {
+  id("otel.java-conventions")
+}
+
+dependencies {
+  implementation("com.google.errorprone:error_prone_core")
+
+  testImplementation("com.google.errorprone:error_prone_test_helpers")
+}
+
+otelJava.moduleName.set("io.opentelemetry.javaagent.customchecks")
+
+// We cannot use "--release" javac option here because that will forbid exporting com.sun.tools package.
+// We also can't seem to use the toolchain without the "--release" option. So disable everything.
+
+java {
+  sourceCompatibility = JavaVersion.VERSION_17
+  targetCompatibility = JavaVersion.VERSION_17
+  toolchain {
+    languageVersion.set(null as JavaLanguageVersion?)
+  }
+}
+
+tasks {
+  withType<JavaCompile>().configureEach {
+    with(options) {
+      release.set(null as Int?)
+
+      compilerArgs.addAll(
+        listOf(
+          "--add-exports",
+          "jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",
+          "--add-exports",
+          "jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
+          "--add-exports",
+          "jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED",
+          "--add-exports",
+          "jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
+          "--add-exports",
+          "jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
+        ),
+      )
+    }
+  }
+
+  // only test on java 17+
+  val testJavaVersion: String? by project
+  if (testJavaVersion != null && Integer.valueOf(testJavaVersion) < 17) {
+    test {
+      enabled = false
+    }
+  }
+}
+
+tasks.withType<Test>().configureEach {
+  // required on jdk17
+  jvmArgs("--add-opens=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED")
+  jvmArgs("--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED")
+  jvmArgs("--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED")
+  jvmArgs("--add-opens=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED")
+  jvmArgs("--add-opens=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED")
+  jvmArgs("--add-opens=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED")
+  jvmArgs("--add-opens=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED")
+  jvmArgs("--add-opens=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED")
+  jvmArgs("-XX:+IgnoreUnrecognizedVMOptions")
+}
+
+tasks.withType<Javadoc>().configureEach {
+  // using com.sun.tools.javac.api.JavacTrees breaks javadoc generation
+  enabled = false
+}
+
+// Our conventions apply this project as a dependency in the errorprone configuration, which would cause
+// a circular dependency if trying to compile this project with that still there. So we filter this
+// project out.
+configurations {
+  named("errorprone") {
+    dependencies.removeIf {
+      it is ProjectDependency && it.dependencyProject == project
+    }
+  }
+}

custom-checks/src/main/java/io/opentelemetry/gradle/customchecks/OtelInternalJavadoc.java

@@ -0,0 +1,72 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.gradle.customchecks;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.doctree.DocCommentTree;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.PackageTree;
+import com.sun.tools.javac.api.JavacTrees;
+import java.util.regex.Pattern;
+import javax.annotation.Nullable;
+import javax.lang.model.element.Modifier;
+
+@BugPattern(
+    summary =
+        "This public internal class doesn't end with the javadoc disclaimer: \""
+            + OtelInternalJavadoc.EXPECTED_INTERNAL_COMMENT
+            + "\"",
+    severity = WARNING)
+public class OtelInternalJavadoc extends BugChecker implements BugChecker.ClassTreeMatcher {
+
+  private static final long serialVersionUID = 1L;
+
+  private static final Pattern INTERNAL_PACKAGE_PATTERN = Pattern.compile("\\binternal\\b");
+
+  static final String EXPECTED_INTERNAL_COMMENT =
+      "This class is internal and is hence not for public use."
+          + " Its APIs are unstable and can change at any time.";
+
+  @Override
+  public Description matchClass(ClassTree tree, VisitorState state) {
+    if (!isPublic(tree) || !isInternal(state) || tree.getSimpleName().toString().endsWith("Test")) {
+      return Description.NO_MATCH;
+    }
+    String javadoc = getJavadoc(state);
+    if (javadoc != null && javadoc.contains(EXPECTED_INTERNAL_COMMENT)) {
+      return Description.NO_MATCH;
+    }
+    return describeMatch(tree);
+  }
+
+  private static boolean isPublic(ClassTree tree) {
+    return tree.getModifiers().getFlags().contains(Modifier.PUBLIC);
+  }
+
+  private static boolean isInternal(VisitorState state) {
+    PackageTree packageTree = state.getPath().getCompilationUnit().getPackage();
+    if (packageTree == null) {
+      return false;
+    }
+    String packageName = state.getSourceForNode(packageTree.getPackageName());
+    return packageName != null && INTERNAL_PACKAGE_PATTERN.matcher(packageName).find();
+  }
+
+  @Nullable
+  private static String getJavadoc(VisitorState state) {
+    DocCommentTree docCommentTree =
+        JavacTrees.instance(state.context).getDocCommentTree(state.getPath());
+    if (docCommentTree == null) {
+      return null;
+    }
+    return docCommentTree.toString().replace("\n", "");
+  }
+}

custom-checks/src/main/java/io/opentelemetry/gradle/customchecks/OtelPrivateConstructorForUtilityClass.java

@@ -0,0 +1,38 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.gradle.customchecks;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.PrivateConstructorForUtilityClass;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.ClassTree;
+
+@BugPattern(
+    summary =
+        "Classes which are not intended to be instantiated should be made non-instantiable with a private constructor. This includes utility classes (classes with only static members), and the main class.",
+    severity = WARNING)
+public class OtelPrivateConstructorForUtilityClass extends BugChecker
+    implements BugChecker.ClassTreeMatcher {
+
+  private static final long serialVersionUID = 1L;
+
+  private final PrivateConstructorForUtilityClass delegate =
+      new PrivateConstructorForUtilityClass();
+
+  @Override
+  public Description matchClass(ClassTree tree, VisitorState state) {
+    Description description = delegate.matchClass(tree, state);
+    if (description == NO_MATCH) {
+      return description;
+    }
+    return describeMatch(tree);
+  }
+}

custom-checks/src/main/resources/META-INF/services/com.google.errorprone.bugpatterns.BugChecker

@@ -0,0 +1,2 @@
+io.opentelemetry.gradle.customchecks.OtelInternalJavadoc
+io.opentelemetry.gradle.customchecks.OtelPrivateConstructorForUtilityClass

custom-checks/src/test/java/io/opentelemetry/gradle/customchecks/OtelInternalJavadocTest.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.gradle.customchecks;
+
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+class OtelInternalJavadocTest {
+
+  @Test
+  void test() {
+    doTest("internal/InternalJavadocPositiveCases.java");
+    doTest("internal/InternalJavadocNegativeCases.java");
+  }
+
+  private static void doTest(String path) {
+    CompilationTestHelper.newInstance(OtelInternalJavadoc.class, OtelInternalJavadocTest.class)
+        .addSourceFile(path)
+        .doTest();
+  }
+}

custom-checks/src/test/resources/io/opentelemetry/gradle/customchecks/internal/InternalJavadocNegativeCases.java

@@ -0,0 +1,21 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.gradle.customchecks.internal;
+
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public class InternalJavadocNegativeCases {
+
+  /**
+   * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+   * any time.
+   */
+  public static class One {}
+
+  static class Two {}
+}

custom-checks/src/test/resources/io/opentelemetry/gradle/customchecks/internal/InternalJavadocPositiveCases.java

@@ -0,0 +1,17 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.gradle.customchecks.internal;
+
+// BUG: Diagnostic contains: doesn't end with the javadoc disclaimer
+public class InternalJavadocPositiveCases {
+
+  // BUG: Diagnostic contains: doesn't end with the javadoc disclaimer
+  public static class One {}
+
+  /** Doesn't have the disclaimer. */
+  // BUG: Diagnostic contains: doesn't end with the javadoc disclaimer
+  public static class Two {}
+}

dependencyManagement/build.gradle.kts

@@ -38,6 +38,7 @@ val DEPENDENCIES = listOf(
   "com.google.auto.value:auto-value-annotations:${autoValueVersion}",
   "com.google.errorprone:error_prone_annotations:${errorProneVersion}",
   "com.google.errorprone:error_prone_core:${errorProneVersion}",
+  "com.google.errorprone:error_prone_test_helpers:${errorProneVersion}",
   "io.opencensus:opencensus-api:${opencensusVersion}",
   "io.opencensus:opencensus-impl-core:${opencensusVersion}",
   "io.opencensus:opencensus-impl:${opencensusVersion}",

exporters/common/src/main/java/io/opentelemetry/exporter/internal/FailedExportException.java

@@ -46,7 +46,12 @@ public static GrpcExportException grpcFailedExceptionally(Throwable cause) {
   /** Returns true if the export failed with a response from the server. */
   public abstract boolean failedWithResponse();
 
-  /** Represents the failure of an HTTP exporter. */
+  /**
+   * Represents the failure of an HTTP exporter.
+   *
+   * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+   * at any time.
+   */
   public static final class HttpExportException extends FailedExportException {
 
     private static final long serialVersionUID = -6787390183017184775L;
@@ -85,7 +90,12 @@ public Throwable getCause() {
     }
   }
 
-  /** Represents the failure of a gRPC exporter. */
+  /**
+   * Represents the failure of a gRPC exporter.
+   *
+   * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+   * at any time.
+   */
   public static final class GrpcExportException extends FailedExportException {
 
     private static final long serialVersionUID = -9157548250286695364L;

exporters/common/src/main/java/io/opentelemetry/exporter/internal/InstrumentationUtil.java

@@ -9,7 +9,7 @@
 
 /**
  * This class is internal and is hence not for public use. Its APIs are unstable and can change at
- * any time
+ * any time.
  *
  * @deprecated use {@link io.opentelemetry.api.internal.InstrumentationUtil} instead. This class
  *     should be removed once instrumentation does not refer to it anymore.

exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpSender.java

@@ -41,7 +41,12 @@ void send(
   /** Shutdown the sender. */
   CompletableResultCode shutdown();
 
-  /** The HTTP response. */
+  /**
+   * The HTTP response.
+   *
+   * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+   * at any time.
+   */
   interface Response {
 
     /** The HTTP status code. */

exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/MarshalerContext.java

@@ -21,6 +21,9 @@
  * objects, that we call data. Both integers and objects can be read from the state in the order
  * they were added (first in, first out). Additionally, this class provides various pools and caches
  * for objects that can be reused between marshalling attempts.
+ *
+ * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+ * at any time.
  */
 public final class MarshalerContext {
   private final boolean marshalStringNoAllocation;
@@ -203,6 +206,10 @@ public void reset() {
 
   private static final AtomicInteger KEY_INDEX = new AtomicInteger();
 
+  /**
+   * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+   * any time.
+   */
   public static class Key {
     final int index = KEY_INDEX.getAndIncrement();
   }

exporters/otlp/common/src/main/java/io/opentelemetry/exporter/internal/otlp/KeyValueStatelessMarshaler.java

@@ -12,7 +12,12 @@
 import io.opentelemetry.exporter.internal.marshal.StatelessMarshalerUtil;
 import java.io.IOException;
 
-/** A Marshaler of key value pairs. See {@link AnyValueMarshaler}. */
+/**
+ * A Marshaler of key value pairs. See {@link AnyValueMarshaler}.
+ *
+ * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+ * at any time.
+ */
 public final class KeyValueStatelessMarshaler implements StatelessMarshaler<KeyValue> {
 
   public static final KeyValueStatelessMarshaler INSTANCE = new KeyValueStatelessMarshaler();

exporters/otlp/common/src/main/java/io/opentelemetry/exporter/internal/otlp/traces/SpanFlags.java

@@ -11,6 +11,9 @@
  * Represents the 32 bit span flags <a
  * href="https://github.com/open-telemetry/opentelemetry-proto/blob/342e1d4c3a1fe43312823ffb53bd38327f263059/opentelemetry/proto/trace/v1/trace.proto#L133">as
  * specified in the proto definition</a>.
+ *
+ * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+ * at any time.
  */
 public final class SpanFlags {
   // As defined at:

sdk-extensions/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/AutoConfigureListener.java

@@ -13,6 +13,9 @@
  *
  * <p>This is not a standalone SPI. Instead, implementations of other SPIs can also implement this
  * interface to receive a callback with the configured SDK.
+ *
+ * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+ * at any time.
  */
 public interface AutoConfigureListener {