Skip to content

Commit

Permalink
Reverts enriching of CLientInterceptor to achieve observability
Browse files Browse the repository at this point in the history
  • Loading branch information
@marcingrzejszczak
marcingrzejszczak committed Sep 28, 2022
1 parent d9748a1 commit 61c49ab
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 49 deletions.
21 changes: 1 addition & 20 deletions micrometer/src/main/java/feign/micrometer/MicrometerCapability.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -26,42 +24,25 @@
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
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<Object> enrich(AsyncClient<Object> client) {
return new MeteredAsyncClient(client, meterRegistry);
Expand Down
56 changes: 32 additions & 24 deletions micrometer/src/test/java/feign/micrometer/AbstractMetricsTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand All @@ -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();

Expand Down Expand Up @@ -152,14 +152,14 @@ public void clientPropagatesUncheckedException() {
final AtomicReference<FeignException.NotFound> 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");
Expand All @@ -180,15 +180,15 @@ public void decoderPropagatesUncheckedException() {
final AtomicReference<FeignException.NotFound> 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"));
Expand All @@ -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"));
Expand All @@ -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");

Expand All @@ -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");

Expand All @@ -272,15 +272,15 @@ public void decoderExceptionCounterHasUriLabelWithPathExpression() {
final AtomicReference<FeignException.NotFound> 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"));
Expand Down Expand Up @@ -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 <C> AsyncFeign.AsyncBuilder<C> customizeBuilder(AsyncFeign.AsyncBuilder<C> builder) {
return builder;
}
}
3 changes: 1 addition & 2 deletions micrometer/src/test/java/feign/micrometer/FeignHeaderInstrumentationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
20 changes: 17 additions & 3 deletions micrometer/src/test/java/feign/micrometer/MicrometerObservationRegistryCapabilityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <C> AsyncFeign.AsyncBuilder<C> customizeBuilder(AsyncFeign.AsyncBuilder<C> builder) {
return super.customizeBuilder(builder)
.clientInterceptor(new ObservedClientInterceptor(this.observationRegistry));
}
}

0 comments on commit 61c49ab

Please sign in to comment.