From 61c49ab422d2e101dd28441a0cf6dd60ab035893 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 28 Sep 2022 14:42:20 +0200 Subject: [PATCH] Reverts enriching of CLientInterceptor to achieve observability --- .../micrometer/MicrometerCapability.java | 21 +------ .../micrometer/AbstractMetricsTestBase.java | 56 +++++++++++-------- .../FeignHeaderInstrumentationTest.java | 3 +- ...eterObservationRegistryCapabilityTest.java | 20 ++++++- 4 files changed, 51 insertions(+), 49 deletions(-) diff --git a/micrometer/src/main/java/feign/micrometer/MicrometerCapability.java b/micrometer/src/main/java/feign/micrometer/MicrometerCapability.java index d2cc61cbf..e4f3d76a5 100644 --- a/micrometer/src/main/java/feign/micrometer/MicrometerCapability.java +++ b/micrometer/src/main/java/feign/micrometer/MicrometerCapability.java @@ -14,10 +14,8 @@ package feign.micrometer; import feign.AsyncClient; -import feign.BaseBuilder; import feign.Capability; import feign.Client; -import feign.ClientInterceptor; import feign.InvocationHandlerFactory; import feign.codec.Decoder; import feign.codec.Encoder; @@ -26,27 +24,18 @@ import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.simple.SimpleConfig; import io.micrometer.core.instrument.simple.SimpleMeterRegistry; -import io.micrometer.observation.ObservationRegistry; public class MicrometerCapability implements Capability { private final MeterRegistry meterRegistry; - private final ObservationRegistry observationRegistry; - public MicrometerCapability() { - this(new SimpleMeterRegistry(SimpleConfig.DEFAULT, Clock.SYSTEM), ObservationRegistry.NOOP); + this(new SimpleMeterRegistry(SimpleConfig.DEFAULT, Clock.SYSTEM)); Metrics.addRegistry(meterRegistry); } public MicrometerCapability(MeterRegistry meterRegistry) { - this(meterRegistry, ObservationRegistry.NOOP); - } - - public MicrometerCapability(MeterRegistry meterRegistry, - ObservationRegistry observationRegistry) { this.meterRegistry = meterRegistry; - this.observationRegistry = observationRegistry; } @Override @@ -54,14 +43,6 @@ public Client enrich(Client client) { return new MeteredClient(client, meterRegistry); } - @Override - public ClientInterceptor enrich(ClientInterceptor clientInterceptor) { - if (!this.observationRegistry.isNoop()) { - return new ObservedClientInterceptor(observationRegistry); - } - return clientInterceptor; - } - @Override public AsyncClient enrich(AsyncClient client) { return new MeteredAsyncClient(client, meterRegistry); diff --git a/micrometer/src/test/java/feign/micrometer/AbstractMetricsTestBase.java b/micrometer/src/test/java/feign/micrometer/AbstractMetricsTestBase.java index a109c829c..c515ddcab 100644 --- a/micrometer/src/test/java/feign/micrometer/AbstractMetricsTestBase.java +++ b/micrometer/src/test/java/feign/micrometer/AbstractMetricsTestBase.java @@ -63,10 +63,10 @@ public final void initializeMetricRegistry() { @Test public final void addMetricsCapability() { final SimpleSource source = - Feign.builder() + customizeBuilder(Feign.builder() .client(new MockClient().ok(HttpMethod.GET, "/get", "1234567890abcde")) - .addCapability(createMetricCapability()) - .target(new MockTarget<>(SimpleSource.class)); + .addCapability(createMetricCapability())) + .target(new MockTarget<>(SimpleSource.class)); source.get("0x3456789"); @@ -76,10 +76,10 @@ public final void addMetricsCapability() { @Test public final void addAsyncMetricsCapability() { final CompletableSource source = - AsyncFeign.builder() + customizeBuilder(AsyncFeign.builder() .client(new MockClient().ok(HttpMethod.GET, "/get", "1234567890abcde")) - .addCapability(createMetricCapability()) - .target(new MockTarget<>(CompletableSource.class)); + .addCapability(createMetricCapability())) + .target(new MockTarget<>(CompletableSource.class)); source.get("0x3456789").join(); @@ -152,14 +152,14 @@ public void clientPropagatesUncheckedException() { final AtomicReference notFound = new AtomicReference<>(); final SimpleSource source = - Feign.builder() + customizeBuilder(Feign.builder() .client( (request, options) -> { notFound.set(new FeignException.NotFound("test", request, null, null)); throw notFound.get(); }) - .addCapability(createMetricCapability()) - .target(new MockTarget<>(MicrometerCapabilityTest.SimpleSource.class)); + .addCapability(createMetricCapability())) + .target(new MockTarget<>(MicrometerCapabilityTest.SimpleSource.class)); try { source.get("0x3456789"); @@ -180,15 +180,15 @@ public void decoderPropagatesUncheckedException() { final AtomicReference notFound = new AtomicReference<>(); final SimpleSource source = - Feign.builder() + customizeBuilder(Feign.builder() .client(new MockClient().ok(HttpMethod.GET, "/get", "1234567890abcde")) .decoder( (response, type) -> { notFound.set(new FeignException.NotFound("test", response.request(), null, null)); throw notFound.get(); }) - .addCapability(createMetricCapability()) - .target(new MockTarget<>(MicrometerCapabilityTest.SimpleSource.class)); + .addCapability(createMetricCapability())) + .target(new MockTarget<>(MicrometerCapabilityTest.SimpleSource.class)); FeignException.NotFound thrown = assertThrows(FeignException.NotFound.class, () -> source.get("0x3456789")); @@ -198,13 +198,13 @@ public void decoderPropagatesUncheckedException() { @Test public void shouldMetricCollectionWithCustomException() { final SimpleSource source = - Feign.builder() + customizeBuilder(Feign.builder() .client( (request, options) -> { throw new RuntimeException("Test error"); }) - .addCapability(createMetricCapability()) - .target(new MockTarget<>(MicrometerCapabilityTest.SimpleSource.class)); + .addCapability(createMetricCapability())) + .target(new MockTarget<>(MicrometerCapabilityTest.SimpleSource.class)); RuntimeException thrown = assertThrows(RuntimeException.class, () -> source.get("0x3456789")); assertThat(thrown.getMessage(), equalTo("Test error")); @@ -217,10 +217,10 @@ public void shouldMetricCollectionWithCustomException() { @Test public void clientMetricsHaveUriLabel() { final SimpleSource source = - Feign.builder() + customizeBuilder(Feign.builder() .client(new MockClient().ok(HttpMethod.GET, "/get", "1234567890abcde")) - .addCapability(createMetricCapability()) - .target(new MockTarget<>(SimpleSource.class)); + .addCapability(createMetricCapability())) + .target(new MockTarget<>(SimpleSource.class)); source.get("0x3456789"); @@ -246,10 +246,10 @@ public interface SourceWithPathExpressions { @Test public void clientMetricsHaveUriLabelWithPathExpression() { final SourceWithPathExpressions source = - Feign.builder() + customizeBuilder(Feign.builder() .client(new MockClient().ok(HttpMethod.GET, "/get/123", "1234567890abcde")) - .addCapability(createMetricCapability()) - .target(new MockTarget<>(SourceWithPathExpressions.class)); + .addCapability(createMetricCapability())) + .target(new MockTarget<>(SourceWithPathExpressions.class)); source.get("123", "0x3456789"); @@ -272,15 +272,15 @@ public void decoderExceptionCounterHasUriLabelWithPathExpression() { final AtomicReference notFound = new AtomicReference<>(); final SourceWithPathExpressions source = - Feign.builder() + customizeBuilder(Feign.builder() .client(new MockClient().ok(HttpMethod.GET, "/get/123", "1234567890abcde")) .decoder( (response, type) -> { notFound.set(new FeignException.NotFound("test", response.request(), null, null)); throw notFound.get(); }) - .addCapability(createMetricCapability()) - .target(new MockTarget<>(MicrometerCapabilityTest.SourceWithPathExpressions.class)); + .addCapability(createMetricCapability())) + .target(new MockTarget<>(MicrometerCapabilityTest.SourceWithPathExpressions.class)); FeignException.NotFound thrown = assertThrows(FeignException.NotFound.class, () -> source.get("123", "0x3456789")); @@ -311,4 +311,12 @@ public void decoderExceptionCounterHasUriLabelWithPathExpression() { protected abstract boolean doesMetricHasCounter(METRIC metric); protected abstract long getMetricCounter(METRIC metric); + + protected Feign.Builder customizeBuilder(Feign.Builder builder) { + return builder; + } + + protected AsyncFeign.AsyncBuilder customizeBuilder(AsyncFeign.AsyncBuilder builder) { + return builder; + } } diff --git a/micrometer/src/test/java/feign/micrometer/FeignHeaderInstrumentationTest.java b/micrometer/src/test/java/feign/micrometer/FeignHeaderInstrumentationTest.java index df94785dc..ad8586faa 100644 --- a/micrometer/src/test/java/feign/micrometer/FeignHeaderInstrumentationTest.java +++ b/micrometer/src/test/java/feign/micrometer/FeignHeaderInstrumentationTest.java @@ -73,8 +73,7 @@ void getTemplatedPathForUri(WireMockRuntimeInfo wmRuntimeInfo) { private TestClient clientInstrumentedWithObservations(String url) { return Feign.builder() .client(new feign.okhttp.OkHttpClient(new OkHttpClient())) - .addCapability(new MicrometerCapability(meterRegistry, observationRegistry)) - .clientInterceptor(ClientInterceptor.DEFAULT) + .clientInterceptor(new ObservedClientInterceptor(this.observationRegistry)) .target(TestClient.class, url); } diff --git a/micrometer/src/test/java/feign/micrometer/MicrometerObservationRegistryCapabilityTest.java b/micrometer/src/test/java/feign/micrometer/MicrometerObservationRegistryCapabilityTest.java index ef6503bad..3b0e0aa19 100644 --- a/micrometer/src/test/java/feign/micrometer/MicrometerObservationRegistryCapabilityTest.java +++ b/micrometer/src/test/java/feign/micrometer/MicrometerObservationRegistryCapabilityTest.java @@ -13,18 +13,32 @@ */ package feign.micrometer; +import feign.AsyncFeign; import feign.Capability; +import feign.Feign; import io.micrometer.core.instrument.observation.DefaultMeterObservationHandler; import io.micrometer.observation.ObservationRegistry; +import org.junit.Before; public class MicrometerObservationRegistryCapabilityTest extends MicrometerCapabilityTest { ObservationRegistry observationRegistry = ObservationRegistry.create(); - @Override - protected Capability createMetricCapability() { + @Before + public void setupRegistry() { observationRegistry.observationConfig() .observationHandler(new DefaultMeterObservationHandler(metricsRegistry)); - return new MicrometerCapability(metricsRegistry, observationRegistry); + } + + @Override + protected Feign.Builder customizeBuilder(Feign.Builder builder) { + return super.customizeBuilder(builder) + .clientInterceptor(new ObservedClientInterceptor(this.observationRegistry)); + } + + @Override + protected AsyncFeign.AsyncBuilder customizeBuilder(AsyncFeign.AsyncBuilder builder) { + return super.customizeBuilder(builder) + .clientInterceptor(new ObservedClientInterceptor(this.observationRegistry)); } }