From e3106f0a618e4cf35a54ae1eb0ea5c7dd097024d Mon Sep 17 00:00:00 2001 From: JaroslawDembek Date: Fri, 26 Jan 2024 13:35:20 +0100 Subject: [PATCH 1/2] URI_TEMPLATE_ATTRIBUTE used when available with feature flag. Fixes gh-1302 --- .../LoadBalancerStatsAutoConfiguration.java | 6 ++- .../loadbalancer/stats/LoadBalancerTags.java | 29 +++++++++++---- .../MicrometerStatsLoadBalancerLifecycle.java | 18 ++++++--- ...ometerStatsLoadBalancerLifecycleTests.java | 37 ++++++++++++++++++- 4 files changed, 73 insertions(+), 17 deletions(-) diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java index 1e6092847..8ed90b612 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java @@ -18,6 +18,7 @@ import io.micrometer.core.instrument.MeterRegistry; +import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; @@ -38,8 +39,9 @@ public class LoadBalancerStatsAutoConfiguration { @Bean @ConditionalOnBean(MeterRegistry.class) - public MicrometerStatsLoadBalancerLifecycle micrometerStatsLifecycle(MeterRegistry meterRegistry) { - return new MicrometerStatsLoadBalancerLifecycle(meterRegistry); + public MicrometerStatsLoadBalancerLifecycle micrometerStatsLifecycle(MeterRegistry meterRegistry, + @Value("${spring.cloud.loadbalancer.stats.micrometer.use-uri-template-attribute: false}") boolean useUriTemplateAttribute) { + return new MicrometerStatsLoadBalancerLifecycle(meterRegistry, useUriTemplateAttribute); } } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java index d21672ce8..1e397edd2 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java @@ -25,22 +25,27 @@ import org.springframework.cloud.client.loadbalancer.RequestDataContext; import org.springframework.cloud.client.loadbalancer.ResponseData; import org.springframework.util.StringUtils; +import org.springframework.web.reactive.function.client.WebClient; /** * Utility class for building metrics tags for load-balanced calls. * * @author Olga Maciaszek-Sharma + * @author Jaroslaw Dembek * @since 3.0.0 */ final class LoadBalancerTags { static final String UNKNOWN = "UNKNOWN"; + static final String URI_TEMPLATE_ATTRIBUTE = WebClient.class.getName() + ".uriTemplate"; + private LoadBalancerTags() { throw new UnsupportedOperationException("Cannot instantiate utility class"); } - static Iterable buildSuccessRequestTags(CompletionContext completionContext) { + static Iterable buildSuccessRequestTags(CompletionContext completionContext, + boolean useUriTemplateAttribute) { ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); Tags tags = Tags.of(buildServiceInstanceTags(serviceInstance)); Object clientResponse = completionContext.getClientResponse(); @@ -48,7 +53,7 @@ static Iterable buildSuccessRequestTags(CompletionContext buildDiscardedRequestTags( - CompletionContext completionContext) { + static Iterable buildDiscardedRequestTags(CompletionContext completionContext, + boolean useUriTemplateAttribute) { if (completionContext.getLoadBalancerRequest().getContext() instanceof RequestDataContext) { RequestData requestData = ((RequestDataContext) completionContext.getLoadBalancerRequest().getContext()) .getClientRequest(); if (requestData != null) { return Tags.of(valueOrUnknown("method", requestData.getHttpMethod()), - valueOrUnknown("uri", getPath(requestData)), valueOrUnknown("serviceId", getHost(requestData))); + valueOrUnknown("uri", getPath(requestData, useUriTemplateAttribute)), + valueOrUnknown("serviceId", getHost(requestData))); } } return Tags.of(valueOrUnknown("method", UNKNOWN), valueOrUnknown("uri", UNKNOWN), @@ -92,7 +104,8 @@ private static String getHost(RequestData requestData) { return requestData.getUrl() != null ? requestData.getUrl().getHost() : UNKNOWN; } - static Iterable buildFailedRequestTags(CompletionContext completionContext) { + static Iterable buildFailedRequestTags(CompletionContext completionContext, + boolean useUriTemplateAttribute) { ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); Tags tags = Tags.of(buildServiceInstanceTags(serviceInstance)).and(exception(completionContext.getThrowable())); if (completionContext.getLoadBalancerRequest().getContext() instanceof RequestDataContext) { @@ -100,7 +113,7 @@ static Iterable buildFailedRequestTags(CompletionContext { private final MeterRegistry meterRegistry; + private final boolean useUriTemplateAttribute; + private final ConcurrentHashMap activeRequestsPerInstance = new ConcurrentHashMap<>(); - public MicrometerStatsLoadBalancerLifecycle(MeterRegistry meterRegistry) { + public MicrometerStatsLoadBalancerLifecycle(MeterRegistry meterRegistry, boolean useUriTemplateAttribute) { this.meterRegistry = meterRegistry; + this.useUriTemplateAttribute = useUriTemplateAttribute; } @Override @@ -86,8 +90,9 @@ public void onStartRequest(Request request, Response lb public void onComplete(CompletionContext completionContext) { long requestFinishedTimestamp = System.nanoTime(); if (CompletionContext.Status.DISCARD.equals(completionContext.status())) { - Counter.builder("loadbalancer.requests.discard").tags(buildDiscardedRequestTags(completionContext)) - .register(meterRegistry).increment(); + Counter.builder("loadbalancer.requests.discard") + .tags(buildDiscardedRequestTags(completionContext, useUriTemplateAttribute)).register(meterRegistry) + .increment(); return; } ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); @@ -98,15 +103,16 @@ public void onComplete(CompletionContext comple Object loadBalancerRequestContext = completionContext.getLoadBalancerRequest().getContext(); if (requestHasBeenTimed(loadBalancerRequestContext)) { if (CompletionContext.Status.FAILED.equals(completionContext.status())) { - Timer.builder("loadbalancer.requests.failed").tags(buildFailedRequestTags(completionContext)) + Timer.builder("loadbalancer.requests.failed") + .tags(buildFailedRequestTags(completionContext, useUriTemplateAttribute)) .register(meterRegistry) .record(requestFinishedTimestamp - ((TimedRequestContext) loadBalancerRequestContext).getRequestStartTime(), TimeUnit.NANOSECONDS); return; } - Timer.builder("loadbalancer.requests.success").tags(buildSuccessRequestTags(completionContext)) - .register(meterRegistry) + Timer.builder("loadbalancer.requests.success") + .tags(buildSuccessRequestTags(completionContext, useUriTemplateAttribute)).register(meterRegistry) .record(requestFinishedTimestamp - ((TimedRequestContext) loadBalancerRequestContext).getRequestStartTime(), TimeUnit.NANOSECONDS); diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java index 49d8714c7..f9e20155c 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java @@ -18,6 +18,7 @@ import java.net.URI; import java.util.HashMap; +import java.util.Map; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Tag; @@ -43,17 +44,23 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.UNKNOWN; +import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.URI_TEMPLATE_ATTRIBUTE; /** * Tests for {@link MicrometerStatsLoadBalancerLifecycle}. * * @author Olga Maciaszek-Sharma + * @author Jaroslaw Dembek */ class MicrometerStatsLoadBalancerLifecycleTests { MeterRegistry meterRegistry = new SimpleMeterRegistry(); - MicrometerStatsLoadBalancerLifecycle statsLifecycle = new MicrometerStatsLoadBalancerLifecycle(meterRegistry); + MicrometerStatsLoadBalancerLifecycle statsLifecycle = new MicrometerStatsLoadBalancerLifecycle(meterRegistry, + false); + + MicrometerStatsLoadBalancerLifecycle statsLifecycleWithUriTemplateAttributeUse = new MicrometerStatsLoadBalancerLifecycle( + meterRegistry, true); @Test void shouldRecordSuccessfulTimedRequest() { @@ -80,6 +87,34 @@ void shouldRecordSuccessfulTimedRequest() { Tag.of("serviceInstance.port", "8080"), Tag.of("status", "200"), Tag.of("uri", "/test")); } + @Test + void shouldRecordSuccessfulTimedRequestWithUriTemplate() { + Map attributes = new HashMap<>(); + String uriTemplate = "/test/{pathParam}/test"; + attributes.put(URI_TEMPLATE_ATTRIBUTE, uriTemplate); + RequestData requestData = new RequestData(HttpMethod.GET, URI.create("http://test.org/test/123/test"), + new HttpHeaders(), new HttpHeaders(), attributes); + Request lbRequest = new DefaultRequest<>(new RequestDataContext(requestData)); + Response lbResponse = new DefaultResponse( + new DefaultServiceInstance("test-1", "test", "test.org", 8080, false, new HashMap<>())); + ResponseData responseData = new ResponseData(HttpStatus.OK, new HttpHeaders(), + new MultiValueMapAdapter<>(new HashMap<>()), requestData); + statsLifecycleWithUriTemplateAttributeUse.onStartRequest(lbRequest, lbResponse); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); + + statsLifecycleWithUriTemplateAttributeUse.onComplete( + new CompletionContext<>(CompletionContext.Status.SUCCESS, lbRequest, lbResponse, responseData)); + + assertThat(meterRegistry.getMeters()).hasSize(2); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0); + assertThat(meterRegistry.get("loadbalancer.requests.success").timers()).hasSize(1); + assertThat(meterRegistry.get("loadbalancer.requests.success").timer().count()).isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.success").timer().getId().getTags()).contains( + Tag.of("method", "GET"), Tag.of("outcome", "SUCCESS"), Tag.of("serviceId", "test"), + Tag.of("serviceInstance.host", "test.org"), Tag.of("serviceInstance.instanceId", "test-1"), + Tag.of("serviceInstance.port", "8080"), Tag.of("status", "200"), Tag.of("uri", uriTemplate)); + } + @Test void shouldRecordFailedTimedRequest() { RequestData requestData = new RequestData(HttpMethod.GET, URI.create("http://test.org/test"), new HttpHeaders(), From bf4bcb9fe9c64ad4748b0ed75b1911f0397533f8 Mon Sep 17 00:00:00 2001 From: JaroslawDembek Date: Wed, 31 Jan 2024 07:40:48 +0100 Subject: [PATCH 2/2] Revert usage of feature flag. --- .../LoadBalancerStatsAutoConfiguration.java | 6 ++---- .../loadbalancer/stats/LoadBalancerTags.java | 21 ++++++++----------- .../MicrometerStatsLoadBalancerLifecycle.java | 18 ++++++---------- ...ometerStatsLoadBalancerLifecycleTests.java | 10 +++------ 4 files changed, 20 insertions(+), 35 deletions(-) diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java index 8ed90b612..1e6092847 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java @@ -18,7 +18,6 @@ import io.micrometer.core.instrument.MeterRegistry; -import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; @@ -39,9 +38,8 @@ public class LoadBalancerStatsAutoConfiguration { @Bean @ConditionalOnBean(MeterRegistry.class) - public MicrometerStatsLoadBalancerLifecycle micrometerStatsLifecycle(MeterRegistry meterRegistry, - @Value("${spring.cloud.loadbalancer.stats.micrometer.use-uri-template-attribute: false}") boolean useUriTemplateAttribute) { - return new MicrometerStatsLoadBalancerLifecycle(meterRegistry, useUriTemplateAttribute); + public MicrometerStatsLoadBalancerLifecycle micrometerStatsLifecycle(MeterRegistry meterRegistry) { + return new MicrometerStatsLoadBalancerLifecycle(meterRegistry); } } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java index 1e397edd2..b72575952 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java @@ -44,8 +44,7 @@ private LoadBalancerTags() { throw new UnsupportedOperationException("Cannot instantiate utility class"); } - static Iterable buildSuccessRequestTags(CompletionContext completionContext, - boolean useUriTemplateAttribute) { + static Iterable buildSuccessRequestTags(CompletionContext completionContext) { ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); Tags tags = Tags.of(buildServiceInstanceTags(serviceInstance)); Object clientResponse = completionContext.getClientResponse(); @@ -53,7 +52,7 @@ static Iterable buildSuccessRequestTags(CompletionContext buildDiscardedRequestTags(CompletionContext completionContext, - boolean useUriTemplateAttribute) { + static Iterable buildDiscardedRequestTags( + CompletionContext completionContext) { if (completionContext.getLoadBalancerRequest().getContext() instanceof RequestDataContext) { RequestData requestData = ((RequestDataContext) completionContext.getLoadBalancerRequest().getContext()) .getClientRequest(); if (requestData != null) { return Tags.of(valueOrUnknown("method", requestData.getHttpMethod()), - valueOrUnknown("uri", getPath(requestData, useUriTemplateAttribute)), - valueOrUnknown("serviceId", getHost(requestData))); + valueOrUnknown("uri", getPath(requestData)), valueOrUnknown("serviceId", getHost(requestData))); } } return Tags.of(valueOrUnknown("method", UNKNOWN), valueOrUnknown("uri", UNKNOWN), @@ -104,8 +102,7 @@ private static String getHost(RequestData requestData) { return requestData.getUrl() != null ? requestData.getUrl().getHost() : UNKNOWN; } - static Iterable buildFailedRequestTags(CompletionContext completionContext, - boolean useUriTemplateAttribute) { + static Iterable buildFailedRequestTags(CompletionContext completionContext) { ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); Tags tags = Tags.of(buildServiceInstanceTags(serviceInstance)).and(exception(completionContext.getThrowable())); if (completionContext.getLoadBalancerRequest().getContext() instanceof RequestDataContext) { @@ -113,7 +110,7 @@ static Iterable buildFailedRequestTags(CompletionContext { private final MeterRegistry meterRegistry; - private final boolean useUriTemplateAttribute; - private final ConcurrentHashMap activeRequestsPerInstance = new ConcurrentHashMap<>(); - public MicrometerStatsLoadBalancerLifecycle(MeterRegistry meterRegistry, boolean useUriTemplateAttribute) { + public MicrometerStatsLoadBalancerLifecycle(MeterRegistry meterRegistry) { this.meterRegistry = meterRegistry; - this.useUriTemplateAttribute = useUriTemplateAttribute; } @Override @@ -90,9 +86,8 @@ public void onStartRequest(Request request, Response lb public void onComplete(CompletionContext completionContext) { long requestFinishedTimestamp = System.nanoTime(); if (CompletionContext.Status.DISCARD.equals(completionContext.status())) { - Counter.builder("loadbalancer.requests.discard") - .tags(buildDiscardedRequestTags(completionContext, useUriTemplateAttribute)).register(meterRegistry) - .increment(); + Counter.builder("loadbalancer.requests.discard").tags(buildDiscardedRequestTags(completionContext)) + .register(meterRegistry).increment(); return; } ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); @@ -103,16 +98,15 @@ public void onComplete(CompletionContext comple Object loadBalancerRequestContext = completionContext.getLoadBalancerRequest().getContext(); if (requestHasBeenTimed(loadBalancerRequestContext)) { if (CompletionContext.Status.FAILED.equals(completionContext.status())) { - Timer.builder("loadbalancer.requests.failed") - .tags(buildFailedRequestTags(completionContext, useUriTemplateAttribute)) + Timer.builder("loadbalancer.requests.failed").tags(buildFailedRequestTags(completionContext)) .register(meterRegistry) .record(requestFinishedTimestamp - ((TimedRequestContext) loadBalancerRequestContext).getRequestStartTime(), TimeUnit.NANOSECONDS); return; } - Timer.builder("loadbalancer.requests.success") - .tags(buildSuccessRequestTags(completionContext, useUriTemplateAttribute)).register(meterRegistry) + Timer.builder("loadbalancer.requests.success").tags(buildSuccessRequestTags(completionContext)) + .register(meterRegistry) .record(requestFinishedTimestamp - ((TimedRequestContext) loadBalancerRequestContext).getRequestStartTime(), TimeUnit.NANOSECONDS); diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java index f9e20155c..700aa7f92 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java @@ -56,11 +56,7 @@ class MicrometerStatsLoadBalancerLifecycleTests { MeterRegistry meterRegistry = new SimpleMeterRegistry(); - MicrometerStatsLoadBalancerLifecycle statsLifecycle = new MicrometerStatsLoadBalancerLifecycle(meterRegistry, - false); - - MicrometerStatsLoadBalancerLifecycle statsLifecycleWithUriTemplateAttributeUse = new MicrometerStatsLoadBalancerLifecycle( - meterRegistry, true); + MicrometerStatsLoadBalancerLifecycle statsLifecycle = new MicrometerStatsLoadBalancerLifecycle(meterRegistry); @Test void shouldRecordSuccessfulTimedRequest() { @@ -99,10 +95,10 @@ void shouldRecordSuccessfulTimedRequestWithUriTemplate() { new DefaultServiceInstance("test-1", "test", "test.org", 8080, false, new HashMap<>())); ResponseData responseData = new ResponseData(HttpStatus.OK, new HttpHeaders(), new MultiValueMapAdapter<>(new HashMap<>()), requestData); - statsLifecycleWithUriTemplateAttributeUse.onStartRequest(lbRequest, lbResponse); + statsLifecycle.onStartRequest(lbRequest, lbResponse); assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); - statsLifecycleWithUriTemplateAttributeUse.onComplete( + statsLifecycle.onComplete( new CompletionContext<>(CompletionContext.Status.SUCCESS, lbRequest, lbResponse, responseData)); assertThat(meterRegistry.getMeters()).hasSize(2);