Add InternalJavadoc custom error prone check (#5277)

* Add InternalJavadoc custom error prone check

* Add example usage

* Move to conventions

* Revert "Move to conventions"

This reverts commit d8a8209.

* Just get it working

* Clearer error message

* versions

* Apply almost everywhere

* feedback

* Always at the end of javadoc

* Fix test

* Missed (at least) one

* No longer internal

* Fix NPE

* Spotless

* Convert awslambda Java test to JUnit 5 so can reduce visibility

* Reduce visibility

* Rename the check

* More

* Move into errorprone-convention

* Fix UserExcludedClassesConfigurerTest

Author: trask

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

@@ -6,6 +6,7 @@ plugins {
 
 dependencies {
   errorprone("com.google.errorprone:error_prone_core")
+  errorprone(project(":custom-checks"))
 }
 
 val disableErrorProne = properties["disableErrorProne"]?.toString()?.toBoolean() ?: false

custom-checks/build.gradle.kts

@@ -0,0 +1,29 @@
+plugins {
+  `java-library`
+}
+
+dependencies {
+  implementation("com.google.errorprone:error_prone_core:2.10.0")
+
+  annotationProcessor("com.google.auto.service:auto-service:1.0.1")
+  compileOnly("com.google.auto.service:auto-service-annotations:1.0.1")
+
+  testImplementation("org.junit.jupiter:junit-jupiter-api:5.8.2")
+  testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:5.8.2")
+
+  testImplementation("com.google.errorprone:error_prone_test_helpers:2.11.0")
+}
+
+tasks {
+  test {
+    useJUnitPlatform()
+  }
+
+  withType<JavaCompile> {
+    options.compilerArgs.addAll(listOf(
+      "--add-exports", "jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",
+      "--add-exports", "jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
+      "--add-exports", "jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED"
+    ))
+  }
+}

custom-checks/src/main/java/io/opentelemetry/javaagent/customchecks/InternalJavadoc.java

@@ -0,0 +1,77 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.customchecks;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+
+import com.google.auto.service.AutoService;
+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;
+
+@AutoService(BugChecker.class)
+@BugPattern(
+    name = "OtelInternalJavadoc",
+    summary =
+        "This public internal class doesn't end with the javadoc disclaimer: \""
+            + InternalJavadoc.EXPECTED_INTERNAL_COMMENT
+            + "\"",
+    severity = WARNING)
+public class InternalJavadoc extends BugChecker implements BugChecker.ClassTreeMatcher {
+
+  private static final Pattern INTERNAL_PACKAGE_PATTERN = Pattern.compile("\\binternal\\b");
+
+  private static final Pattern EXCLUDE_PACKAGE_PATTERN =
+      Pattern.compile("^io\\.opentelemetry\\.javaagent\\.instrumentation\\.internal\\.");
+
+  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)) {
+      return Description.NO_MATCH;
+    }
+    String javadoc = getJavadoc(state);
+    if (javadoc != null && javadoc.endsWith(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 = packageTree.getPackageName().toString();
+    return INTERNAL_PACKAGE_PATTERN.matcher(packageName).find()
+        && !EXCLUDE_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/test/java/io/opentelemetry/javaagent/customchecks/InternalJavadocTest.java

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

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

@@ -0,0 +1,21 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.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/javaagent/customchecks/internal/InternalJavadocPositiveCases.java

@@ -0,0 +1,17 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.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

@@ -73,7 +73,7 @@ val DEPENDENCY_SETS = listOf(
   DependencySet(
     "org.mockito",
     "4.2.0",
-    listOf("mockito-core", "mockito-junit-jupiter")
+    listOf("mockito-core", "mockito-junit-jupiter", "mockito-inline")
   ),
   DependencySet(
     "org.slf4j",
@@ -91,6 +91,7 @@ val DEPENDENCIES = listOf(
   "ch.qos.logback:logback-classic:1.2.10",
   "com.github.stefanbirkner:system-lambda:1.2.1",
   "com.github.stefanbirkner:system-rules:1.19.0",
+  "uk.org.webcompere:system-stubs-jupiter:2.0.1",
   "com.uber.nullaway:nullaway:0.9.5",
   "commons-beanutils:commons-beanutils:1.9.4",
   "commons-cli:commons-cli:1.5.0",

instrumentation-api/src/jmh/java/io/opentelemetry/instrumentation/api/cache/internal/CacheBenchmark.java renamed to instrumentation-api/src/jmh/java/io/opentelemetry/instrumentation/api/cache/CacheBenchmark.java

@@ -3,9 +3,8 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package io.opentelemetry.instrumentation.api.cache.internal;
+package io.opentelemetry.instrumentation.api.cache;
 
-import io.opentelemetry.instrumentation.api.cache.Cache;
 import java.util.concurrent.TimeUnit;
 import org.openjdk.jmh.annotations.Benchmark;
 import org.openjdk.jmh.annotations.Fork;

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/cache/internal/weaklockfree/WeakConcurrentMap.java

@@ -40,6 +40,9 @@
  *
  * <p>This class has been copied as is from
  * https://github.com/raphw/weak-lock-free/blob/ad0e5e0c04d4a31f9485bf12b89afbc9d75473b3/src/main/java/com/blogspot/mydailyjava/weaklockfree/WeakConcurrentMap.java
+ *
+ * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+ * at any time.
  */
 // Suppress warnings since this is copied as-is.
 @SuppressWarnings({
@@ -196,6 +199,9 @@ public int hashCode() {
   /**
    * A {@link WeakConcurrentMap} where stale entries are removed as a side effect of interacting
    * with this map.
+   *
+   * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+   * at any time.
    */
   public static class WithInlinedExpunction<K, V> extends WeakConcurrentMap<K, V> {
 

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/AsyncInstrumentRegistry.java

@@ -24,6 +24,10 @@
 // OTel SDK in the agent, and therefore there must be only one AsyncInstrumentRegistry too -
 // otherwise some async metrics would get lost because of duplicate instrument registrations.
 
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
 // TODO: refactor this class, there's too much copy-paste here
 public final class AsyncInstrumentRegistry {
 
@@ -251,6 +255,10 @@ private LongMeasurementSource(@Nullable T obj, ToLongFunction<T> metricFunction)
     }
   }
 
+  /**
+   * 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 AsyncMeasurementHandle {
 
     private final MeasurementsRecorder<?> measurementsRecorder;