Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Aggregate Micrometer "uri" label values under uri template #1493

Merged
merged 5 commits into from
Sep 1, 2021
Merged

Aggregate Micrometer "uri" label values under uri template #1493

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd2fa71
Use path expression rather than explicit value for Micrometer "uri" l…
martinacat Aug 26, 2021
bc6171c
Make assertions more self explanatory
martinacat Aug 26, 2021
a976caf
Test Micrometer Decoder uri label
martinacat Aug 26, 2021
c214eef
Merge branch 'master' into uri-label-value-as-expression
velo Sep 1, 2021
ecbccf1
Move tests to AbstractMetricsTestBase, add uri label to dropwizard-me…
martinacat Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions micrometer/src/main/java/feign/micrometer/MeteredClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,9 @@ protected void countResponseCode(Request request,
final RequestTemplate template = request.requestTemplate();
final Tags allTags = metricTagResolver
.tag(template.methodMetadata(), template.feignTarget(), e,
new Tag[] {Tag.of("http_status", String.valueOf(responseStatus)),
Tag.of("status_group", responseStatus / 100 + "xx"),
Tag.of("uri", template.path())})
Tag.of("http_status", String.valueOf(responseStatus)),
Tag.of("status_group", responseStatus / 100 + "xx"),
Tag.of("uri", template.methodMetadata().template().path()))
martinacat marked this conversation as resolved.
Show resolved Hide resolved
.and(extraTags);
meterRegistry.counter(
metricName.name("http_response_code"),
Expand All @@ -99,7 +99,8 @@ protected Timer createTimer(Request request,
Exception e) {
final RequestTemplate template = request.requestTemplate();
final Tags allTags = metricTagResolver
.tag(template.methodMetadata(), template.feignTarget(), e, Tag.of("uri", template.path()))
.tag(template.methodMetadata(), template.feignTarget(), e,
Tag.of("uri", template.methodMetadata().template().path()))
martinacat marked this conversation as resolved.
Show resolved Hide resolved
.and(extraTags(request, response, options, e));
return meterRegistry.timer(metricName.name(e), allTags);
}
Expand Down
2 changes: 1 addition & 1 deletion micrometer/src/main/java/feign/micrometer/MeteredDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ protected Counter createExceptionCounter(Response response, Type type, Exception
final Tag[] extraTags = extraTags(response, type, e);
final RequestTemplate template = response.request().requestTemplate();
final Tags allTags = metricTagResolver.tag(template.methodMetadata(), template.feignTarget(),
Tag.of("uri", template.path()),
Tag.of("uri", template.methodMetadata().template().path()),
martinacat marked this conversation as resolved.
Show resolved Hide resolved
Tag.of("exception_name", e.getClass().getSimpleName()))
.and(extraTags);
return meterRegistry.counter(metricName.name("error_count"), allTags);
Expand Down
60 changes: 58 additions & 2 deletions micrometer/src/test/java/feign/micrometer/MicrometerCapabilityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,66 @@
import java.util.Map.Entry;
import java.util.function.Function;
import java.util.stream.Collectors;
import feign.Capability;
import feign.Util;

import feign.*;
import feign.mock.HttpMethod;
import feign.mock.MockClient;
import feign.mock.MockTarget;
import io.micrometer.core.instrument.Meter;
import io.micrometer.core.instrument.Meter.Id;
import io.micrometer.core.instrument.MockClock;
import io.micrometer.core.instrument.simple.SimpleConfig;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import org.junit.Test;
import static org.hamcrest.MatcherAssert.assertThat;

public class MicrometerCapabilityTest
extends AbstractMetricsTestBase<SimpleMeterRegistry, Id, Meter> {


public interface SourceWithPathExpressions {

@RequestLine("GET /get/{id}")
String get(@Param("id") String id, String body);

}

@Test
public void clientMetricsHaveUriLabel() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this tests to the AbstractMetricsTestBase, likely this will fail for dropwizard-metrics which would need to be made consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - I originally placed it in AbstractMetricsTestBase, however I realised that dropwizard-metrics don't attach uri and thought that was a design decision. If necessary I can add uri on dropwizard 4 and 5 - is that what we would like? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the tests in AbstractMetricsTestBase and made dropwizard-metrics 4 & 5 consistent :)

final SimpleSource source = Feign.builder()
.client(new MockClient()
.ok(HttpMethod.GET, "/get", "1234567890abcde"))
.addCapability(createMetricCapability())
.target(new MockTarget<>(SimpleSource.class));

source.get("0x3456789");

getFeignMetrics().entrySet()
.stream()
.filter(this::isClientMetric)
.peek(e -> assertThat(
"Expect Client metric names to include uri:" + e,
doesMetricIncludeUri(e.getKey(), "/get")));
}

@Test
public void clientMetricsHaveUriLabelWithPathExpression() {
final SourceWithPathExpressions source = Feign.builder()
.client(new MockClient()
.ok(HttpMethod.GET, "/get/123", "1234567890abcde"))
.addCapability(createMetricCapability())
.target(new MockTarget<>(SourceWithPathExpressions.class));

source.get("123", "0x3456789");

getFeignMetrics().entrySet()
.stream()
.filter(this::isClientMetric)
.peek(e -> assertThat(
"Expect Client metric names to include uri with path expression:" + e,
doesMetricIncludeUri(e.getKey(), "/get/{id}")));
}

@Override
protected SimpleMeterRegistry createMetricsRegistry() {
return new SimpleMeterRegistry(SimpleConfig.DEFAULT, new MockClock());
Expand Down Expand Up @@ -97,5 +146,12 @@ protected Meter getMetric(String suffix, String... tags) {
.orElse(null);
}

private boolean isClientMetric(Map.Entry<Id, Meter> entry) {
return entry.getKey().getName().startsWith("feign.Client");
}

private boolean doesMetricIncludeUri(Id metricId, String uri) {
return uri.equals(metricId.getTag("uri"));
}

}