From 1f3bae9d3f9ed8a182f5f560582abba66630a03d Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Tue, 10 Jan 2023 15:46:47 -0600 Subject: [PATCH] Add close method to MetricReader --- .../otel.japicmp-conventions.gradle.kts | 57 ++++++++++--------- .../opentelemetry-sdk-metrics.txt | 11 +++- .../prometheus/PrometheusHttpServer.java | 3 +- .../sdk/metrics/export/MetricReader.java | 12 +++- .../export/PeriodicMetricReaderTest.java | 20 ++++++- 5 files changed, 68 insertions(+), 35 deletions(-) diff --git a/buildSrc/src/main/kotlin/otel.japicmp-conventions.gradle.kts b/buildSrc/src/main/kotlin/otel.japicmp-conventions.gradle.kts index 65671f7728f..8a42404f765 100644 --- a/buildSrc/src/main/kotlin/otel.japicmp-conventions.gradle.kts +++ b/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() + // 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 diff --git a/docs/apidiffs/current_vs_latest/opentelemetry-sdk-metrics.txt b/docs/apidiffs/current_vs_latest/opentelemetry-sdk-metrics.txt index df26146497b..9fa38661470 100644 --- a/docs/apidiffs/current_vs_latest/opentelemetry-sdk-metrics.txt +++ b/docs/apidiffs/current_vs_latest/opentelemetry-sdk-metrics.txt @@ -1,2 +1,11 @@ Comparing source compatibility of against -No changes. \ No newline at end of file +*** 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 diff --git a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java index 3e0eafe9670..6e49e4a47b4 100644 --- a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java +++ b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java @@ -23,7 +23,6 @@ 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 @@ */ // 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"); diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricReader.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricReader.java index 0bde34853e6..3752ba2640c 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricReader.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricReader.java @@ -9,6 +9,9 @@ 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 @@ * * @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 @@ default Aggregation getDefaultAggregation(InstrumentType instrumentType) { * @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); + } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/export/PeriodicMetricReaderTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/export/PeriodicMetricReaderTest.java index 6f12941f867..09a806237e2 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/export/PeriodicMetricReaderTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/export/PeriodicMetricReaderTest.java @@ -10,6 +10,7 @@ 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.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 @@ public void intervalExport_exporterThrowsException() throws Exception { } @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 @@ void oneLastExportAfterShutdown() throws Exception { 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() {