Add close method to MetricReader (#5109)

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

@@ -4,9 +4,9 @@ import japicmp.model.JApiCompatibility
 import japicmp.model.JApiCompatibilityChange
 import japicmp.model.JApiMethod
 import me.champeau.gradle.japicmp.JapicmpTask
-import me.champeau.gradle.japicmp.report.Severity
 import me.champeau.gradle.japicmp.report.Violation
 import me.champeau.gradle.japicmp.report.stdrules.AbstractRecordingSeenMembers
+import me.champeau.gradle.japicmp.report.stdrules.BinaryIncompatibleRule
 import me.champeau.gradle.japicmp.report.stdrules.RecordSeenMembersSetup
 import me.champeau.gradle.japicmp.report.stdrules.SourceCompatibleRule
 import me.champeau.gradle.japicmp.report.stdrules.UnchangedMemberRule
@@ -32,32 +32,24 @@ val latestReleasedVersion: String by lazy {
   moduleVersion
 }
 
-class AllowDefaultMethodRule : AbstractRecordingSeenMembers() {
+class AllowNewAbstractMethodOnAutovalueClasses : AbstractRecordingSeenMembers() {
   override fun maybeAddViolation(member: JApiCompatibility): Violation? {
-    for (change in member.compatibilityChanges) {
-      if (isAbstractMethodOnAutoValue(member, change)) {
-        continue
-      }
-      if (!change.isSourceCompatible) {
-        return Violation.error(member, "Not source compatible")
-      }
-      if (!change.isBinaryCompatible) {
-        return Violation.notBinaryCompatible(member, Severity.error)
-      }
+    if (member.compatibilityChanges == listOf(JApiCompatibilityChange.METHOD_ABSTRACT_ADDED_TO_CLASS) &&
+      member is JApiMethod &&
+      member.getjApiClass().newClass.get().getAnnotation(AutoValue::class.java) != null
+    ) {
+      return Violation.accept(member, "Autovalue will automatically add implementation")
     }
     return null
   }
+}
 
-  /**
-   * Checks if the change is an abstract method on a class annotated with AutoValue.
-   * AutoValues need to override default interface methods and declare them abstract again.
-   * It causes METHOD_ABSTRACT_ADDED_TO_CLASS - source-incompatible change. It's
-   * false-positive since AutoValue will generate implementation anyway.
-   */
-  fun isAbstractMethodOnAutoValue(member: JApiCompatibility, change: JApiCompatibilityChange): Boolean {
-    return change == JApiCompatibilityChange.METHOD_ABSTRACT_ADDED_TO_CLASS &&
-      member is JApiMethod &&
-      member.getjApiClass().newClass.get().getAnnotation(AutoValue::class.java) != null
+class SourceIncompatibleRule : AbstractRecordingSeenMembers() {
+  override fun maybeAddViolation(member: JApiCompatibility): Violation? {
+    if (!member.isSourceCompatible()) {
+      return Violation.error(member, "Not source compatible")
+    }
+    return null
   }
 }
 
@@ -114,17 +106,26 @@ if (!project.hasProperty("otel.release") && !project.name.startsWith("bom")) {
         )
 
         // Reproduce defaults from https://github.com/melix/japicmp-gradle-plugin/blob/09f52739ef1fccda6b4310cf3f4b19dc97377024/src/main/java/me/champeau/gradle/japicmp/report/ViolationsGenerator.java#L130
-        // but allow new default methods on interfaces, adding default implementations to
-        // interface methods previously abstract, and select additional customizations defined in
-        // AllowDefaultMethodRule.
-        compatibilityChangeExcludes.set(listOf("METHOD_NEW_DEFAULT", "METHOD_ABSTRACT_NOW_DEFAULT"))
+        // with some changes.
+        val exclusions = mutableListOf<String>()
+        // Allow new default methods on interfaces
+        exclusions.add("METHOD_NEW_DEFAULT")
+        // Allow adding default implementations for default methods
+        exclusions.add("METHOD_ABSTRACT_NOW_DEFAULT")
+        // Bug prevents recognizing default methods of superinterface.
+        // Fixed in https://github.com/siom79/japicmp/pull/343 but not yet available in me.champeau.gradle.japicmp
+        exclusions.add("METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE")
+        compatibilityChangeExcludes.set(exclusions)
         richReport {
           addSetupRule(RecordSeenMembersSetup::class.java)
           addRule(JApiChangeStatus.NEW, SourceCompatibleRule::class.java)
           addRule(JApiChangeStatus.MODIFIED, SourceCompatibleRule::class.java)
           addRule(JApiChangeStatus.UNCHANGED, UnchangedMemberRule::class.java)
-          addRule(AllowDefaultMethodRule::class.java)
-          addRule(SourceCompatibleRule::class.java)
+          // Allow new abstract methods on autovalue
+          addRule(AllowNewAbstractMethodOnAutovalueClasses::class.java)
+          addRule(BinaryIncompatibleRule::class.java)
+          // Disallow source incompatible changes, which are allowed by default for some reason
+          addRule(SourceIncompatibleRule::class.java)
         }
 
         // this is needed so that we only consider the current artifact, and not dependencies

docs/apidiffs/current_vs_latest/opentelemetry-sdk-metrics.txt

@@ -1,2 +1,11 @@
 Comparing source compatibility of  against 
-No changes.
+***  MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.sdk.metrics.export.MetricReader  (not serializable)
+	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+	+++  NEW INTERFACE: java.io.Closeable
+	+++  NEW INTERFACE: java.lang.AutoCloseable
+	+++  NEW METHOD: PUBLIC(+) void close()
+		+++  NEW EXCEPTION: java.io.IOException
+***  MODIFIED CLASS: PUBLIC FINAL io.opentelemetry.sdk.metrics.export.PeriodicMetricReader  (not serializable)
+	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+	+++  NEW INTERFACE: java.io.Closeable
+	+++  NEW INTERFACE: java.lang.AutoCloseable

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java

@@ -23,7 +23,6 @@ import io.opentelemetry.sdk.metrics.data.MetricData;
 import io.opentelemetry.sdk.metrics.export.CollectionRegistration;
 import io.opentelemetry.sdk.metrics.export.MetricReader;
 import io.opentelemetry.sdk.metrics.internal.export.MetricProducer;
-import java.io.Closeable;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.io.UncheckedIOException;
@@ -53,7 +52,7 @@ import javax.annotation.Nullable;
  */
 // Very similar to
 // https://github.com/prometheus/client_java/blob/master/simpleclient_httpserver/src/main/java/io/prometheus/client/exporter/HTTPServer.java
-public final class PrometheusHttpServer implements Closeable, MetricReader {
+public final class PrometheusHttpServer implements MetricReader {
 
   private static final DaemonThreadFactory THREAD_FACTORY =
       new DaemonThreadFactory("prometheus-http");

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricReader.java

@@ -9,6 +9,9 @@ import io.opentelemetry.sdk.common.CompletableResultCode;
 import io.opentelemetry.sdk.metrics.Aggregation;
 import io.opentelemetry.sdk.metrics.InstrumentType;
 import io.opentelemetry.sdk.metrics.SdkMeterProvider;
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.concurrent.TimeUnit;
 
 /**
  * A metric reader reads metrics from an {@link SdkMeterProvider}.
@@ -18,7 +21,8 @@ import io.opentelemetry.sdk.metrics.SdkMeterProvider;
  *
  * @since 1.14.0
  */
-public interface MetricReader extends AggregationTemporalitySelector, DefaultAggregationSelector {
+public interface MetricReader
+    extends AggregationTemporalitySelector, DefaultAggregationSelector, Closeable {
 
   /**
    * Called by {@link SdkMeterProvider} and supplies the {@link MetricReader} with a handle to
@@ -63,4 +67,10 @@ public interface MetricReader extends AggregationTemporalitySelector, DefaultAgg
    * @return the result of the shutdown.
    */
   CompletableResultCode shutdown();
+
+  /** Close this {@link MetricReader}, releasing any resources. */
+  @Override
+  default void close() throws IOException {
+    shutdown().join(10, TimeUnit.SECONDS);
+  }
 }

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/export/PeriodicMetricReaderTest.java

@@ -10,6 +10,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -27,6 +28,7 @@ import io.opentelemetry.sdk.metrics.internal.data.ImmutableMetricData;
 import io.opentelemetry.sdk.metrics.internal.data.ImmutableSumData;
 import io.opentelemetry.sdk.metrics.internal.export.MetricProducer;
 import io.opentelemetry.sdk.resources.Resource;
+import java.io.IOException;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -172,14 +174,13 @@ class PeriodicMetricReaderTest {
   }
 
   @Test
-  void oneLastExportAfterShutdown() throws Exception {
+  void shutdown_ExportsOneLastTime() throws Exception {
     WaitingMetricExporter waitingMetricExporter = new WaitingMetricExporter();
     PeriodicMetricReader reader =
         PeriodicMetricReader.builder(waitingMetricExporter)
-            .setInterval(Duration.ofSeconds(100))
+            .setInterval(Duration.ofSeconds(Integer.MAX_VALUE))
             .build();
     reader.register(metricProducer);
-    // Assume that this will be called in less than 100 seconds.
     reader.shutdown();
 
     // This export was called during shutdown.
@@ -189,6 +190,19 @@ class PeriodicMetricReaderTest {
     assertThat(waitingMetricExporter.hasShutdown.get()).isTrue();
   }
 
+  @Test
+  void close_CallsShutdown() throws IOException {
+    PeriodicMetricReader reader =
+        spy(
+            PeriodicMetricReader.builder(new WaitingMetricExporter())
+                .setInterval(Duration.ofSeconds(Integer.MAX_VALUE))
+                .build());
+    reader.register(metricProducer);
+    reader.close();
+
+    verify(reader, times(1)).shutdown();
+  }
+
   @Test
   @SuppressWarnings("PreferJavaTimeOverload") // Testing the overload
   void invalidConfig() {