From 1ecf4933326c6c5c3884d427af837117d1ca18fb Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 19 Jan 2022 10:43:50 +0100 Subject: [PATCH] Fix flaky micrometer retries in javaagent test (#5168) * Fix flaky micrometer retries in javaagent test * remove comment * add clarifying comment --- .../internal}/AsyncInstrumentRegistry.java | 60 +++++++++---------- .../v1_5/OpenTelemetryFunctionCounter.java | 8 ++- .../v1_5/OpenTelemetryFunctionTimer.java | 3 +- .../micrometer/v1_5/OpenTelemetryGauge.java | 8 ++- .../micrometer/v1_5/OpenTelemetryMeter.java | 3 +- .../v1_5/OpenTelemetryMeterRegistry.java | 11 +--- 6 files changed, 48 insertions(+), 45 deletions(-) rename {instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5 => instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal}/AsyncInstrumentRegistry.java (82%) diff --git a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/AsyncInstrumentRegistry.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/AsyncInstrumentRegistry.java similarity index 82% rename from instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/AsyncInstrumentRegistry.java rename to instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/AsyncInstrumentRegistry.java index 60edc4ff84..aece0acacb 100644 --- a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/AsyncInstrumentRegistry.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/AsyncInstrumentRegistry.java @@ -3,15 +3,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.instrumentation.micrometer.v1_5; - -import static io.opentelemetry.instrumentation.micrometer.v1_5.Bridging.baseUnit; -import static io.opentelemetry.instrumentation.micrometer.v1_5.Bridging.description; +package io.opentelemetry.instrumentation.api.internal; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.metrics.Meter; import io.opentelemetry.api.metrics.ObservableDoubleMeasurement; import io.opentelemetry.api.metrics.ObservableLongMeasurement; +import io.opentelemetry.instrumentation.api.cache.Cache; import java.lang.ref.WeakReference; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -20,15 +18,35 @@ import java.util.function.ToDoubleFunction; import java.util.function.ToLongFunction; import javax.annotation.Nullable; +// this class must live in the bootstrap classloader - in case different metrics instrumentations +// (micrometer) get applied to classes in different classloaders, we want them to share the same +// async instrument registry - because the underlying OTel Meter is shared too. There is only one +// 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. + // TODO: refactor this class, there's too much copy-paste here -final class AsyncInstrumentRegistry { +public final class AsyncInstrumentRegistry { + + // we need to re-use instrument registries per OpenTelemetry instance so that async instruments + // that were created by other OpenTelemetryMeterRegistries can be reused; otherwise the SDK will + // start logging errors and async measurements will not be recorded + private static final Cache asyncInstrumentRegistries = + Cache.weak(); + + /** + * Returns the {@link AsyncInstrumentRegistry} for the passed {@link Meter}. There is at most one + * {@link AsyncInstrumentRegistry} created for each OpenTelemetry {@link Meter}. + */ + public static AsyncInstrumentRegistry getOrCreate(Meter meter) { + return asyncInstrumentRegistries.computeIfAbsent(meter, AsyncInstrumentRegistry::new); + } // using a weak ref so that the AsyncInstrumentRegistry (which is stored in a static maps) does // not hold strong references to Meter (and thus make it impossible to collect Meter garbage). // in practice this should never return null - OpenTelemetryMeterRegistry maintains a strong // reference to both Meter and AsyncInstrumentRegistry; if the meter registry is GC'd then its // corresponding AsyncInstrumentRegistry cannot possibly be used; and Meter cannot be GC'd until - // OpentelemetryMeterRegistry is GC'd + // OpenTelemetryMeterRegistry is GC'd private final WeakReference meter; // values from the maps below are never removed - that is because the underlying OpenTelemetry @@ -47,16 +65,7 @@ final class AsyncInstrumentRegistry { this.meter = new WeakReference<>(meter); } - AsyncMeasurementHandle buildGauge( - io.micrometer.core.instrument.Meter.Id meterId, - Attributes attributes, - @Nullable T obj, - ToDoubleFunction objMetric) { - return buildGauge( - meterId.getName(), description(meterId), baseUnit(meterId), attributes, obj, objMetric); - } - - AsyncMeasurementHandle buildGauge( + public AsyncMeasurementHandle buildGauge( String name, String description, String baseUnit, @@ -81,16 +90,7 @@ final class AsyncInstrumentRegistry { return new AsyncMeasurementHandle(recorder, attributes); } - AsyncMeasurementHandle buildDoubleCounter( - io.micrometer.core.instrument.Meter.Id meterId, - Attributes attributes, - T obj, - ToDoubleFunction objMetric) { - return buildDoubleCounter( - meterId.getName(), description(meterId), baseUnit(meterId), attributes, obj, objMetric); - } - - AsyncMeasurementHandle buildDoubleCounter( + public AsyncMeasurementHandle buildDoubleCounter( String name, String description, String baseUnit, @@ -116,7 +116,7 @@ final class AsyncInstrumentRegistry { return new AsyncMeasurementHandle(recorder, attributes); } - AsyncMeasurementHandle buildLongCounter( + public AsyncMeasurementHandle buildLongCounter( String name, String description, String baseUnit, @@ -141,7 +141,7 @@ final class AsyncInstrumentRegistry { return new AsyncMeasurementHandle(recorder, attributes); } - AsyncMeasurementHandle buildUpDownDoubleCounter( + public AsyncMeasurementHandle buildUpDownDoubleCounter( String name, String description, String baseUnit, @@ -251,7 +251,7 @@ final class AsyncInstrumentRegistry { } } - static final class AsyncMeasurementHandle { + public static final class AsyncMeasurementHandle { private final MeasurementsRecorder measurementsRecorder; private final Attributes attributes; @@ -261,7 +261,7 @@ final class AsyncInstrumentRegistry { this.attributes = attributes; } - void remove() { + public void remove() { measurementsRecorder.removeMeasurement(attributes); } } diff --git a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryFunctionCounter.java b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryFunctionCounter.java index 734462ff01..c7d88b82eb 100644 --- a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryFunctionCounter.java +++ b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryFunctionCounter.java @@ -5,12 +5,15 @@ package io.opentelemetry.instrumentation.micrometer.v1_5; +import static io.opentelemetry.instrumentation.micrometer.v1_5.Bridging.baseUnit; +import static io.opentelemetry.instrumentation.micrometer.v1_5.Bridging.description; import static io.opentelemetry.instrumentation.micrometer.v1_5.Bridging.tagsAsAttributes; import io.micrometer.core.instrument.FunctionCounter; import io.micrometer.core.instrument.Measurement; import io.micrometer.core.instrument.util.MeterEquivalence; -import io.opentelemetry.instrumentation.micrometer.v1_5.AsyncInstrumentRegistry.AsyncMeasurementHandle; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry.AsyncMeasurementHandle; import java.util.Collections; import java.util.function.ToDoubleFunction; import javax.annotation.Nullable; @@ -29,7 +32,8 @@ final class OpenTelemetryFunctionCounter implements FunctionCounter, Removabl this.id = id; countMeasurementHandle = - asyncInstrumentRegistry.buildDoubleCounter(id, tagsAsAttributes(id), obj, countFunction); + asyncInstrumentRegistry.buildDoubleCounter( + id.getName(), description(id), baseUnit(id), tagsAsAttributes(id), obj, countFunction); } @Override diff --git a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryFunctionTimer.java b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryFunctionTimer.java index 6d05e85e84..97e904ea83 100644 --- a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryFunctionTimer.java +++ b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryFunctionTimer.java @@ -15,7 +15,8 @@ import io.micrometer.core.instrument.Statistic; import io.micrometer.core.instrument.util.MeterEquivalence; import io.micrometer.core.instrument.util.TimeUtils; import io.opentelemetry.api.common.Attributes; -import io.opentelemetry.instrumentation.micrometer.v1_5.AsyncInstrumentRegistry.AsyncMeasurementHandle; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry.AsyncMeasurementHandle; import java.util.Collections; import java.util.concurrent.TimeUnit; import java.util.function.ToDoubleFunction; diff --git a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryGauge.java b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryGauge.java index 9fde5f9ba5..bd94792db9 100644 --- a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryGauge.java +++ b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryGauge.java @@ -5,12 +5,15 @@ package io.opentelemetry.instrumentation.micrometer.v1_5; +import static io.opentelemetry.instrumentation.micrometer.v1_5.Bridging.baseUnit; +import static io.opentelemetry.instrumentation.micrometer.v1_5.Bridging.description; import static io.opentelemetry.instrumentation.micrometer.v1_5.Bridging.tagsAsAttributes; import io.micrometer.core.instrument.Gauge; import io.micrometer.core.instrument.Measurement; import io.micrometer.core.instrument.util.MeterEquivalence; -import io.opentelemetry.instrumentation.micrometer.v1_5.AsyncInstrumentRegistry.AsyncMeasurementHandle; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry.AsyncMeasurementHandle; import java.util.Collections; import java.util.function.ToDoubleFunction; import javax.annotation.Nullable; @@ -29,7 +32,8 @@ final class OpenTelemetryGauge implements Gauge, RemovableMeter { this.id = id; gaugeMeasurementHandle = - asyncInstrumentRegistry.buildGauge(id, tagsAsAttributes(id), obj, objMetric); + asyncInstrumentRegistry.buildGauge( + id.getName(), description(id), baseUnit(id), tagsAsAttributes(id), obj, objMetric); } @Override diff --git a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeter.java b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeter.java index 96004d9585..9a7491f95d 100644 --- a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeter.java +++ b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeter.java @@ -14,7 +14,8 @@ import io.micrometer.core.instrument.Measurement; import io.micrometer.core.instrument.Meter; import io.micrometer.core.instrument.util.MeterEquivalence; import io.opentelemetry.api.common.Attributes; -import io.opentelemetry.instrumentation.micrometer.v1_5.AsyncInstrumentRegistry.AsyncMeasurementHandle; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry.AsyncMeasurementHandle; import java.util.ArrayList; import java.util.Collections; import java.util.List; diff --git a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeterRegistry.java b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeterRegistry.java index 908c86eeca..532558ba22 100644 --- a/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeterRegistry.java +++ b/instrumentation/micrometer/micrometer-1.5/library/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/OpenTelemetryMeterRegistry.java @@ -20,7 +20,7 @@ import io.micrometer.core.instrument.distribution.DistributionStatisticConfig; import io.micrometer.core.instrument.distribution.HistogramGauges; import io.micrometer.core.instrument.distribution.pause.PauseDetector; import io.opentelemetry.api.OpenTelemetry; -import io.opentelemetry.instrumentation.api.cache.Cache; +import io.opentelemetry.instrumentation.api.internal.AsyncInstrumentRegistry; import java.util.concurrent.TimeUnit; import java.util.function.ToDoubleFunction; import java.util.function.ToLongFunction; @@ -49,20 +49,13 @@ public final class OpenTelemetryMeterRegistry extends MeterRegistry { return new OpenTelemetryMeterRegistryBuilder(openTelemetry); } - // we need to re-use instrument registries per OpenTelemetry instance so that async instruments - // that were created by other OpenTelemetryMeterRegistries can be reused; otherwise the SDK will - // start logging errors and async measurements will not be recorded - private static final Cache - asyncInstrumentRegistries = Cache.weak(); - private final io.opentelemetry.api.metrics.Meter otelMeter; private final AsyncInstrumentRegistry asyncInstrumentRegistry; OpenTelemetryMeterRegistry(Clock clock, io.opentelemetry.api.metrics.Meter otelMeter) { super(clock); this.otelMeter = otelMeter; - this.asyncInstrumentRegistry = - asyncInstrumentRegistries.computeIfAbsent(otelMeter, AsyncInstrumentRegistry::new); + this.asyncInstrumentRegistry = AsyncInstrumentRegistry.getOrCreate(otelMeter); this.config().onMeterRemoved(OpenTelemetryMeterRegistry::onMeterRemoved); }