From 8ebe1fb54d1f9574e6a567f5f152a0f527c72c59 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 15 Dec 2020 19:58:56 +0100 Subject: [PATCH 01/19] Add stats lifecycle bean. Add onStartRequest method to LoadBalancerLifecycle. Add loadbalancer.Request field to CompletionContext. --- .../loadbalancer/CompletionContext.java | 30 +++-- .../loadbalancer/HintRequestContext.java | 11 ++ .../loadbalancer/LoadBalancerLifecycle.java | 11 +- ...torLoadBalancerExchangeFilterFunction.java | 23 ++-- spring-cloud-loadbalancer/pom.xml | 5 + .../client/BlockingLoadBalancerClient.java | 12 +- .../stats/MicrometerStatsLifecycle.java | 117 ++++++++++++++++++ 7 files changed, 186 insertions(+), 23 deletions(-) create mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java index 975e63a1e..8699e977c 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java @@ -16,6 +16,8 @@ package org.springframework.cloud.client.loadbalancer; +import java.util.UUID; + import org.springframework.core.style.ToStringCreator; /** @@ -25,7 +27,7 @@ * @author Olga Maciaszek-Sharma * @since 3.0.0 */ -public class CompletionContext { +public class CompletionContext { private final Status status; @@ -35,27 +37,31 @@ public class CompletionContext { private final RES clientResponse; - public CompletionContext(Status status) { - this(status, null, null, null); + private final Request loadBalancerRequest; + + public CompletionContext(Status status, Request loadBalancerRequest) { + this(status, null, null, null, loadBalancerRequest); } - public CompletionContext(Status status, Response response) { - this(status, null, response, null); + public CompletionContext(Status status, Response response, Request loadBalancerRequest) { + this(status, null, response, null, loadBalancerRequest); } - public CompletionContext(Status status, Throwable throwable, Response loadBalancerResponse) { - this(status, throwable, loadBalancerResponse, null); + public CompletionContext(Status status, Throwable throwable, Response loadBalancerResponse, Request loadBalancerRequest) { + this(status, throwable, loadBalancerResponse, null, loadBalancerRequest); } - public CompletionContext(Status status, Response loadBalancerResponse, RES clientResponse) { - this(status, null, loadBalancerResponse, clientResponse); + public CompletionContext(Status status, Response loadBalancerResponse, RES clientResponse,Request loadBalancerRequest) { + this(status, null, loadBalancerResponse, clientResponse, loadBalancerRequest); } - public CompletionContext(Status status, Throwable throwable, Response loadBalancerResponse, RES clientResponse) { + public CompletionContext(Status status, Throwable throwable, Response loadBalancerResponse, RES clientResponse, + Request loadBalancerRequest) { this.status = status; this.throwable = throwable; this.loadBalancerResponse = loadBalancerResponse; this.clientResponse = clientResponse; + this.loadBalancerRequest = loadBalancerRequest; } public Status status() { @@ -74,6 +80,10 @@ public RES getClientResponse() { return clientResponse; } + public Request getLoadBalancerRequest() { + return loadBalancerRequest; + } + @Override public String toString() { ToStringCreator to = new ToStringCreator(this); diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/HintRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/HintRequestContext.java index 0ef6b1cc1..348888d72 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/HintRequestContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/HintRequestContext.java @@ -17,6 +17,7 @@ package org.springframework.cloud.client.loadbalancer; import java.util.Objects; +import java.util.UUID; import org.springframework.core.style.ToStringCreator; @@ -33,6 +34,8 @@ public class HintRequestContext { */ private String hint = "default"; + private long requestStartTime; + public HintRequestContext() { } @@ -48,6 +51,14 @@ public void setHint(String hint) { this.hint = hint; } + public void setRequestStartTime(long requestStartTime) { + this.requestStartTime = requestStartTime; + } + + public long getRequestStartTimestamp() { + return requestStartTime; + } + @Override public String toString() { ToStringCreator to = new ToStringCreator(this); diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java index 4db50ed60..34ceb88cd 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java @@ -16,6 +16,8 @@ package org.springframework.cloud.client.loadbalancer; +import org.springframework.cloud.client.ServiceInstance; + /** * Allows to define actions that should be carried out before and after load-balancing. * @@ -45,11 +47,18 @@ default boolean supports(Class requestContextClass, Class responseClass, Class s */ void onStart(Request request); + /** + * A callback method executed after a service instance has been selected, before executing the actual load-balanced request. + * @param request the {@link Request} that has been used by the LoadBalancer to select a service instance + * @param lbResponse the {@link Response} returned by the LoadBalancer + */ + void onStartRequest(Request request, Response lbResponse); + /** * A callback method executed after load-balancing. * @param completionContext the {@link CompletionContext} containing data relevant to * the load-balancing and the response returned from the selected service instance */ - void onComplete(CompletionContext completionContext); + void onComplete(CompletionContext completionContext); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java index de703f933..699a42afe 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java @@ -94,26 +94,33 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction LOG.warn(message); } supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse))); + .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse, lbRequest))); return Mono.just(ClientResponse.create(HttpStatus.SERVICE_UNAVAILABLE) .body(serviceInstanceUnavailableMessage(serviceId)).build()); } if (LOG.isDebugEnabled()) { - LOG.debug(String.format("LoadBalancer has retrieved the instance for service %s: %s", serviceId, - instance.getUri())); + LOG.debug(String + .format("LoadBalancer has retrieved the instance for service %s: %s", serviceId, + instance.getUri())); } - LoadBalancerProperties.StickySession stickySessionProperties = properties.getStickySession(); + LoadBalancerProperties.StickySession stickySessionProperties = properties + .getStickySession(); ClientRequest newRequest = buildClientRequest(clientRequest, instance, stickySessionProperties.getInstanceIdCookieName(), stickySessionProperties.isAddServiceInstanceCookie()); + supportedLifecycleProcessors + .forEach(lifecycle -> lifecycle.onStartRequest( + lbRequest, lbResponse)); return next.exchange(newRequest) .doOnError(throwable -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle.onComplete(new CompletionContext( - CompletionContext.Status.FAILED, throwable, lbResponse)))) + lifecycle -> lifecycle + .onComplete(new CompletionContext( + CompletionContext.Status.FAILED, throwable, lbResponse, lbRequest)))) .doOnSuccess(clientResponse -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, - lbResponse, new ResponseData(clientResponse, requestData))))); + lifecycle -> lifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, + lbResponse, new ResponseData(clientResponse, requestData), lbRequest)))); }); } diff --git a/spring-cloud-loadbalancer/pom.xml b/spring-cloud-loadbalancer/pom.xml index 76529410d..675b40adc 100644 --- a/spring-cloud-loadbalancer/pom.xml +++ b/spring-cloud-loadbalancer/pom.xml @@ -82,6 +82,11 @@ spring-retry true + + io.micrometer + micrometer-core + true + org.springframework.boot spring-boot-starter-test diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java index 681318856..1092c3957 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java @@ -19,6 +19,7 @@ import java.io.IOException; import java.net.URI; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import reactor.core.publisher.Mono; @@ -75,7 +76,7 @@ public T execute(String serviceId, LoadBalancerRequest request) throws IO ServiceInstance serviceInstance = choose(serviceId, lbRequest); if (serviceInstance == null) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, new EmptyResponse()))); + .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, new EmptyResponse(), lbRequest))); throw new IllegalStateException("No instances available for " + serviceId); } return execute(serviceId, serviceInstance, request); @@ -89,20 +90,23 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala .getSupportedLifecycleProcessors( loadBalancerClientFactory.getInstances(serviceId, LoadBalancerLifecycle.class), DefaultRequestContext.class, Object.class, ServiceInstance.class); + // TODO: maybe better just pass start time? + Request fakeLbRequest = new DefaultRequest<>(); + fakeLbRequest.getContext().setRequestStartTime(System.nanoTime()); try { T response = request.apply(serviceInstance); supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, defaultResponse, response))); + .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, defaultResponse, response, fakeLbRequest))); return response; } catch (IOException iOException) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.FAILED, iOException, defaultResponse))); + new CompletionContext<>(CompletionContext.Status.FAILED, iOException, defaultResponse, fakeLbRequest ))); throw iOException; } catch (Exception exception) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.FAILED, exception, defaultResponse))); + .onComplete(new CompletionContext<>(CompletionContext.Status.FAILED, exception, defaultResponse, fakeLbRequest))); ReflectionUtils.rethrowRuntimeException(exception); } return null; diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java new file mode 100644 index 000000000..fd72ae650 --- /dev/null +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java @@ -0,0 +1,117 @@ +package org.springframework.cloud.loadbalancer.stats; + +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; + +import io.micrometer.core.instrument.Gauge; +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; +import io.micrometer.core.instrument.Timer; + +import org.springframework.boot.actuate.metrics.http.Outcome; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.CompletionContext; +import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; +import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.cloud.client.loadbalancer.RequestData; +import org.springframework.cloud.client.loadbalancer.RequestDataContext; +import org.springframework.cloud.client.loadbalancer.Response; +import org.springframework.cloud.client.loadbalancer.ResponseData; +import org.springframework.http.HttpStatus; +import org.springframework.util.StringUtils; + +/** + * @author Olga Maciaszek-Sharma + */ +public class MicrometerStatsLifecycle implements LoadBalancerLifecycle { + + private final ConcurrentHashMap activeRequestsPerInstance = new ConcurrentHashMap<>(); + + private final MeterRegistry meterRegistry; + + public MicrometerStatsLifecycle(MeterRegistry meterRegistry) { + this.meterRegistry = meterRegistry; + } + + private static Tag status(HttpStatus status) { + if (status == null) { + status = HttpStatus.OK; + } + return Tag.of("status", String.valueOf(status.value())); + } + + private static Tag exception(Throwable exception) { + if (exception != null) { + String simpleName = exception.getClass().getSimpleName(); + return Tag.of("exception", StringUtils + .hasText(simpleName) ? simpleName : exception.getClass().getName()); + } + return Tag.of("exception", "None"); + } + + + @Override + public boolean supports(Class requestContextClass, Class responseClass, Class serverTypeClass) { + throw new UnsupportedOperationException("Please, implement me."); + } + + @Override + public void onStart(Request request) { + + } + + @Override + public void onStartRequest(Request request, Response lbResponse) { + request.getContext().setRequestStartTime(System.nanoTime()); + if (!lbResponse.hasServer()) { + return; + } + ServiceInstance serviceInstance = lbResponse.getServer(); + AtomicLong activeRequestsCounter = activeRequestsPerInstance.get(serviceInstance); + if (activeRequestsCounter == null) { + activeRequestsPerInstance.put(serviceInstance, new AtomicLong()); + AtomicLong createdCounter = activeRequestsPerInstance.get(serviceInstance); + Gauge.builder("loadbalanced.requests.active", () -> createdCounter) + .tags(Tags.of(Tag.of("serviceId", serviceInstance.getServiceId()), + Tag.of("instanceId", serviceInstance.getInstanceId()))) + .register(meterRegistry); + createdCounter.incrementAndGet(); + } + } + + @Override + public void onComplete(CompletionContext completionContext) { + long requestFinishedTimestamp = System.nanoTime(); + if (CompletionContext.Status.DISCARD.equals(completionContext.status())) { + return; + } + ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse() + .getServer(); + AtomicLong activeRequestsCounter = activeRequestsPerInstance.get(serviceInstance); + if (activeRequestsCounter != null) { + activeRequestsCounter.decrementAndGet(); + } + Request loadBalancerRequest = completionContext + .getLoadBalancerRequest(); + RequestData requestData = loadBalancerRequest.getContext() + .getClientRequest(); + ResponseData responseData = completionContext.getClientResponse(); + Timer.builder("loadbalanced.requests.total") + // Tags corresponding to the ones used in actuator + .tags(Tags.of(Tag.of("lbCompletionStatus", completionContext.status() + .toString()), + Tag.of("method", requestData.getHttpMethod().toString()), + Tag.of("uri", requestData.getUrl().getPath()), + exception(completionContext.getThrowable()), + Outcome.forStatus(responseData.getHttpStatus().value()).asTag(), + status(responseData.getHttpStatus()) + )) + .register(meterRegistry) + .record(requestFinishedTimestamp - loadBalancerRequest.getContext() + .getRequestStartTimestamp(), + TimeUnit.NANOSECONDS); + } + +} From e63caac90f587f0a8f724ca9dbd33b1889b52d2d Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Wed, 16 Dec 2020 15:17:12 +0100 Subject: [PATCH 02/19] Add TimedRequestContext interface. Make RetryableRequestContext extend RequestDataContext. Improve generating metrics. Add utility class for working with tags. Ensure tags are not null. --- .../loadbalancer/HintRequestContext.java | 13 +- .../loadbalancer/RequestDataContext.java | 4 + .../loadbalancer/RetryableRequestContext.java | 10 +- .../loadbalancer/TimedRequestContext.java | 12 ++ ...bleLoadBalancerExchangeFilterFunction.java | 16 ++- .../client/BlockingLoadBalancerClient.java | 1 - .../loadbalancer/stats/LoadBalancerTags.java | 111 ++++++++++++++++++ .../stats/MicrometerStatsLifecycle.java | 104 +++++++--------- .../BlockingLoadBalancerClientTests.java | 39 +++--- 9 files changed, 215 insertions(+), 95 deletions(-) create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java create mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/HintRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/HintRequestContext.java index 348888d72..df02d9d0a 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/HintRequestContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/HintRequestContext.java @@ -17,7 +17,6 @@ package org.springframework.cloud.client.loadbalancer; import java.util.Objects; -import java.util.UUID; import org.springframework.core.style.ToStringCreator; @@ -26,7 +25,7 @@ * * @author Olga Maciaszek-Sharma */ -public class HintRequestContext { +public class HintRequestContext implements TimedRequestContext { /** * A {@link String} value of hint that can be used to choose the correct service @@ -51,12 +50,14 @@ public void setHint(String hint) { this.hint = hint; } - public void setRequestStartTime(long requestStartTime) { - this.requestStartTime = requestStartTime; + @Override + public long getRequestStartTime() { + return requestStartTime; } - public long getRequestStartTimestamp() { - return requestStartTime; + @Override + public void setRequestStartTime(long requestStartTime) { + this.requestStartTime = requestStartTime; } @Override diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RequestDataContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RequestDataContext.java index 003aa9f67..6bb23deee 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RequestDataContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RequestDataContext.java @@ -26,6 +26,10 @@ */ public class RequestDataContext extends DefaultRequestContext { + public RequestDataContext() { + super(); + } + public RequestDataContext(RequestData requestData) { this(requestData, "default"); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java index ef8b32ec3..2e83e561f 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java @@ -27,7 +27,7 @@ * * @author Olga Maciaszek-Sharma */ -public class RetryableRequestContext extends DefaultRequestContext { +public class RetryableRequestContext extends RequestDataContext { private final ServiceInstance previousServiceInstance; @@ -35,13 +35,13 @@ public RetryableRequestContext(ServiceInstance previousServiceInstance) { this.previousServiceInstance = previousServiceInstance; } - public RetryableRequestContext(ServiceInstance previousServiceInstance, Object clientRequest) { - super(clientRequest); + public RetryableRequestContext(ServiceInstance previousServiceInstance, RequestData clientRequestData) { + super(clientRequestData); this.previousServiceInstance = previousServiceInstance; } - public RetryableRequestContext(ServiceInstance previousServiceInstance, Object clientRequest, String hint) { - super(clientRequest, hint); + public RetryableRequestContext(ServiceInstance previousServiceInstance, RequestData clientRequestData, String hint) { + super(clientRequestData, hint); this.previousServiceInstance = previousServiceInstance; } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java new file mode 100644 index 000000000..a8b18c7fd --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java @@ -0,0 +1,12 @@ +package org.springframework.cloud.client.loadbalancer; + +/** + * @author Olga Maciaszek-Sharma + */ +public interface TimedRequestContext { + + long getRequestStartTime(); + + void setRequestStartTime(long requestStartTime); + +} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index aa5fb8803..8feb99b0d 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -110,14 +110,16 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); return Mono.defer(() -> choose(serviceId, lbRequest).flatMap(lbResponse -> { ServiceInstance instance = lbResponse.getServer(); - lbRequest.setContext(new RetryableRequestContext(instance, clientRequest, hint)); + lbRequest + .setContext(new RetryableRequestContext(instance, requestData, hint)); if (instance == null) { String message = serviceInstanceUnavailableMessage(serviceId); if (LOG.isWarnEnabled()) { LOG.warn(message); } supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse))); + .onComplete(new CompletionContext(CompletionContext.Status.DISCARD, + lbResponse, lbRequest))); return Mono.just(ClientResponse.create(HttpStatus.SERVICE_UNAVAILABLE) .body(serviceInstanceUnavailableMessage(serviceId)).build()); } @@ -132,11 +134,13 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction stickySessionProperties.isAddServiceInstanceCookie()); return next.exchange(newRequest) .doOnError(throwable -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle.onComplete(new CompletionContext( - CompletionContext.Status.FAILED, throwable, lbResponse)))) + lifecycle -> lifecycle + .onComplete(new CompletionContext( + CompletionContext.Status.FAILED, throwable, lbResponse, lbRequest)))) .doOnSuccess(clientResponse -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, - lbResponse, new ResponseData(clientResponse, requestData))))) + lifecycle -> lifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, + lbResponse, new ResponseData(clientResponse, requestData), lbRequest)))) .map(clientResponse -> { loadBalancerRetryContext.setClientResponse(clientResponse); if (shouldRetrySameServiceInstance(loadBalancerRetryContext)) { diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java index 1092c3957..9e4734e7c 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java @@ -19,7 +19,6 @@ import java.io.IOException; import java.net.URI; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import reactor.core.publisher.Mono; 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 new file mode 100644 index 000000000..726c6c390 --- /dev/null +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java @@ -0,0 +1,111 @@ +package org.springframework.cloud.loadbalancer.stats; + +import java.util.function.Function; +import java.util.regex.Pattern; + +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; + +import org.springframework.boot.actuate.metrics.http.Outcome; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.CompletionContext; +import org.springframework.cloud.client.loadbalancer.RequestData; +import org.springframework.cloud.client.loadbalancer.ResponseData; +import org.springframework.http.HttpStatus; +import org.springframework.util.StringUtils; + +/** + * @author Olga Maciaszek-Sharma + */ +public class LoadBalancerTags { + + private static final String UNKNOWN = "UNKNOWN"; + private static final Pattern PATTERN_BEFORE_PATH = Pattern + .compile("^https?://[^/]+/"); + + private LoadBalancerTags() { + throw new UnsupportedOperationException("Cannot instantiate utility class"); + } + + static Iterable buildLoadBalancedRequestTags(CompletionContext completionContext) { + Tags tags = Tags.of(Tag.of("lbCompletionStatus", completionContext.status() + .toString()), exception(completionContext.getThrowable())); + if (completionContext.getLoadBalancerResponse().hasServer()) { + ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse() + .getServer(); + tags = tags.and(buildServiceInstanceTags(serviceInstance)); + } + Object clientResponse = completionContext.getClientResponse(); + if (clientResponse instanceof ResponseData) { + ResponseData responseData = (ResponseData) clientResponse; + RequestData requestData = responseData.getRequestData(); + if (requestData != null) { + tags = tags.and(valueOrUnknown("method", requestData.getHttpMethod()), + valueOrUnknown("uri", requestData.getUrl().getPath())); + } + tags = tags.and(Outcome.forStatus(responseData.getHttpStatus().value()) + .asTag(), + status(responseData.getHttpStatus())); + } + return tags; + } + + static Iterable buildServiceInstanceTags(ServiceInstance serviceInstance) { + return Tags.of(valueOrUnknown("serviceId", serviceInstance.getServiceId()), + valueOrUnknown("serviceInstance.instanceId", serviceInstance + .getInstanceId()), + valueOrUnknown("serviceInstance.host", serviceInstance.getHost()), + valueOrUnknown("serviceInstance.port", String + .valueOf(serviceInstance.getPort())), + valueOrUnknown("serviceInstance.uri", serviceInstance + .getUri(), extractPath()), + valueOrUnknown("serviceInstance.secure", String + .valueOf(serviceInstance.isSecure()))); + } + + private static Tag valueOrUnknown(String key, String value) { + if (value != null) { + return Tag.of(key, value); + } + return Tag.of(key, UNKNOWN); + } + + private static Tag valueOrUnknown(String key, Object value) { + if (value != null) { + return Tag.of(key, String.valueOf(value)); + } + return Tag.of(key, UNKNOWN); + } + + private static Tag valueOrUnknown(String key, Object value, Function supplier) { + if (value != null) { + return Tag.of(key, supplier.apply(value.toString())); + } + return Tag.of(key, UNKNOWN); + } + + + private static Function extractPath() { + return url -> { + String path = PATTERN_BEFORE_PATH.matcher(url).replaceFirst(""); + return (path.startsWith("/") ? path : "/" + path); + }; + } + + + private static Tag status(HttpStatus status) { + if (status == null) { + status = HttpStatus.OK; + } + return Tag.of("status", String.valueOf(status.value())); + } + + private static Tag exception(Throwable exception) { + if (exception != null) { + String simpleName = exception.getClass().getSimpleName(); + return Tag.of("exception", StringUtils + .hasText(simpleName) ? simpleName : exception.getClass().getName()); + } + return Tag.of("exception", "None"); + } +} diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java index fd72ae650..3e4a59192 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java @@ -4,87 +4,74 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.Gauge; import io.micrometer.core.instrument.MeterRegistry; -import io.micrometer.core.instrument.Tag; -import io.micrometer.core.instrument.Tags; import io.micrometer.core.instrument.Timer; -import org.springframework.boot.actuate.metrics.http.Outcome; import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.loadbalancer.CompletionContext; import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; import org.springframework.cloud.client.loadbalancer.Request; -import org.springframework.cloud.client.loadbalancer.RequestData; -import org.springframework.cloud.client.loadbalancer.RequestDataContext; import org.springframework.cloud.client.loadbalancer.Response; -import org.springframework.cloud.client.loadbalancer.ResponseData; -import org.springframework.http.HttpStatus; -import org.springframework.util.StringUtils; +import org.springframework.cloud.client.loadbalancer.TimedRequestContext; + +import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.buildLoadBalancedRequestTags; +import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.buildServiceInstanceTags; /** * @author Olga Maciaszek-Sharma */ -public class MicrometerStatsLifecycle implements LoadBalancerLifecycle { - - private final ConcurrentHashMap activeRequestsPerInstance = new ConcurrentHashMap<>(); +public class MicrometerStatsLifecycle implements LoadBalancerLifecycle { private final MeterRegistry meterRegistry; + private final ConcurrentHashMap activeRequestsPerInstance = new ConcurrentHashMap<>(); public MicrometerStatsLifecycle(MeterRegistry meterRegistry) { this.meterRegistry = meterRegistry; } - private static Tag status(HttpStatus status) { - if (status == null) { - status = HttpStatus.OK; - } - return Tag.of("status", String.valueOf(status.value())); - } - - private static Tag exception(Throwable exception) { - if (exception != null) { - String simpleName = exception.getClass().getSimpleName(); - return Tag.of("exception", StringUtils - .hasText(simpleName) ? simpleName : exception.getClass().getName()); - } - return Tag.of("exception", "None"); - } - - @Override public boolean supports(Class requestContextClass, Class responseClass, Class serverTypeClass) { - throw new UnsupportedOperationException("Please, implement me."); + return ServiceInstance.class.isAssignableFrom(serverTypeClass); } @Override - public void onStart(Request request) { - + public void onStart(Request request) { + // do nothing } @Override - public void onStartRequest(Request request, Response lbResponse) { - request.getContext().setRequestStartTime(System.nanoTime()); + public void onStartRequest(Request request, Response lbResponse) { + if (request.getContext() instanceof TimedRequestContext) { + ((TimedRequestContext) request.getContext()) + .setRequestStartTime(System.nanoTime()); + } if (!lbResponse.hasServer()) { return; } ServiceInstance serviceInstance = lbResponse.getServer(); - AtomicLong activeRequestsCounter = activeRequestsPerInstance.get(serviceInstance); - if (activeRequestsCounter == null) { - activeRequestsPerInstance.put(serviceInstance, new AtomicLong()); - AtomicLong createdCounter = activeRequestsPerInstance.get(serviceInstance); - Gauge.builder("loadbalanced.requests.active", () -> createdCounter) - .tags(Tags.of(Tag.of("serviceId", serviceInstance.getServiceId()), - Tag.of("instanceId", serviceInstance.getInstanceId()))) - .register(meterRegistry); - createdCounter.incrementAndGet(); - } + AtomicLong activeRequestsCounter = activeRequestsPerInstance + .computeIfAbsent(serviceInstance, + instance -> { + AtomicLong createdCounter = activeRequestsPerInstance + .get(serviceInstance); + Gauge.builder("loadbalanced.requests.active", () -> createdCounter) + .tags(buildServiceInstanceTags(serviceInstance)) + .register(meterRegistry); + return createdCounter; + }); + activeRequestsCounter.incrementAndGet(); } @Override - public void onComplete(CompletionContext completionContext) { + public void onComplete(CompletionContext completionContext) { long requestFinishedTimestamp = System.nanoTime(); if (CompletionContext.Status.DISCARD.equals(completionContext.status())) { + Counter.builder("loadbalanced.requests.discarded") + .tags(buildLoadBalancedRequestTags(completionContext)) + .register(meterRegistry) + .increment(); return; } ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse() @@ -93,25 +80,16 @@ public void onComplete(CompletionContext loadBalancerRequest = completionContext - .getLoadBalancerRequest(); - RequestData requestData = loadBalancerRequest.getContext() - .getClientRequest(); - ResponseData responseData = completionContext.getClientResponse(); - Timer.builder("loadbalanced.requests.total") - // Tags corresponding to the ones used in actuator - .tags(Tags.of(Tag.of("lbCompletionStatus", completionContext.status() - .toString()), - Tag.of("method", requestData.getHttpMethod().toString()), - Tag.of("uri", requestData.getUrl().getPath()), - exception(completionContext.getThrowable()), - Outcome.forStatus(responseData.getHttpStatus().value()).asTag(), - status(responseData.getHttpStatus()) - )) - .register(meterRegistry) - .record(requestFinishedTimestamp - loadBalancerRequest.getContext() - .getRequestStartTimestamp(), - TimeUnit.NANOSECONDS); + Object loadBalancerRequestContext = completionContext.getLoadBalancerRequest() + .getContext(); + if (loadBalancerRequestContext instanceof TimedRequestContext) { + Timer.builder("loadbalanced.requests.executed") + .tags(buildLoadBalancedRequestTags(completionContext) + ) + .register(meterRegistry) + .record(requestFinishedTimestamp - ((TimedRequestContext) loadBalancerRequestContext) + .getRequestStartTime(), TimeUnit.NANOSECONDS); + } } } diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java index 92e343165..928204961 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java @@ -165,21 +165,27 @@ void loadBalancerLifecycleCallbacksExecuted() throws IOException { String callbackTestHint = "callbackTestHint"; loadBalancerProperties.getHint().put("myservice", "callbackTestHint"); final String result = "callbackTestResult"; - Object actualResult = loadBalancerClient.execute("myservice", (LoadBalancerRequest) instance -> { - assertThat(instance.getHost()).isEqualTo("test.example"); - return result; - }); + Object actualResult = loadBalancerClient + .execute("myservice", (LoadBalancerRequest) instance -> { + assertThat(instance.getHost()).isEqualTo("test.example"); + return result; + }); Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory - .getInstances("myservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() - .values(); - Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory - .getInstances("myservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) - .getCompleteLog().values(); + .getInstances("myservice", LoadBalancerLifecycle.class) + .get("loadBalancerLifecycle")).getStartLog() + .values(); + Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory + .getInstances("myservice", LoadBalancerLifecycle.class) + .get("anotherLoadBalancerLifecycle")) + .getCompleteLog().values(); assertThat(actualResult).isEqualTo(result); - assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) + assertThat(lifecycleLogRequests) + .extracting(request -> ((DefaultRequestContext) request.getContext()) + .getHint()) .contains(callbackTestHint); - assertThat(anotherLifecycleLogRequests).extracting(CompletionContext::getClientResponse).contains(result); + assertThat(anotherLifecycleLogRequests) + .extracting(CompletionContext::getClientResponse).contains(result); } @Configuration(proxyBeanMethods = false) @@ -233,7 +239,7 @@ protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycl final ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); - final ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); + final ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); @Override public void onStart(Request request) { @@ -241,7 +247,12 @@ public void onStart(Request request) { } @Override - public void onComplete(CompletionContext completionContext) { + public void onStartRequest(Request request, Response lbResponse) { + + } + + @Override + public void onComplete(CompletionContext completionContext) { completeLog.put(getName() + UUID.randomUUID(), completionContext); } @@ -249,7 +260,7 @@ ConcurrentHashMap> getStartLog() { return startLog; } - ConcurrentHashMap> getCompleteLog() { + ConcurrentHashMap> getCompleteLog() { return completeLog; } From cfdc34791dd861c706cd3042b89ad937cf866cd7 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Wed, 16 Dec 2020 18:49:50 +0100 Subject: [PATCH 03/19] Add separate meters depending on CompletionContext.Status. --- .../loadbalancer/stats/LoadBalancerTags.java | 105 ++++++++++++++---- .../stats/MicrometerStatsLifecycle.java | 63 ++++++++--- 2 files changed, 127 insertions(+), 41 deletions(-) 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 726c6c390..a906a4740 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 @@ -1,3 +1,19 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.springframework.cloud.loadbalancer.stats; import java.util.function.Function; @@ -10,16 +26,17 @@ import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.loadbalancer.CompletionContext; import org.springframework.cloud.client.loadbalancer.RequestData; +import org.springframework.cloud.client.loadbalancer.RequestDataContext; import org.springframework.cloud.client.loadbalancer.ResponseData; -import org.springframework.http.HttpStatus; import org.springframework.util.StringUtils; /** * @author Olga Maciaszek-Sharma */ -public class LoadBalancerTags { +final class LoadBalancerTags { private static final String UNKNOWN = "UNKNOWN"; + private static final Pattern PATTERN_BEFORE_PATH = Pattern .compile("^https?://[^/]+/"); @@ -27,29 +44,79 @@ private LoadBalancerTags() { throw new UnsupportedOperationException("Cannot instantiate utility class"); } - static Iterable buildLoadBalancedRequestTags(CompletionContext completionContext) { - Tags tags = Tags.of(Tag.of("lbCompletionStatus", completionContext.status() - .toString()), exception(completionContext.getThrowable())); - if (completionContext.getLoadBalancerResponse().hasServer()) { - ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse() - .getServer(); - tags = tags.and(buildServiceInstanceTags(serviceInstance)); - } + static Iterable buildSuccessRequestTags( + CompletionContext completionContext) { + ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse() + .getServer(); + Tags tags = Tags.of(buildServiceInstanceTags(serviceInstance)); Object clientResponse = completionContext.getClientResponse(); if (clientResponse instanceof ResponseData) { ResponseData responseData = (ResponseData) clientResponse; RequestData requestData = responseData.getRequestData(); if (requestData != null) { tags = tags.and(valueOrUnknown("method", requestData.getHttpMethod()), - valueOrUnknown("uri", requestData.getUrl().getPath())); + valueOrUnknown("uri", getPath(requestData))); } - tags = tags.and(Outcome.forStatus(responseData.getHttpStatus().value()) - .asTag(), - status(responseData.getHttpStatus())); + else { + tags = tags.and(Tag.of("method", UNKNOWN), Tag.of("uri", UNKNOWN)); + } + + tags = tags + .and(Outcome.forStatus(responseData.getHttpStatus().value()).asTag(), + valueOrUnknown("status", responseData.getHttpStatus())); + } + else { + tags = tags.and(Tag.of("method", UNKNOWN), Tag.of("uri", UNKNOWN), + Tag.of("outcome", UNKNOWN), Tag.of("status", UNKNOWN)); } return tags; } + private static String getPath(RequestData requestData) { + return requestData.getUrl() != null ? requestData.getUrl().getPath() : UNKNOWN; + } + + 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)), + valueOrUnknown("serviceId", getHost(requestData))); + } + } + return Tags.of(valueOrUnknown("method", UNKNOWN), + valueOrUnknown("uri", UNKNOWN), + valueOrUnknown("serviceId", UNKNOWN)); + + } + + private static String getHost(RequestData requestData) { + return requestData.getUrl() != null ? requestData.getUrl().getHost() : UNKNOWN; + } + + 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) { + RequestData requestData = ((RequestDataContext) completionContext + .getLoadBalancerRequest().getContext()).getClientRequest(); + if (requestData != null) { + return tags.and(Tags + .of(valueOrUnknown("method", requestData.getHttpMethod()), + valueOrUnknown("uri", getPath(requestData)))); + } + } + return tags.and(Tags.of(valueOrUnknown("method", UNKNOWN), + valueOrUnknown("uri", UNKNOWN))); + } + + static Iterable buildServiceInstanceTags(ServiceInstance serviceInstance) { return Tags.of(valueOrUnknown("serviceId", serviceInstance.getServiceId()), valueOrUnknown("serviceInstance.instanceId", serviceInstance @@ -84,7 +151,6 @@ private static Tag valueOrUnknown(String key, Object value, Function extractPath() { return url -> { String path = PATTERN_BEFORE_PATH.matcher(url).replaceFirst(""); @@ -92,14 +158,6 @@ private static Function extractPath() { }; } - - private static Tag status(HttpStatus status) { - if (status == null) { - status = HttpStatus.OK; - } - return Tag.of("status", String.valueOf(status.value())); - } - private static Tag exception(Throwable exception) { if (exception != null) { String simpleName = exception.getClass().getSimpleName(); @@ -108,4 +166,5 @@ private static Tag exception(Throwable exception) { } return Tag.of("exception", "None"); } + } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java index 3e4a59192..1418b3557 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java @@ -1,3 +1,19 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.springframework.cloud.loadbalancer.stats; import java.util.concurrent.ConcurrentHashMap; @@ -16,8 +32,10 @@ import org.springframework.cloud.client.loadbalancer.Response; import org.springframework.cloud.client.loadbalancer.TimedRequestContext; -import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.buildLoadBalancedRequestTags; +import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.buildDiscardedRequestTags; +import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.buildFailedRequestTags; import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.buildServiceInstanceTags; +import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.buildSuccessRequestTags; /** * @author Olga Maciaszek-Sharma @@ -25,6 +43,7 @@ public class MicrometerStatsLifecycle implements LoadBalancerLifecycle { private final MeterRegistry meterRegistry; + private final ConcurrentHashMap activeRequestsPerInstance = new ConcurrentHashMap<>(); public MicrometerStatsLifecycle(MeterRegistry meterRegistry) { @@ -52,15 +71,14 @@ public void onStartRequest(Request request, Response lb } ServiceInstance serviceInstance = lbResponse.getServer(); AtomicLong activeRequestsCounter = activeRequestsPerInstance - .computeIfAbsent(serviceInstance, - instance -> { - AtomicLong createdCounter = activeRequestsPerInstance - .get(serviceInstance); - Gauge.builder("loadbalanced.requests.active", () -> createdCounter) - .tags(buildServiceInstanceTags(serviceInstance)) - .register(meterRegistry); - return createdCounter; - }); + .computeIfAbsent(serviceInstance, instance -> { + AtomicLong createdCounter = activeRequestsPerInstance + .get(serviceInstance); + Gauge.builder("loadbalanced.requests.active", () -> createdCounter) + .tags(buildServiceInstanceTags(serviceInstance)) + .register(meterRegistry); + return createdCounter; + }); activeRequestsCounter.incrementAndGet(); } @@ -69,9 +87,8 @@ public void onComplete(CompletionContext comple long requestFinishedTimestamp = System.nanoTime(); if (CompletionContext.Status.DISCARD.equals(completionContext.status())) { Counter.builder("loadbalanced.requests.discarded") - .tags(buildLoadBalancedRequestTags(completionContext)) - .register(meterRegistry) - .increment(); + .tags(buildDiscardedRequestTags(completionContext)) + .register(meterRegistry).increment(); return; } ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse() @@ -83,12 +100,22 @@ public void onComplete(CompletionContext comple Object loadBalancerRequestContext = completionContext.getLoadBalancerRequest() .getContext(); if (loadBalancerRequestContext instanceof TimedRequestContext) { - Timer.builder("loadbalanced.requests.executed") - .tags(buildLoadBalancedRequestTags(completionContext) - ) + if (CompletionContext.Status.FAILED.equals(completionContext.status())) { + Timer.builder("loadbalanced.requests.failed") + .tags(buildFailedRequestTags(completionContext)) + .register(meterRegistry) + .record(requestFinishedTimestamp + - ((TimedRequestContext) loadBalancerRequestContext) + .getRequestStartTime(), + TimeUnit.NANOSECONDS); + } + Timer.builder("loadbalanced.requests.success") + .tags(buildSuccessRequestTags(completionContext)) .register(meterRegistry) - .record(requestFinishedTimestamp - ((TimedRequestContext) loadBalancerRequestContext) - .getRequestStartTime(), TimeUnit.NANOSECONDS); + .record(requestFinishedTimestamp + - ((TimedRequestContext) loadBalancerRequestContext) + .getRequestStartTime(), + TimeUnit.NANOSECONDS); } } From 101db0442e3b042899c4ed848599f540b15c7106 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Wed, 16 Dec 2020 20:25:45 +0100 Subject: [PATCH 04/19] Modify registered metrics. Add adapter for BlockingLoadBalancerClient requests. Add autoconfiguration. --- .../loadbalancer/CompletionContext.java | 8 +- .../loadbalancer/LoadBalancerLifecycle.java | 6 +- .../LoadBalancerRequestAdapter.java | 38 ++++++++ .../RetryLoadBalancerInterceptor.java | 22 +++-- .../loadbalancer/RetryableRequestContext.java | 3 +- .../loadbalancer/TimedRequestContext.java | 16 ++++ ...torLoadBalancerExchangeFilterFunction.java | 24 ++---- ...bleLoadBalancerExchangeFilterFunction.java | 20 ++--- .../RetryLoadBalancerInterceptorTests.java | 13 ++- ...adBalancerExchangeFilterFunctionTests.java | 14 ++- ...xchangeFilterFunctionIntegrationTests.java | 14 ++- .../client/BlockingLoadBalancerClient.java | 25 +++--- .../LoadBalancerStatsAutoConfiguration.java | 40 +++++++++ .../loadbalancer/stats/LoadBalancerTags.java | 86 ++++++------------- .../stats/MicrometerStatsLifecycle.java | 39 +++------ .../main/resources/META-INF/spring.factories | 3 +- .../BlockingLoadBalancerClientTests.java | 26 +++--- 17 files changed, 229 insertions(+), 168 deletions(-) create mode 100644 spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java create mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java index 8699e977c..639ded145 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java @@ -16,8 +16,6 @@ package org.springframework.cloud.client.loadbalancer; -import java.util.UUID; - import org.springframework.core.style.ToStringCreator; /** @@ -47,11 +45,13 @@ public CompletionContext(Status status, Response response, Request loadBal this(status, null, response, null, loadBalancerRequest); } - public CompletionContext(Status status, Throwable throwable, Response loadBalancerResponse, Request loadBalancerRequest) { + public CompletionContext(Status status, Throwable throwable, Response loadBalancerResponse, + Request loadBalancerRequest) { this(status, throwable, loadBalancerResponse, null, loadBalancerRequest); } - public CompletionContext(Status status, Response loadBalancerResponse, RES clientResponse,Request loadBalancerRequest) { + public CompletionContext(Status status, Response loadBalancerResponse, RES clientResponse, + Request loadBalancerRequest) { this(status, null, loadBalancerResponse, clientResponse, loadBalancerRequest); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java index 34ceb88cd..e78505eaa 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java @@ -48,8 +48,10 @@ default boolean supports(Class requestContextClass, Class responseClass, Class s void onStart(Request request); /** - * A callback method executed after a service instance has been selected, before executing the actual load-balanced request. - * @param request the {@link Request} that has been used by the LoadBalancer to select a service instance + * A callback method executed after a service instance has been selected, before + * executing the actual load-balanced request. + * @param request the {@link Request} that has been used by the LoadBalancer to select + * a service instance * @param lbResponse the {@link Response} returned by the LoadBalancer */ void onStartRequest(Request request, Response lbResponse); diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java new file mode 100644 index 000000000..a39e09f03 --- /dev/null +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java @@ -0,0 +1,38 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.client.loadbalancer; + +import org.springframework.cloud.client.ServiceInstance; + +/** + * @author Olga Maciaszek-Sharma + */ +public class LoadBalancerRequestAdapter extends DefaultRequest + implements LoadBalancerRequest { + + private final LoadBalancerRequest delegate; + + public LoadBalancerRequestAdapter(LoadBalancerRequest delegate) { + this.delegate = delegate; + } + + @Override + public T apply(ServiceInstance instance) throws Exception { + return delegate.apply(instance); + } + +} diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java index 56d267976..8d2f48bae 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java @@ -90,6 +90,7 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body .getSupportedLifecycleProcessors( loadBalancerFactory.getInstances(serviceName, LoadBalancerLifecycle.class), RequestDataContext.class, ResponseData.class, ServiceInstance.class); + String hint = getHint(serviceName); if (serviceInstance == null) { if (LOG.isDebugEnabled()) { LOG.debug("Service instance retrieved from LoadBalancedRetryContext: was null. " @@ -100,7 +101,6 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; previousServiceInstance = lbContext.getPreviousServiceInstance(); } - String hint = getHint(serviceName); DefaultRequest lbRequest = new DefaultRequest<>( new RetryableRequestContext(previousServiceInstance, new RequestData(request), hint)); supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); @@ -112,15 +112,21 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext) context; lbContext.setServiceInstance(serviceInstance); } + Response lbResponse = new DefaultResponse(serviceInstance); + if (serviceInstance == null) { + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle + .onComplete(new CompletionContext( + CompletionContext.Status.DISCARD, lbResponse, new DefaultRequest<>( + new RetryableRequestContext(null, new RequestData(request), hint))))); + } } - Response lbResponse = new DefaultResponse(serviceInstance); - if (serviceInstance == null) { - supportedLifecycleProcessors - .forEach(lifecycle -> lifecycle.onComplete(new CompletionContext( - CompletionContext.Status.DISCARD, lbResponse))); - } + ServiceInstance finalServiceInstance = serviceInstance; + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStartRequest( + new DefaultRequest<>(new RetryableRequestContext(null, new RequestData(request), hint)), + new DefaultResponse(finalServiceInstance))); ClientHttpResponse response = RetryLoadBalancerInterceptor.this.loadBalancer.execute(serviceName, - serviceInstance, requestFactory.createRequest(request, body, execution)); + finalServiceInstance, + new LoadBalancerRequestAdapter<>(requestFactory.createRequest(request, body, execution))); int statusCode = response.getRawStatusCode(); if (retryPolicy != null && retryPolicy.retryableStatusCode(statusCode)) { if (LOG.isDebugEnabled()) { diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java index 2e83e561f..79ec07a3b 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java @@ -40,7 +40,8 @@ public RetryableRequestContext(ServiceInstance previousServiceInstance, RequestD this.previousServiceInstance = previousServiceInstance; } - public RetryableRequestContext(ServiceInstance previousServiceInstance, RequestData clientRequestData, String hint) { + public RetryableRequestContext(ServiceInstance previousServiceInstance, RequestData clientRequestData, + String hint) { super(clientRequestData, hint); this.previousServiceInstance = previousServiceInstance; } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java index a8b18c7fd..b9e257fac 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java @@ -1,3 +1,19 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.springframework.cloud.client.loadbalancer; /** diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java index 699a42afe..f63bd6c0e 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java @@ -100,27 +100,21 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction } if (LOG.isDebugEnabled()) { - LOG.debug(String - .format("LoadBalancer has retrieved the instance for service %s: %s", serviceId, - instance.getUri())); + LOG.debug(String.format("LoadBalancer has retrieved the instance for service %s: %s", serviceId, + instance.getUri())); } - LoadBalancerProperties.StickySession stickySessionProperties = properties - .getStickySession(); + LoadBalancerProperties.StickySession stickySessionProperties = properties.getStickySession(); ClientRequest newRequest = buildClientRequest(clientRequest, instance, stickySessionProperties.getInstanceIdCookieName(), stickySessionProperties.isAddServiceInstanceCookie()); - supportedLifecycleProcessors - .forEach(lifecycle -> lifecycle.onStartRequest( - lbRequest, lbResponse)); + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStartRequest(lbRequest, lbResponse)); return next.exchange(newRequest) - .doOnError(throwable -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle - .onComplete(new CompletionContext( - CompletionContext.Status.FAILED, throwable, lbResponse, lbRequest)))) + .doOnError(throwable -> supportedLifecycleProcessors.forEach(lifecycle -> lifecycle + .onComplete(new CompletionContext( + CompletionContext.Status.FAILED, throwable, lbResponse, lbRequest)))) .doOnSuccess(clientResponse -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, - lbResponse, new ResponseData(clientResponse, requestData), lbRequest)))); + lifecycle -> lifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, + lbResponse, new ResponseData(clientResponse, requestData), lbRequest)))); }); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index 8feb99b0d..47a15ba2a 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -110,16 +110,15 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); return Mono.defer(() -> choose(serviceId, lbRequest).flatMap(lbResponse -> { ServiceInstance instance = lbResponse.getServer(); - lbRequest - .setContext(new RetryableRequestContext(instance, requestData, hint)); + lbRequest.setContext(new RetryableRequestContext(instance, requestData, hint)); if (instance == null) { String message = serviceInstanceUnavailableMessage(serviceId); if (LOG.isWarnEnabled()) { LOG.warn(message); } supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext(CompletionContext.Status.DISCARD, - lbResponse, lbRequest))); + .onComplete(new CompletionContext( + CompletionContext.Status.DISCARD, lbResponse, lbRequest))); return Mono.just(ClientResponse.create(HttpStatus.SERVICE_UNAVAILABLE) .body(serviceInstanceUnavailableMessage(serviceId)).build()); } @@ -132,15 +131,14 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction ClientRequest newRequest = buildClientRequest(clientRequest, instance, stickySessionProperties.getInstanceIdCookieName(), stickySessionProperties.isAddServiceInstanceCookie()); + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStartRequest(lbRequest, lbResponse)); return next.exchange(newRequest) - .doOnError(throwable -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle - .onComplete(new CompletionContext( - CompletionContext.Status.FAILED, throwable, lbResponse, lbRequest)))) + .doOnError(throwable -> supportedLifecycleProcessors.forEach(lifecycle -> lifecycle + .onComplete(new CompletionContext( + CompletionContext.Status.FAILED, throwable, lbResponse, lbRequest)))) .doOnSuccess(clientResponse -> supportedLifecycleProcessors.forEach( - lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, - lbResponse, new ResponseData(clientResponse, requestData), lbRequest)))) + lifecycle -> lifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, + lbResponse, new ResponseData(clientResponse, requestData), lbRequest)))) .map(clientResponse -> { loadBalancerRetryContext.setClientResponse(clientResponse); if (shouldRetrySameServiceInstance(loadBalancerRetryContext)) { diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java index 55f906258..55bf4b3fe 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java @@ -503,7 +503,7 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala T response = (T) new MockClientHttpResponse(new byte[] {}, HttpStatus.OK); supportedLoadBalancerProcessors .forEach(lifecycle -> lifecycle.onComplete(new CompletionContext(CompletionContext.Status.SUCCESS, - new DefaultResponse(defaultServiceInstance())))); + new DefaultResponse(defaultServiceInstance()), new DefaultRequest<>()))); return response; } @@ -532,7 +532,7 @@ protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycl final ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); - final ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); + final ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); @Override public boolean supports(Class requestContextClass, Class responseClass, Class serverTypeClass) { @@ -547,7 +547,12 @@ public void onStart(Request request) { } @Override - public void onComplete(CompletionContext completionContext) { + public void onStartRequest(Request request, Response lbResponse) { + + } + + @Override + public void onComplete(CompletionContext completionContext) { completeLog.put(getName() + UUID.randomUUID(), completionContext); } @@ -555,7 +560,7 @@ ConcurrentHashMap> getStartLog() { return startLog; } - ConcurrentHashMap> getCompleteLog() { + ConcurrentHashMap> getCompleteLog() { return completeLog; } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java index 4c40b88d9..06e2436a9 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java @@ -43,6 +43,7 @@ import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; import org.springframework.cloud.client.loadbalancer.LoadBalancerProperties; import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.cloud.client.loadbalancer.Response; import org.springframework.cloud.client.loadbalancer.ResponseData; import org.springframework.context.annotation.Bean; import org.springframework.http.HttpStatus; @@ -135,7 +136,7 @@ void loadBalancerLifecycleCallbacksExecuted() { Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() .values(); - Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory + Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) .getCompleteLog().values(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); @@ -206,7 +207,7 @@ protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycl ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); - ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); + ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); @Override public void onStart(Request request) { @@ -214,7 +215,12 @@ public void onStart(Request request) { } @Override - public void onComplete(CompletionContext completionContext) { + public void onStartRequest(Request request, Response lbResponse) { + + } + + @Override + public void onComplete(CompletionContext completionContext) { completeLog.put(getName() + UUID.randomUUID(), completionContext); } @@ -222,7 +228,7 @@ ConcurrentHashMap> getStartLog() { return startLog; } - ConcurrentHashMap> getCompleteLog() { + ConcurrentHashMap> getCompleteLog() { return completeLog; } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java index ad44571e3..892008810 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java @@ -45,6 +45,7 @@ import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycle; import org.springframework.cloud.client.loadbalancer.LoadBalancerProperties; import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.cloud.client.loadbalancer.Response; import org.springframework.cloud.client.loadbalancer.ResponseData; import org.springframework.context.annotation.Bean; import org.springframework.http.HttpMethod; @@ -108,7 +109,7 @@ void loadBalancerLifecycleCallbacksExecuted() { Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() .values(); - Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory + Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) .getCompleteLog().values(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); @@ -270,7 +271,7 @@ protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycl Map> startLog = new ConcurrentHashMap<>(); - Map> completeLog = new ConcurrentHashMap<>(); + Map> completeLog = new ConcurrentHashMap<>(); @Override public void onStart(Request request) { @@ -278,7 +279,12 @@ public void onStart(Request request) { } @Override - public void onComplete(CompletionContext completionContext) { + public void onStartRequest(Request request, Response lbResponse) { + + } + + @Override + public void onComplete(CompletionContext completionContext) { completeLog.clear(); completeLog.put(getName() + UUID.randomUUID(), completionContext); } @@ -287,7 +293,7 @@ Map> getStartLog() { return startLog; } - Map> getCompleteLog() { + Map> getCompleteLog() { return completeLog; } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java index 9e4734e7c..ef1fc38b1 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java @@ -33,9 +33,11 @@ import org.springframework.cloud.client.loadbalancer.LoadBalancerLifecycleValidator; import org.springframework.cloud.client.loadbalancer.LoadBalancerProperties; import org.springframework.cloud.client.loadbalancer.LoadBalancerRequest; +import org.springframework.cloud.client.loadbalancer.LoadBalancerRequestAdapter; import org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools; import org.springframework.cloud.client.loadbalancer.Request; import org.springframework.cloud.client.loadbalancer.Response; +import org.springframework.cloud.client.loadbalancer.TimedRequestContext; import org.springframework.cloud.client.loadbalancer.reactive.ReactiveLoadBalancer; import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; import org.springframework.util.ReflectionUtils; @@ -74,11 +76,11 @@ public T execute(String serviceId, LoadBalancerRequest request) throws IO supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStart(lbRequest)); ServiceInstance serviceInstance = choose(serviceId, lbRequest); if (serviceInstance == null) { - supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, new EmptyResponse(), lbRequest))); + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( + new CompletionContext<>(CompletionContext.Status.DISCARD, new EmptyResponse(), lbRequest))); throw new IllegalStateException("No instances available for " + serviceId); } - return execute(serviceId, serviceInstance, request); + return execute(serviceId, serviceInstance, new LoadBalancerRequestAdapter<>(request)); } @Override @@ -89,23 +91,24 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala .getSupportedLifecycleProcessors( loadBalancerClientFactory.getInstances(serviceId, LoadBalancerLifecycle.class), DefaultRequestContext.class, Object.class, ServiceInstance.class); - // TODO: maybe better just pass start time? - Request fakeLbRequest = new DefaultRequest<>(); - fakeLbRequest.getContext().setRequestStartTime(System.nanoTime()); + Request lbRequest = request instanceof Request ? (Request) request : new DefaultRequest<>(); + if (lbRequest.getContext() instanceof TimedRequestContext) { + ((TimedRequestContext) lbRequest.getContext()).setRequestStartTime(System.nanoTime()); + } try { T response = request.apply(serviceInstance); - supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, defaultResponse, response, fakeLbRequest))); + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( + new CompletionContext<>(CompletionContext.Status.SUCCESS, defaultResponse, response, lbRequest))); return response; } catch (IOException iOException) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.FAILED, iOException, defaultResponse, fakeLbRequest ))); + new CompletionContext<>(CompletionContext.Status.FAILED, iOException, defaultResponse, lbRequest))); throw iOException; } catch (Exception exception) { - supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.FAILED, exception, defaultResponse, fakeLbRequest))); + supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( + new CompletionContext<>(CompletionContext.Status.FAILED, exception, defaultResponse, lbRequest))); ReflectionUtils.rethrowRuntimeException(exception); } return null; 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 new file mode 100644 index 000000000..f2f5824d5 --- /dev/null +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/config/LoadBalancerStatsAutoConfiguration.java @@ -0,0 +1,40 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.loadbalancer.config; + +import io.micrometer.core.instrument.MeterRegistry; + +import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.loadbalancer.stats.MicrometerStatsLifecycle; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +/** + * @author Olga Maciaszek-Sharma + */ +@Configuration(proxyBeanMethods = false) +@ConditionalOnProperty(value = "spring.cloud.loadbalancer.stats.micrometer.enabled", havingValue = "true") +public class LoadBalancerStatsAutoConfiguration { + + @Bean + @ConditionalOnBean(MeterRegistry.class) + public MicrometerStatsLifecycle micrometerStatsLifecycle(MeterRegistry meterRegistry) { + return new MicrometerStatsLifecycle(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 a906a4740..6c961665f 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 @@ -16,9 +16,6 @@ package org.springframework.cloud.loadbalancer.stats; -import java.util.function.Function; -import java.util.regex.Pattern; - import io.micrometer.core.instrument.Tag; import io.micrometer.core.instrument.Tags; @@ -37,17 +34,12 @@ final class LoadBalancerTags { private static final String UNKNOWN = "UNKNOWN"; - private static final Pattern PATTERN_BEFORE_PATH = Pattern - .compile("^https?://[^/]+/"); - private LoadBalancerTags() { throw new UnsupportedOperationException("Cannot instantiate utility class"); } - static Iterable buildSuccessRequestTags( - CompletionContext completionContext) { - ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse() - .getServer(); + static Iterable buildSuccessRequestTags(CompletionContext completionContext) { + ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); Tags tags = Tags.of(buildServiceInstanceTags(serviceInstance)); Object clientResponse = completionContext.getClientResponse(); if (clientResponse instanceof ResponseData) { @@ -61,13 +53,12 @@ static Iterable buildSuccessRequestTags( tags = tags.and(Tag.of("method", UNKNOWN), Tag.of("uri", UNKNOWN)); } - tags = tags - .and(Outcome.forStatus(responseData.getHttpStatus().value()).asTag(), - valueOrUnknown("status", responseData.getHttpStatus())); + tags = tags.and(Outcome.forStatus(responseData.getHttpStatus().value()).asTag(), + valueOrUnknown("status", responseData.getHttpStatus())); } else { - tags = tags.and(Tag.of("method", UNKNOWN), Tag.of("uri", UNKNOWN), - Tag.of("outcome", UNKNOWN), Tag.of("status", UNKNOWN)); + tags = tags.and(Tag.of("method", UNKNOWN), Tag.of("uri", UNKNOWN), Tag.of("outcome", UNKNOWN), + Tag.of("status", UNKNOWN)); } return tags; } @@ -76,19 +67,17 @@ private static String getPath(RequestData requestData) { return requestData.getUrl() != null ? requestData.getUrl().getPath() : UNKNOWN; } - static Iterable buildDiscardedRequestTags(CompletionContext completionContext) { - if (completionContext.getLoadBalancerRequest() - .getContext() instanceof RequestDataContext) { - RequestData requestData = ((RequestDataContext) completionContext - .getLoadBalancerRequest().getContext()).getClientRequest(); + 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)), - valueOrUnknown("serviceId", getHost(requestData))); + valueOrUnknown("uri", getPath(requestData)), valueOrUnknown("serviceId", getHost(requestData))); } } - return Tags.of(valueOrUnknown("method", UNKNOWN), - valueOrUnknown("uri", UNKNOWN), + return Tags.of(valueOrUnknown("method", UNKNOWN), valueOrUnknown("uri", UNKNOWN), valueOrUnknown("serviceId", UNKNOWN)); } @@ -98,36 +87,24 @@ private static String getHost(RequestData requestData) { } 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) { - RequestData requestData = ((RequestDataContext) completionContext - .getLoadBalancerRequest().getContext()).getClientRequest(); + ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); + Tags tags = Tags.of(buildServiceInstanceTags(serviceInstance)).and(exception(completionContext.getThrowable())); + if (completionContext.getLoadBalancerRequest().getContext() instanceof RequestDataContext) { + RequestData requestData = ((RequestDataContext) completionContext.getLoadBalancerRequest().getContext()) + .getClientRequest(); if (requestData != null) { - return tags.and(Tags - .of(valueOrUnknown("method", requestData.getHttpMethod()), - valueOrUnknown("uri", getPath(requestData)))); + return tags.and(Tags.of(valueOrUnknown("method", requestData.getHttpMethod()), + valueOrUnknown("uri", getPath(requestData)))); } } - return tags.and(Tags.of(valueOrUnknown("method", UNKNOWN), - valueOrUnknown("uri", UNKNOWN))); + return tags.and(Tags.of(valueOrUnknown("method", UNKNOWN), valueOrUnknown("uri", UNKNOWN))); } - static Iterable buildServiceInstanceTags(ServiceInstance serviceInstance) { return Tags.of(valueOrUnknown("serviceId", serviceInstance.getServiceId()), - valueOrUnknown("serviceInstance.instanceId", serviceInstance - .getInstanceId()), + valueOrUnknown("serviceInstance.instanceId", serviceInstance.getInstanceId()), valueOrUnknown("serviceInstance.host", serviceInstance.getHost()), - valueOrUnknown("serviceInstance.port", String - .valueOf(serviceInstance.getPort())), - valueOrUnknown("serviceInstance.uri", serviceInstance - .getUri(), extractPath()), - valueOrUnknown("serviceInstance.secure", String - .valueOf(serviceInstance.isSecure()))); + valueOrUnknown("serviceInstance.port", String.valueOf(serviceInstance.getPort()))); } private static Tag valueOrUnknown(String key, String value) { @@ -144,25 +121,10 @@ private static Tag valueOrUnknown(String key, Object value) { return Tag.of(key, UNKNOWN); } - private static Tag valueOrUnknown(String key, Object value, Function supplier) { - if (value != null) { - return Tag.of(key, supplier.apply(value.toString())); - } - return Tag.of(key, UNKNOWN); - } - - private static Function extractPath() { - return url -> { - String path = PATTERN_BEFORE_PATH.matcher(url).replaceFirst(""); - return (path.startsWith("/") ? path : "/" + path); - }; - } - private static Tag exception(Throwable exception) { if (exception != null) { String simpleName = exception.getClass().getSimpleName(); - return Tag.of("exception", StringUtils - .hasText(simpleName) ? simpleName : exception.getClass().getName()); + return Tag.of("exception", StringUtils.hasText(simpleName) ? simpleName : exception.getClass().getName()); } return Tag.of("exception", "None"); } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java index 1418b3557..7744d0e79 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java @@ -63,22 +63,18 @@ public void onStart(Request request) { @Override public void onStartRequest(Request request, Response lbResponse) { if (request.getContext() instanceof TimedRequestContext) { - ((TimedRequestContext) request.getContext()) - .setRequestStartTime(System.nanoTime()); + ((TimedRequestContext) request.getContext()).setRequestStartTime(System.nanoTime()); } if (!lbResponse.hasServer()) { return; } ServiceInstance serviceInstance = lbResponse.getServer(); - AtomicLong activeRequestsCounter = activeRequestsPerInstance - .computeIfAbsent(serviceInstance, instance -> { - AtomicLong createdCounter = activeRequestsPerInstance - .get(serviceInstance); - Gauge.builder("loadbalanced.requests.active", () -> createdCounter) - .tags(buildServiceInstanceTags(serviceInstance)) - .register(meterRegistry); - return createdCounter; - }); + AtomicLong activeRequestsCounter = activeRequestsPerInstance.computeIfAbsent(serviceInstance, instance -> { + AtomicLong createdCounter = new AtomicLong(); + Gauge.builder("loadbalancer.requests.active", () -> createdCounter) + .tags(buildServiceInstanceTags(serviceInstance)).register(meterRegistry); + return createdCounter; + }); activeRequestsCounter.incrementAndGet(); } @@ -86,35 +82,28 @@ 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("loadbalanced.requests.discarded") - .tags(buildDiscardedRequestTags(completionContext)) + Counter.builder("loadbalancer.requests.discarded").tags(buildDiscardedRequestTags(completionContext)) .register(meterRegistry).increment(); return; } - ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse() - .getServer(); + ServiceInstance serviceInstance = completionContext.getLoadBalancerResponse().getServer(); AtomicLong activeRequestsCounter = activeRequestsPerInstance.get(serviceInstance); if (activeRequestsCounter != null) { activeRequestsCounter.decrementAndGet(); } - Object loadBalancerRequestContext = completionContext.getLoadBalancerRequest() - .getContext(); + Object loadBalancerRequestContext = completionContext.getLoadBalancerRequest().getContext(); if (loadBalancerRequestContext instanceof TimedRequestContext) { if (CompletionContext.Status.FAILED.equals(completionContext.status())) { - Timer.builder("loadbalanced.requests.failed") - .tags(buildFailedRequestTags(completionContext)) + Timer.builder("loadbalancer.requests.failed").tags(buildFailedRequestTags(completionContext)) .register(meterRegistry) .record(requestFinishedTimestamp - - ((TimedRequestContext) loadBalancerRequestContext) - .getRequestStartTime(), + - ((TimedRequestContext) loadBalancerRequestContext).getRequestStartTime(), TimeUnit.NANOSECONDS); } - Timer.builder("loadbalanced.requests.success") - .tags(buildSuccessRequestTags(completionContext)) + Timer.builder("loadbalancer.requests.success").tags(buildSuccessRequestTags(completionContext)) .register(meterRegistry) .record(requestFinishedTimestamp - - ((TimedRequestContext) loadBalancerRequestContext) - .getRequestStartTime(), + - ((TimedRequestContext) loadBalancerRequestContext).getRequestStartTime(), TimeUnit.NANOSECONDS); } } diff --git a/spring-cloud-loadbalancer/src/main/resources/META-INF/spring.factories b/spring-cloud-loadbalancer/src/main/resources/META-INF/spring.factories index 3e9c647b4..180aec7f4 100644 --- a/spring-cloud-loadbalancer/src/main/resources/META-INF/spring.factories +++ b/spring-cloud-loadbalancer/src/main/resources/META-INF/spring.factories @@ -2,4 +2,5 @@ org.springframework.boot.autoconfigure.EnableAutoConfiguration=\ org.springframework.cloud.loadbalancer.config.LoadBalancerAutoConfiguration,\ org.springframework.cloud.loadbalancer.config.BlockingLoadBalancerClientAutoConfiguration,\ -org.springframework.cloud.loadbalancer.config.LoadBalancerCacheAutoConfiguration \ No newline at end of file +org.springframework.cloud.loadbalancer.config.LoadBalancerCacheAutoConfiguration,\ +org.springframework.cloud.loadbalancer.config.LoadBalancerStatsAutoConfiguration \ No newline at end of file diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java index 928204961..c9d1381a5 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java @@ -165,27 +165,21 @@ void loadBalancerLifecycleCallbacksExecuted() throws IOException { String callbackTestHint = "callbackTestHint"; loadBalancerProperties.getHint().put("myservice", "callbackTestHint"); final String result = "callbackTestResult"; - Object actualResult = loadBalancerClient - .execute("myservice", (LoadBalancerRequest) instance -> { - assertThat(instance.getHost()).isEqualTo("test.example"); - return result; - }); + Object actualResult = loadBalancerClient.execute("myservice", (LoadBalancerRequest) instance -> { + assertThat(instance.getHost()).isEqualTo("test.example"); + return result; + }); Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory - .getInstances("myservice", LoadBalancerLifecycle.class) - .get("loadBalancerLifecycle")).getStartLog() - .values(); + .getInstances("myservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() + .values(); Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory - .getInstances("myservice", LoadBalancerLifecycle.class) - .get("anotherLoadBalancerLifecycle")) - .getCompleteLog().values(); + .getInstances("myservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) + .getCompleteLog().values(); assertThat(actualResult).isEqualTo(result); - assertThat(lifecycleLogRequests) - .extracting(request -> ((DefaultRequestContext) request.getContext()) - .getHint()) + assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) .contains(callbackTestHint); - assertThat(anotherLifecycleLogRequests) - .extracting(CompletionContext::getClientResponse).contains(result); + assertThat(anotherLifecycleLogRequests).extracting(CompletionContext::getClientResponse).contains(result); } @Configuration(proxyBeanMethods = false) From 3c6d30c805ba638a49363c91ded4afc1bd3564b4 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 12:45:56 +0100 Subject: [PATCH 05/19] Make new config conditional on MeterRegistry class. --- .../loadbalancer/config/LoadBalancerStatsAutoConfiguration.java | 2 ++ 1 file changed, 2 insertions(+) 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 f2f5824d5..9bdef276d 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 @@ -19,6 +19,7 @@ import io.micrometer.core.instrument.MeterRegistry; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.cloud.loadbalancer.stats.MicrometerStatsLifecycle; import org.springframework.context.annotation.Bean; @@ -28,6 +29,7 @@ * @author Olga Maciaszek-Sharma */ @Configuration(proxyBeanMethods = false) +@ConditionalOnClass(MeterRegistry.class) @ConditionalOnProperty(value = "spring.cloud.loadbalancer.stats.micrometer.enabled", havingValue = "true") public class LoadBalancerStatsAutoConfiguration { From 28ad9294771dea5db8e8b40dfd2c02b7e88ae112 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 12:55:29 +0100 Subject: [PATCH 06/19] Rename lifecycle bean. Do not log request if 0 timestamp. --- .../config/LoadBalancerStatsAutoConfiguration.java | 6 +++--- ...java => MicrometerStatsLoadBalancerLifecycle.java} | 11 ++++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) rename spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/{MicrometerStatsLifecycle.java => MicrometerStatsLoadBalancerLifecycle.java} (90%) 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 9bdef276d..7478a3966 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 @@ -21,7 +21,7 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; -import org.springframework.cloud.loadbalancer.stats.MicrometerStatsLifecycle; +import org.springframework.cloud.loadbalancer.stats.MicrometerStatsLoadBalancerLifecycle; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -35,8 +35,8 @@ public class LoadBalancerStatsAutoConfiguration { @Bean @ConditionalOnBean(MeterRegistry.class) - public MicrometerStatsLifecycle micrometerStatsLifecycle(MeterRegistry meterRegistry) { - return new MicrometerStatsLifecycle(meterRegistry); + public MicrometerStatsLoadBalancerLifecycle micrometerStatsLifecycle(MeterRegistry meterRegistry) { + return new MicrometerStatsLoadBalancerLifecycle(meterRegistry); } } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java similarity index 90% rename from spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java rename to spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java index 7744d0e79..592aef876 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLifecycle.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java @@ -40,13 +40,13 @@ /** * @author Olga Maciaszek-Sharma */ -public class MicrometerStatsLifecycle implements LoadBalancerLifecycle { +public class MicrometerStatsLoadBalancerLifecycle implements LoadBalancerLifecycle { private final MeterRegistry meterRegistry; private final ConcurrentHashMap activeRequestsPerInstance = new ConcurrentHashMap<>(); - public MicrometerStatsLifecycle(MeterRegistry meterRegistry) { + public MicrometerStatsLoadBalancerLifecycle(MeterRegistry meterRegistry) { this.meterRegistry = meterRegistry; } @@ -92,7 +92,7 @@ public void onComplete(CompletionContext comple activeRequestsCounter.decrementAndGet(); } Object loadBalancerRequestContext = completionContext.getLoadBalancerRequest().getContext(); - if (loadBalancerRequestContext instanceof TimedRequestContext) { + if (requestHasBeenTimed(loadBalancerRequestContext)) { if (CompletionContext.Status.FAILED.equals(completionContext.status())) { Timer.builder("loadbalancer.requests.failed").tags(buildFailedRequestTags(completionContext)) .register(meterRegistry) @@ -108,4 +108,9 @@ public void onComplete(CompletionContext comple } } + private boolean requestHasBeenTimed(Object loadBalancerRequestContext) { + return loadBalancerRequestContext instanceof TimedRequestContext && (((TimedRequestContext) loadBalancerRequestContext) + .getRequestStartTime() != 0L); + } + } From 05edd79fb36d8e7aac450ec011d67ddb50b5de77 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 13:28:49 +0100 Subject: [PATCH 07/19] Fix onStartRequest call arguments for BlockingLoadBalancerClient. --- .../client/loadbalancer/LoadBalancerRequestAdapter.java | 8 ++++++-- .../blocking/client/BlockingLoadBalancerClient.java | 6 ++++-- .../stats/MicrometerStatsLoadBalancerLifecycle.java | 4 ++-- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java index a39e09f03..a859846d9 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java @@ -21,8 +21,7 @@ /** * @author Olga Maciaszek-Sharma */ -public class LoadBalancerRequestAdapter extends DefaultRequest - implements LoadBalancerRequest { +public class LoadBalancerRequestAdapter extends DefaultRequest implements LoadBalancerRequest { private final LoadBalancerRequest delegate; @@ -30,6 +29,11 @@ public LoadBalancerRequestAdapter(LoadBalancerRequest delegate) { this.delegate = delegate; } + public LoadBalancerRequestAdapter(LoadBalancerRequest delegate, RC context) { + super(context); + this.delegate = delegate; + } + @Override public T apply(ServiceInstance instance) throws Exception { return delegate.apply(instance); diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java index ef1fc38b1..72824af95 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java @@ -67,7 +67,7 @@ public BlockingLoadBalancerClient(LoadBalancerClientFactory loadBalancerClientFa @Override public T execute(String serviceId, LoadBalancerRequest request) throws IOException { String hint = getHint(serviceId); - DefaultRequest lbRequest = new DefaultRequest<>( + LoadBalancerRequestAdapter lbRequest = new LoadBalancerRequestAdapter<>(request, new DefaultRequestContext(request, hint)); Set supportedLifecycleProcessors = LoadBalancerLifecycleValidator .getSupportedLifecycleProcessors( @@ -80,7 +80,7 @@ public T execute(String serviceId, LoadBalancerRequest request) throws IO new CompletionContext<>(CompletionContext.Status.DISCARD, new EmptyResponse(), lbRequest))); throw new IllegalStateException("No instances available for " + serviceId); } - return execute(serviceId, serviceInstance, new LoadBalancerRequestAdapter<>(request)); + return execute(serviceId, serviceInstance, lbRequest); } @Override @@ -95,6 +95,8 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala if (lbRequest.getContext() instanceof TimedRequestContext) { ((TimedRequestContext) lbRequest.getContext()).setRequestStartTime(System.nanoTime()); } + supportedLifecycleProcessors + .forEach(lifecycle -> lifecycle.onStartRequest(lbRequest, new DefaultResponse(serviceInstance))); try { T response = request.apply(serviceInstance); supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java index 592aef876..8d8e9ddca 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java @@ -109,8 +109,8 @@ public void onComplete(CompletionContext comple } private boolean requestHasBeenTimed(Object loadBalancerRequestContext) { - return loadBalancerRequestContext instanceof TimedRequestContext && (((TimedRequestContext) loadBalancerRequestContext) - .getRequestStartTime() != 0L); + return loadBalancerRequestContext instanceof TimedRequestContext + && (((TimedRequestContext) loadBalancerRequestContext).getRequestStartTime() != 0L); } } From 3f20e7e24c649fb913ece358fee93b8d1ac18bfa Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 15:07:36 +0100 Subject: [PATCH 08/19] Fix onStartRequest and onComplete calls for RetryLoadBalancerInterceptor. --- .../loadbalancer/RetryLoadBalancerInterceptor.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java index 8d2f48bae..c716b5eb1 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java @@ -120,13 +120,12 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body new RetryableRequestContext(null, new RequestData(request), hint))))); } } + LoadBalancerRequestAdapter lbRequest = new LoadBalancerRequestAdapter<>( + requestFactory.createRequest(request, body, execution), + new RetryableRequestContext(null, new RequestData(request), hint)); ServiceInstance finalServiceInstance = serviceInstance; - supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onStartRequest( - new DefaultRequest<>(new RetryableRequestContext(null, new RequestData(request), hint)), - new DefaultResponse(finalServiceInstance))); ClientHttpResponse response = RetryLoadBalancerInterceptor.this.loadBalancer.execute(serviceName, - finalServiceInstance, - new LoadBalancerRequestAdapter<>(requestFactory.createRequest(request, body, execution))); + finalServiceInstance, lbRequest); int statusCode = response.getRawStatusCode(); if (retryPolicy != null && retryPolicy.retryableStatusCode(statusCode)) { if (LOG.isDebugEnabled()) { From 3cc7b8a1e71defed0c5221fca111479eaad308a6 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 18:14:57 +0100 Subject: [PATCH 09/19] Only register timed request once. Add tests. --- .../loadbalancer/stats/LoadBalancerTags.java | 2 +- .../MicrometerStatsLoadBalancerLifecycle.java | 3 +- ...ometerStatsLoadBalancerLifecycleTests.java | 131 ++++++++++++++++++ 3 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java 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 6c961665f..6e3dd8f4a 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 @@ -54,7 +54,7 @@ static Iterable buildSuccessRequestTags(CompletionContext request, Response lb public void onComplete(CompletionContext completionContext) { long requestFinishedTimestamp = System.nanoTime(); if (CompletionContext.Status.DISCARD.equals(completionContext.status())) { - Counter.builder("loadbalancer.requests.discarded").tags(buildDiscardedRequestTags(completionContext)) + Counter.builder("loadbalancer.requests.discard").tags(buildDiscardedRequestTags(completionContext)) .register(meterRegistry).increment(); return; } @@ -99,6 +99,7 @@ public void onComplete(CompletionContext comple .record(requestFinishedTimestamp - ((TimedRequestContext) loadBalancerRequestContext).getRequestStartTime(), TimeUnit.NANOSECONDS); + return; } Timer.builder("loadbalancer.requests.success").tags(buildSuccessRequestTags(completionContext)) .register(meterRegistry) 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 new file mode 100644 index 000000000..383a9c34b --- /dev/null +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java @@ -0,0 +1,131 @@ +/* + * Copyright 2012-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.loadbalancer.stats; + +import java.net.URI; +import java.util.HashMap; + +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import org.junit.jupiter.api.Test; + +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.CompletionContext; +import org.springframework.cloud.client.loadbalancer.DefaultRequest; +import org.springframework.cloud.client.loadbalancer.DefaultResponse; +import org.springframework.cloud.client.loadbalancer.EmptyResponse; +import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.cloud.client.loadbalancer.RequestData; +import org.springframework.cloud.client.loadbalancer.RequestDataContext; +import org.springframework.cloud.client.loadbalancer.Response; +import org.springframework.cloud.client.loadbalancer.ResponseData; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; +import org.springframework.util.MultiValueMapAdapter; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Olga Maciaszek-Sharma + */ +class MicrometerStatsLoadBalancerLifecycleTests { + + MeterRegistry meterRegistry = new SimpleMeterRegistry(); + + MicrometerStatsLoadBalancerLifecycle statsLifecycle = new MicrometerStatsLoadBalancerLifecycle(meterRegistry); + + @Test + void shouldRecordSuccessfulTimedRequest() { + RequestData requestData = new RequestData(HttpMethod.GET, URI + .create("http://test.org/test"), new HttpHeaders(), new HttpHeaders(), new HashMap<>()); + 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); + statsLifecycle.onStartRequest(lbRequest, lbResponse); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) + .isEqualTo(1); + + statsLifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + + 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", "/test")); + } + + @Test + void shouldRecordFailedTimedRequest() { + RequestData requestData = new RequestData(HttpMethod.GET, URI + .create("http://test.org/test"), new HttpHeaders(), new HttpHeaders(), new HashMap<>()); + Request lbRequest = new DefaultRequest<>(new RequestDataContext(requestData)); + Response lbResponse = new DefaultResponse(new DefaultServiceInstance("test-1", "test", + "test.org", 8080, false, new HashMap<>())); + statsLifecycle.onStartRequest(lbRequest, lbResponse); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) + .isEqualTo(1); + + statsLifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.FAILED, new IllegalStateException(), lbResponse, lbRequest)); + + assertThat(meterRegistry.getMeters()).hasSize(2); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) + .isEqualTo(0); + assertThat(meterRegistry.get("loadbalancer.requests.failed").timers()).hasSize(1); + assertThat(meterRegistry.get("loadbalancer.requests.failed").timer().count()) + .isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.failed").timer().getId() + .getTags()) + .contains(Tag.of("exception", "IllegalStateException"), Tag + .of("method", "GET"), + Tag.of("serviceId", "test"), Tag + .of("serviceInstance.host", "test.org"), Tag + .of("serviceInstance.instanceId", "test-1"), + Tag.of("serviceInstance.port", "8080"), Tag.of("uri", "/test")); + } + + @Test + void shouldNotRecordDiscardedRequest() { + RequestData requestData = new RequestData(HttpMethod.GET, URI + .create("http://test.org/test"), new HttpHeaders(), new HttpHeaders(), new HashMap<>()); + Request lbRequest = new DefaultRequest<>(new RequestDataContext(requestData)); + Response lbResponse = new EmptyResponse(); + statsLifecycle.onStartRequest(lbRequest, lbResponse); + + + statsLifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse, lbRequest)); + assertThat(meterRegistry.getMeters()).hasSize(1); + assertThat(meterRegistry.get("loadbalancer.requests.discard").counter().count()) + .isEqualTo(1); + } +} From 9b1ff9149129d74d4bc2a6bbacce9e350ef1c700 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 18:41:48 +0100 Subject: [PATCH 10/19] Adjust tags logic. Add more tests. --- .../loadbalancer/stats/LoadBalancerTags.java | 12 ++- ...ometerStatsLoadBalancerLifecycleTests.java | 83 +++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) 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 6e3dd8f4a..4e607c5eb 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 @@ -32,7 +32,7 @@ */ final class LoadBalancerTags { - private static final String UNKNOWN = "UNKNOWN"; + static final String UNKNOWN = "UNKNOWN"; private LoadBalancerTags() { throw new UnsupportedOperationException("Cannot instantiate utility class"); @@ -53,8 +53,8 @@ static Iterable buildSuccessRequestTags(CompletionContext buildSuccessRequestTags(CompletionContext lbRequest = new DefaultRequest<>(new StatsTestContext()); + 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<>()), null); + statsLifecycle.onStartRequest(lbRequest, lbResponse); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) + .isEqualTo(1); + + + statsLifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + + assertThat(meterRegistry.getMeters()).hasSize(1); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) + .isEqualTo(0); + } + + @Test + void shouldNotCreateNullTagsWhenNullDataObjects() { + Request lbRequest = new DefaultRequest<>(new DefaultRequestContext()); + Response lbResponse = new DefaultResponse(new DefaultServiceInstance()); + statsLifecycle.onStartRequest(lbRequest, lbResponse); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) + .isEqualTo(1); + + statsLifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, null, lbRequest)); + + 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", UNKNOWN), Tag.of("outcome", UNKNOWN), Tag + .of("serviceId", UNKNOWN) + , Tag.of("serviceInstance.host", UNKNOWN), Tag + .of("serviceInstance.instanceId", UNKNOWN), + Tag.of("serviceInstance.port", "0"), Tag + .of("status", UNKNOWN), Tag.of("uri", UNKNOWN)); + } + + @Test + void shouldNotCreateNullTagsWhenEmptyDataObjects() { + RequestData requestData = new RequestData(null, null, null, null, null); + Request lbRequest = new DefaultRequest<>(new RequestDataContext()); + Response lbResponse = new DefaultResponse(new DefaultServiceInstance()); + ResponseData responseData = new ResponseData(null, null, null, requestData); + statsLifecycle.onStartRequest(lbRequest, lbResponse); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) + .isEqualTo(1); + + statsLifecycle + .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + + 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", UNKNOWN), Tag.of("outcome", "SUCCESS"), Tag + .of("serviceId", UNKNOWN) + , Tag.of("serviceInstance.host", UNKNOWN), Tag + .of("serviceInstance.instanceId", UNKNOWN), + Tag.of("serviceInstance.port", "0"), Tag + .of("status", "200"), Tag.of("uri", UNKNOWN)); + } + + private static class StatsTestContext { + + } } From 9318a06e175993ad625f9ceae0ed28ad5d78c724 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 19:03:52 +0100 Subject: [PATCH 11/19] Add more tests. --- .../RetryLoadBalancerInterceptorTests.java | 21 ++- ...adBalancerExchangeFilterFunctionTests.java | 23 ++- ...xchangeFilterFunctionIntegrationTests.java | 14 +- .../loadbalancer/stats/LoadBalancerTags.java | 3 +- .../BlockingLoadBalancerClientTests.java | 14 +- ...ometerStatsLoadBalancerLifecycleTests.java | 154 +++++++----------- 6 files changed, 123 insertions(+), 106 deletions(-) diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java index 55bf4b3fe..7bd033d49 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java @@ -373,16 +373,26 @@ public void shouldNotDuplicateLifecycleCalls() throws IOException, URISyntaxExce interceptor.intercept(request, new byte[] {}, mock(ClientHttpRequestExecution.class)); assertThat(((TestLoadBalancerLifecycle) lifecycleProcessors.get("testLifecycle")).getStartLog()).hasSize(1); + assertThat(((TestLoadBalancerLifecycle) lifecycleProcessors.get("testLifecycle")).getStartRequestLog()) + .hasSize(0); assertThat(((TestLoadBalancerLifecycle) lifecycleProcessors.get("testLifecycle")).getCompleteLog()).hasSize(0); assertThat(((TestLoadBalancerLifecycle) lifecycleProcessors.get("anotherLifecycle")).getStartLog()).hasSize(1); + assertThat(((TestLoadBalancerLifecycle) lifecycleProcessors.get("anotherLifecycle")).getStartRequestLog()) + .hasSize(0); assertThat(((TestLoadBalancerLifecycle) lifecycleProcessors.get("anotherLifecycle")).getCompleteLog()) .hasSize(0); assertThat(((TestLoadBalancerLifecycle) client.getLifecycleProcessors().get("testLifecycle")).getStartLog()) .hasSize(0); + assertThat( + ((TestLoadBalancerLifecycle) client.getLifecycleProcessors().get("testLifecycle")).getStartRequestLog()) + .hasSize(1); assertThat(((TestLoadBalancerLifecycle) client.getLifecycleProcessors().get("testLifecycle")).getCompleteLog()) .hasSize(1); assertThat(((TestLoadBalancerLifecycle) client.getLifecycleProcessors().get("anotherLifecycle")).getStartLog()) .hasSize(0); + assertThat( + ((TestLoadBalancerLifecycle) client.getLifecycleProcessors().get("testLifecycle")).getStartRequestLog()) + .hasSize(1); assertThat( ((TestLoadBalancerLifecycle) client.getLifecycleProcessors().get("anotherLifecycle")).getCompleteLog()) .hasSize(1); @@ -499,7 +509,8 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala Set supportedLoadBalancerProcessors = LoadBalancerLifecycleValidator .getSupportedLifecycleProcessors(lifecycleProcessors, DefaultRequestContext.class, Object.class, ServiceInstance.class); - + supportedLoadBalancerProcessors.forEach(lifecycle -> lifecycle.onStartRequest(new DefaultRequest<>(), + new DefaultResponse(serviceInstance))); T response = (T) new MockClientHttpResponse(new byte[] {}, HttpStatus.OK); supportedLoadBalancerProcessors .forEach(lifecycle -> lifecycle.onComplete(new CompletionContext(CompletionContext.Status.SUCCESS, @@ -532,6 +543,8 @@ protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycl final ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); + final ConcurrentHashMap> startRequestLog = new ConcurrentHashMap<>(); + final ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); @Override @@ -548,7 +561,7 @@ public void onStart(Request request) { @Override public void onStartRequest(Request request, Response lbResponse) { - + startRequestLog.put(getName() + UUID.randomUUID(), request); } @Override @@ -564,6 +577,10 @@ ConcurrentHashMap> ge return completeLog; } + ConcurrentHashMap> getStartRequestLog() { + return startRequestLog; + } + protected String getName() { return getClass().getSimpleName(); } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java index 06e2436a9..5ddcf7a84 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java @@ -129,19 +129,24 @@ void exceptionNotThrownWhenFactoryReturnsNullLifecycleProcessorsMap() { void loadBalancerLifecycleCallbacksExecuted() { final String callbackTestHint = "callbackTestHint"; loadBalancerProperties.getHint().put("testservice", "callbackTestHint"); - final String result = "callbackTestResult"; ClientResponse clientResponse = WebClient.builder().baseUrl("http://testservice").filter(loadBalancerFunction) .build().get().uri("/callback").exchange().block(); Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() .values(); + Collection> lifecycleStartedLogRequests = ((TestLoadBalancerLifecycle) factory + .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")) + .getStartRequestLog().values(); Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) .getCompleteLog().values(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) .contains(callbackTestHint); + assertThat(lifecycleStartedLogRequests) + .extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) + .contains(callbackTestHint); assertThat(anotherLifecycleLogRequests) .extracting(completionContext -> ((ResponseData) completionContext.getClientResponse()).getRequestData() .getUrl().toString()) @@ -205,9 +210,11 @@ LoadBalancerProperties loadBalancerProperties() { protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycle { - ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); + final Map> startLog = new ConcurrentHashMap<>(); + + final Map> startRequestLog = new ConcurrentHashMap<>(); - ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); + final Map> completeLog = new ConcurrentHashMap<>(); @Override public void onStart(Request request) { @@ -216,7 +223,7 @@ public void onStart(Request request) { @Override public void onStartRequest(Request request, Response lbResponse) { - + startRequestLog.put(getName() + UUID.randomUUID(), request); } @Override @@ -224,14 +231,18 @@ public void onComplete(CompletionContext comple completeLog.put(getName() + UUID.randomUUID(), completionContext); } - ConcurrentHashMap> getStartLog() { + Map> getStartLog() { return startLog; } - ConcurrentHashMap> getCompleteLog() { + Map> getCompleteLog() { return completeLog; } + Map> getStartRequestLog() { + return startRequestLog; + } + protected String getName() { return this.getClass().getSimpleName(); } diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java index 892008810..45b6c2ccd 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunctionIntegrationTests.java @@ -109,12 +109,18 @@ void loadBalancerLifecycleCallbacksExecuted() { Collection> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() .values(); + Collection> lifecycleLogStartRequests = ((TestLoadBalancerLifecycle) factory + .getInstances("testservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")) + .getStartRequestLog().values(); Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory .getInstances("testservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) .getCompleteLog().values(); then(clientResponse.statusCode()).isEqualTo(HttpStatus.OK); assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) .contains(callbackTestHint); + assertThat(lifecycleLogStartRequests) + .extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) + .contains(callbackTestHint); assertThat(anotherLifecycleLogRequests) .extracting(completionContext -> ((ResponseData) completionContext.getClientResponse()).getRequestData() .getHttpMethod()) @@ -271,6 +277,8 @@ protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycl Map> startLog = new ConcurrentHashMap<>(); + Map> startRequestLog = new ConcurrentHashMap<>(); + Map> completeLog = new ConcurrentHashMap<>(); @Override @@ -280,7 +288,7 @@ public void onStart(Request request) { @Override public void onStartRequest(Request request, Response lbResponse) { - + startRequestLog.put(getName() + UUID.randomUUID(), request); } @Override @@ -297,6 +305,10 @@ Map> getCompleteLog() return completeLog; } + Map> getStartRequestLog() { + return startRequestLog; + } + protected String getName() { return this.getClass().getSimpleName(); } 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 4e607c5eb..6513f7463 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 @@ -65,8 +65,7 @@ static Iterable buildSuccessRequestTags(CompletionContext> lifecycleLogRequests = ((TestLoadBalancerLifecycle) factory .getInstances("myservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")).getStartLog() .values(); + Collection> lifecycleLogStartedRequests = ((TestLoadBalancerLifecycle) factory + .getInstances("myservice", LoadBalancerLifecycle.class).get("loadBalancerLifecycle")) + .getStartRequestLog().values(); Collection> anotherLifecycleLogRequests = ((AnotherLoadBalancerLifecycle) factory .getInstances("myservice", LoadBalancerLifecycle.class).get("anotherLoadBalancerLifecycle")) .getCompleteLog().values(); assertThat(actualResult).isEqualTo(result); assertThat(lifecycleLogRequests).extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) .contains(callbackTestHint); + assertThat(lifecycleLogStartedRequests) + .extracting(request -> ((DefaultRequestContext) request.getContext()).getHint()) + .contains(callbackTestHint); assertThat(anotherLifecycleLogRequests).extracting(CompletionContext::getClientResponse).contains(result); } @@ -233,6 +239,8 @@ protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycl final ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); + final ConcurrentHashMap> startRequestLog = new ConcurrentHashMap<>(); + final ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); @Override @@ -242,7 +250,7 @@ public void onStart(Request request) { @Override public void onStartRequest(Request request, Response lbResponse) { - + startRequestLog.put(getName() + UUID.randomUUID(), request); } @Override @@ -258,6 +266,10 @@ ConcurrentHashMap> ge return completeLog; } + ConcurrentHashMap> getStartRequestLog() { + return startRequestLog; + } + protected String getName() { return this.getClass().getSimpleName(); } 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 32f398b3d..e455bb43a 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 @@ -55,99 +55,80 @@ class MicrometerStatsLoadBalancerLifecycleTests { @Test void shouldRecordSuccessfulTimedRequest() { - RequestData requestData = new RequestData(HttpMethod.GET, URI - .create("http://test.org/test"), new HttpHeaders(), new HttpHeaders(), new HashMap<>()); + RequestData requestData = new RequestData(HttpMethod.GET, URI.create("http://test.org/test"), new HttpHeaders(), + new HttpHeaders(), new HashMap<>()); 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); + 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); statsLifecycle.onStartRequest(lbRequest, lbResponse); - assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) - .isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); - statsLifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + statsLifecycle.onComplete( + new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); 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", "/test")); + 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", "/test")); } @Test void shouldRecordFailedTimedRequest() { - RequestData requestData = new RequestData(HttpMethod.GET, URI - .create("http://test.org/test"), new HttpHeaders(), new HttpHeaders(), new HashMap<>()); + RequestData requestData = new RequestData(HttpMethod.GET, URI.create("http://test.org/test"), new HttpHeaders(), + new HttpHeaders(), new HashMap<>()); Request lbRequest = new DefaultRequest<>(new RequestDataContext(requestData)); - Response lbResponse = new DefaultResponse(new DefaultServiceInstance("test-1", "test", - "test.org", 8080, false, new HashMap<>())); + Response lbResponse = new DefaultResponse( + new DefaultServiceInstance("test-1", "test", "test.org", 8080, false, new HashMap<>())); statsLifecycle.onStartRequest(lbRequest, lbResponse); - assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) - .isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); - statsLifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.FAILED, new IllegalStateException(), lbResponse, lbRequest)); + statsLifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.FAILED, new IllegalStateException(), + lbResponse, lbRequest)); assertThat(meterRegistry.getMeters()).hasSize(2); - assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) - .isEqualTo(0); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0); assertThat(meterRegistry.get("loadbalancer.requests.failed").timers()).hasSize(1); - assertThat(meterRegistry.get("loadbalancer.requests.failed").timer().count()) - .isEqualTo(1); - assertThat(meterRegistry.get("loadbalancer.requests.failed").timer().getId() - .getTags()) - .contains(Tag.of("exception", "IllegalStateException"), Tag - .of("method", "GET"), - Tag.of("serviceId", "test"), Tag - .of("serviceInstance.host", "test.org"), Tag - .of("serviceInstance.instanceId", "test-1"), - Tag.of("serviceInstance.port", "8080"), Tag.of("uri", "/test")); + assertThat(meterRegistry.get("loadbalancer.requests.failed").timer().count()).isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.failed").timer().getId().getTags()).contains( + Tag.of("exception", "IllegalStateException"), Tag.of("method", "GET"), Tag.of("serviceId", "test"), + Tag.of("serviceInstance.host", "test.org"), Tag.of("serviceInstance.instanceId", "test-1"), + Tag.of("serviceInstance.port", "8080"), Tag.of("uri", "/test")); } @Test void shouldNotRecordDiscardedRequest() { - RequestData requestData = new RequestData(HttpMethod.GET, URI - .create("http://test.org/test"), new HttpHeaders(), new HttpHeaders(), new HashMap<>()); + RequestData requestData = new RequestData(HttpMethod.GET, URI.create("http://test.org/test"), new HttpHeaders(), + new HttpHeaders(), new HashMap<>()); Request lbRequest = new DefaultRequest<>(new RequestDataContext(requestData)); Response lbResponse = new EmptyResponse(); statsLifecycle.onStartRequest(lbRequest, lbResponse); - - statsLifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse, lbRequest)); + statsLifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse, lbRequest)); assertThat(meterRegistry.getMeters()).hasSize(1); - assertThat(meterRegistry.get("loadbalancer.requests.discard").counter().count()) - .isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.discard").counter().count()).isEqualTo(1); } @Test void shouldNotRecordUnTimedRequest() { Request lbRequest = new DefaultRequest<>(new StatsTestContext()); - 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<>()), null); + 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<>()), null); statsLifecycle.onStartRequest(lbRequest, lbResponse); - assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) - .isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); - - statsLifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + statsLifecycle.onComplete( + new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); assertThat(meterRegistry.getMeters()).hasSize(1); - assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) - .isEqualTo(0); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0); } @Test @@ -155,27 +136,19 @@ void shouldNotCreateNullTagsWhenNullDataObjects() { Request lbRequest = new DefaultRequest<>(new DefaultRequestContext()); Response lbResponse = new DefaultResponse(new DefaultServiceInstance()); statsLifecycle.onStartRequest(lbRequest, lbResponse); - assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) - .isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); statsLifecycle .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, null, lbRequest)); 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", UNKNOWN), Tag.of("outcome", UNKNOWN), Tag - .of("serviceId", UNKNOWN) - , Tag.of("serviceInstance.host", UNKNOWN), Tag - .of("serviceInstance.instanceId", UNKNOWN), - Tag.of("serviceInstance.port", "0"), Tag - .of("status", UNKNOWN), Tag.of("uri", UNKNOWN)); + 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", UNKNOWN), Tag.of("outcome", UNKNOWN), Tag.of("serviceId", UNKNOWN), + Tag.of("serviceInstance.host", UNKNOWN), Tag.of("serviceInstance.instanceId", UNKNOWN), + Tag.of("serviceInstance.port", "0"), Tag.of("status", UNKNOWN), Tag.of("uri", UNKNOWN)); } @Test @@ -185,30 +158,23 @@ void shouldNotCreateNullTagsWhenEmptyDataObjects() { Response lbResponse = new DefaultResponse(new DefaultServiceInstance()); ResponseData responseData = new ResponseData(null, null, null, requestData); statsLifecycle.onStartRequest(lbRequest, lbResponse); - assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()) - .isEqualTo(1); + assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); - statsLifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + statsLifecycle.onComplete( + new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); 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", UNKNOWN), Tag.of("outcome", "SUCCESS"), Tag - .of("serviceId", UNKNOWN) - , Tag.of("serviceInstance.host", UNKNOWN), Tag - .of("serviceInstance.instanceId", UNKNOWN), - Tag.of("serviceInstance.port", "0"), Tag - .of("status", "200"), Tag.of("uri", UNKNOWN)); + 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", UNKNOWN), Tag.of("outcome", "SUCCESS"), Tag.of("serviceId", UNKNOWN), + Tag.of("serviceInstance.host", UNKNOWN), Tag.of("serviceInstance.instanceId", UNKNOWN), + Tag.of("serviceInstance.port", "0"), Tag.of("status", "200"), Tag.of("uri", UNKNOWN)); } private static class StatsTestContext { } + } From b089d9ff24919088b116b5010c1037f0543c848a Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 19:23:39 +0100 Subject: [PATCH 12/19] Refactor. Add javadocs. --- .../loadbalancer/LoadBalancerLifecycle.java | 4 +-- .../LoadBalancerRequestAdapter.java | 4 +++ .../RetryLoadBalancerInterceptor.java | 4 +-- .../loadbalancer/TimedRequestContext.java | 3 ++ .../LoadBalancerStatsAutoConfiguration.java | 3 ++ .../loadbalancer/stats/LoadBalancerTags.java | 29 +++++++++++++++++-- .../MicrometerStatsLoadBalancerLifecycle.java | 4 +++ .../BlockingLoadBalancerClientTests.java | 13 +++++---- ...ometerStatsLoadBalancerLifecycleTests.java | 2 ++ 9 files changed, 52 insertions(+), 14 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java index e78505eaa..4ba9240b5 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerLifecycle.java @@ -16,8 +16,6 @@ package org.springframework.cloud.client.loadbalancer; -import org.springframework.cloud.client.ServiceInstance; - /** * Allows to define actions that should be carried out before and after load-balancing. * @@ -54,7 +52,7 @@ default boolean supports(Class requestContextClass, Class responseClass, Class s * a service instance * @param lbResponse the {@link Response} returned by the LoadBalancer */ - void onStartRequest(Request request, Response lbResponse); + void onStartRequest(Request request, Response lbResponse); /** * A callback method executed after load-balancing. diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java index a859846d9..038ffa759 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerRequestAdapter.java @@ -19,7 +19,11 @@ import org.springframework.cloud.client.ServiceInstance; /** + * An adapter class that allows creating {@link Request} objects from previously + * {@link LoadBalancerRequest} objects. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ public class LoadBalancerRequestAdapter extends DefaultRequest implements LoadBalancerRequest { diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java index c716b5eb1..75111a536 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java @@ -89,7 +89,7 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body Set supportedLifecycleProcessors = LoadBalancerLifecycleValidator .getSupportedLifecycleProcessors( loadBalancerFactory.getInstances(serviceName, LoadBalancerLifecycle.class), - RequestDataContext.class, ResponseData.class, ServiceInstance.class); + RetryableRequestContext.class, ResponseData.class, ServiceInstance.class); String hint = getHint(serviceName); if (serviceInstance == null) { if (LOG.isDebugEnabled()) { @@ -115,7 +115,7 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body Response lbResponse = new DefaultResponse(serviceInstance); if (serviceInstance == null) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext( + .onComplete(new CompletionContext( CompletionContext.Status.DISCARD, lbResponse, new DefaultRequest<>( new RetryableRequestContext(null, new RequestData(request), hint))))); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java index b9e257fac..13f8eb90c 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/TimedRequestContext.java @@ -17,7 +17,10 @@ package org.springframework.cloud.client.loadbalancer; /** + * Allows setting and retrieving request start time. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ public interface TimedRequestContext { 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 7478a3966..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 @@ -26,7 +26,10 @@ import org.springframework.context.annotation.Configuration; /** + * Autoconfiguration that provides a {@link MicrometerStatsLoadBalancerLifecycle} bean. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ @Configuration(proxyBeanMethods = false) @ConditionalOnClass(MeterRegistry.class) 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 6513f7463..4dd8d452a 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 @@ -19,7 +19,6 @@ import io.micrometer.core.instrument.Tag; import io.micrometer.core.instrument.Tags; -import org.springframework.boot.actuate.metrics.http.Outcome; import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.loadbalancer.CompletionContext; import org.springframework.cloud.client.loadbalancer.RequestData; @@ -28,7 +27,10 @@ import org.springframework.util.StringUtils; /** + * Utility class for building metrics tags for load-balanced calls. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ final class LoadBalancerTags { @@ -53,7 +55,7 @@ static Iterable buildSuccessRequestTags(CompletionContext= 100 && status < 200) { + return "INFORMATIONAL"; + } + else if (status >= 200 && status < 300) { + return "SUCCESS"; + } + else if (status >= 300 && status < 400) { + return "REDIRECTION"; + } + else if (status >= 400 && status < 500) { + return "CLIENT_ERROR"; + } + else if (status >= 500 && status < 600) { + return "SERVER_ERROR"; + } + return UNKNOWN; + } + } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java index fe2aa993c..fe7daf0e9 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycle.java @@ -38,7 +38,11 @@ import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.buildSuccessRequestTags; /** + * An implementation of {@link LoadBalancerLifecycle} that records metrics for + * load-balanced calls. + * * @author Olga Maciaszek-Sharma + * @since 3.0.0 */ public class MicrometerStatsLoadBalancerLifecycle implements LoadBalancerLifecycle { diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java index 307da1cb0..8d0e623c1 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClientTests.java @@ -20,6 +20,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Random; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; @@ -237,11 +238,11 @@ ReactorLoadBalancer reactiveLoadBalancer(DiscoveryClient discov protected static class TestLoadBalancerLifecycle implements LoadBalancerLifecycle { - final ConcurrentHashMap> startLog = new ConcurrentHashMap<>(); + final Map> startLog = new ConcurrentHashMap<>(); - final ConcurrentHashMap> startRequestLog = new ConcurrentHashMap<>(); + final Map> startRequestLog = new ConcurrentHashMap<>(); - final ConcurrentHashMap> completeLog = new ConcurrentHashMap<>(); + final Map> completeLog = new ConcurrentHashMap<>(); @Override public void onStart(Request request) { @@ -258,15 +259,15 @@ public void onComplete(CompletionContext comple completeLog.put(getName() + UUID.randomUUID(), completionContext); } - ConcurrentHashMap> getStartLog() { + Map> getStartLog() { return startLog; } - ConcurrentHashMap> getCompleteLog() { + Map> getCompleteLog() { return completeLog; } - ConcurrentHashMap> getStartRequestLog() { + Map> getStartRequestLog() { return startRequestLog; } 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 e455bb43a..c2495b6ee 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 @@ -45,6 +45,8 @@ import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.UNKNOWN; /** + * Tests for {@link MicrometerStatsLoadBalancerLifecycle}. + * * @author Olga Maciaszek-Sharma */ class MicrometerStatsLoadBalancerLifecycleTests { From b73341dd7713ad4d9e3b460029d3ed60b9d1fc32 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 19:27:25 +0100 Subject: [PATCH 13/19] Refactor. --- .../reactive/RetryableLoadBalancerExchangeFilterFunction.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index 47a15ba2a..b8dbdf20a 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -39,7 +39,6 @@ import org.springframework.cloud.client.loadbalancer.LoadBalancerProperties; import org.springframework.cloud.client.loadbalancer.Request; import org.springframework.cloud.client.loadbalancer.RequestData; -import org.springframework.cloud.client.loadbalancer.RequestDataContext; import org.springframework.cloud.client.loadbalancer.Response; import org.springframework.cloud.client.loadbalancer.ResponseData; import org.springframework.cloud.client.loadbalancer.RetryableRequestContext; @@ -102,7 +101,7 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction Set supportedLifecycleProcessors = LoadBalancerLifecycleValidator .getSupportedLifecycleProcessors( loadBalancerFactory.getInstances(serviceId, LoadBalancerLifecycle.class), - RequestDataContext.class, ResponseData.class, ServiceInstance.class); + RetryableRequestContext.class, ResponseData.class, ServiceInstance.class); String hint = getHint(serviceId, properties.getHint()); RequestData requestData = new RequestData(clientRequest); DefaultRequest lbRequest = new DefaultRequest<>( From af8d2c60aa4060d30bd5f5e47ec9871f248c1326 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 19:28:15 +0100 Subject: [PATCH 14/19] Refactor. --- .../cloud/loadbalancer/stats/LoadBalancerTags.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 4dd8d452a..7679f2868 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 @@ -131,13 +131,13 @@ private static Tag valueOrUnknown(String key, Object value) { private static Tag exception(Throwable exception) { if (exception != null) { String simpleName = exception.getClass().getSimpleName(); - return Tag.of("exception", StringUtils - .hasText(simpleName) ? simpleName : exception.getClass().getName()); + return Tag.of("exception", StringUtils.hasText(simpleName) ? simpleName : exception.getClass().getName()); } return Tag.of("exception", "None"); } - // Logic from Actuator's `Outcome` class. Copied in here to avoid adding Actuator dependency. + // Logic from Actuator's `Outcome` class. Copied in here to avoid adding Actuator + // dependency. public static String forStatus(int status) { if (status >= 100 && status < 200) { return "INFORMATIONAL"; From 8fba4d42ab7e5882893f20e3cb5d81d86d37d671 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 20:34:39 +0100 Subject: [PATCH 15/19] Retrieve client response data if possible in BlockingLoadBalancerClient. --- .../AsyncLoadBalancerInterceptor.java | 13 ++----- .../client/loadbalancer/ResponseData.java | 39 ++++++++++++++++--- .../client/BlockingLoadBalancerClient.java | 23 ++++++++++- 3 files changed, 59 insertions(+), 16 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/AsyncLoadBalancerInterceptor.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/AsyncLoadBalancerInterceptor.java index 2be38a1f0..6d00965df 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/AsyncLoadBalancerInterceptor.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/AsyncLoadBalancerInterceptor.java @@ -19,7 +19,6 @@ import java.io.IOException; import java.net.URI; -import org.springframework.cloud.client.ServiceInstance; import org.springframework.http.HttpRequest; import org.springframework.http.client.AsyncClientHttpRequestExecution; import org.springframework.http.client.AsyncClientHttpRequestInterceptor; @@ -42,14 +41,10 @@ public ListenableFuture intercept(final HttpRequest request, final AsyncClientHttpRequestExecution execution) throws IOException { final URI originalUri = request.getURI(); String serviceName = originalUri.getHost(); - return this.loadBalancer.execute(serviceName, new LoadBalancerRequest>() { - @Override - public ListenableFuture apply(final ServiceInstance instance) throws Exception { - HttpRequest serviceRequest = new ServiceRequestWrapper(request, instance, - AsyncLoadBalancerInterceptor.this.loadBalancer); - return execution.executeAsync(serviceRequest, body); - } - + return this.loadBalancer.execute(serviceName, instance -> { + HttpRequest serviceRequest = new ServiceRequestWrapper(request, instance, + AsyncLoadBalancerInterceptor.this.loadBalancer); + return execution.executeAsync(serviceRequest, body); }); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/ResponseData.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/ResponseData.java index f6583cf6e..740685e4f 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/ResponseData.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/ResponseData.java @@ -16,13 +16,18 @@ package org.springframework.cloud.client.loadbalancer; +import java.io.IOException; +import java.util.Collections; +import java.util.List; import java.util.Objects; import org.springframework.core.style.ToStringCreator; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseCookie; +import org.springframework.http.client.ClientHttpResponse; import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.web.reactive.function.client.ClientResponse; @@ -59,6 +64,11 @@ public ResponseData(ServerHttpResponse response, RequestData requestData) { this(response.getStatusCode(), response.getHeaders(), response.getCookies(), requestData); } + public ResponseData(ClientHttpResponse clientHttpResponse, RequestData requestData) throws IOException { + this(clientHttpResponse.getStatusCode(), clientHttpResponse.getHeaders(), + buildCookiesFromHeaders(clientHttpResponse.getHeaders()), requestData); + } + public HttpStatus getHttpStatus() { return httpStatus; } @@ -82,6 +92,30 @@ public String toString() { return to.toString(); } + static MultiValueMap buildCookiesFromHeaders(HttpHeaders headers) { + LinkedMultiValueMap newCookies = new LinkedMultiValueMap<>(); + if (headers == null) { + return newCookies; + } + List cookiesFromHeaders = headers.get(HttpHeaders.COOKIE); + if (cookiesFromHeaders != null) { + cookiesFromHeaders.forEach(cookie -> { + String[] splitCookie = cookie.split("="); + if (splitCookie.length < 2) { + return; + } + newCookies.put(splitCookie[0], + Collections.singletonList(ResponseCookie.from(splitCookie[0], splitCookie[1]).build())); + }); + } + return newCookies; + } + + @Override + public int hashCode() { + return Objects.hash(httpStatus, headers, cookies, requestData); + } + @Override public boolean equals(Object o) { if (this == o) { @@ -95,9 +129,4 @@ public boolean equals(Object o) { && Objects.equals(cookies, that.cookies) && Objects.equals(requestData, that.requestData); } - @Override - public int hashCode() { - return Objects.hash(httpStatus, headers, cookies, requestData); - } - } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java index 72824af95..f23564885 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java @@ -37,9 +37,11 @@ import org.springframework.cloud.client.loadbalancer.LoadBalancerUriTools; import org.springframework.cloud.client.loadbalancer.Request; import org.springframework.cloud.client.loadbalancer.Response; +import org.springframework.cloud.client.loadbalancer.ResponseData; import org.springframework.cloud.client.loadbalancer.TimedRequestContext; import org.springframework.cloud.client.loadbalancer.reactive.ReactiveLoadBalancer; import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; +import org.springframework.http.client.ClientHttpResponse; import org.springframework.util.ReflectionUtils; import static org.springframework.cloud.client.loadbalancer.reactive.ReactiveLoadBalancer.REQUEST; @@ -99,8 +101,10 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala .forEach(lifecycle -> lifecycle.onStartRequest(lbRequest, new DefaultResponse(serviceInstance))); try { T response = request.apply(serviceInstance); - supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.SUCCESS, defaultResponse, response, lbRequest))); + Object clientResponse = getClientResponse(response); + supportedLifecycleProcessors + .forEach(lifecycle -> lifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, + defaultResponse, clientResponse, lbRequest))); return response; } catch (IOException iOException) { @@ -116,6 +120,21 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala return null; } + private Object getClientResponse(T response) { + ClientHttpResponse clientHttpResponse = null; + if (response instanceof ClientHttpResponse) { + clientHttpResponse = (ClientHttpResponse) response; + } + if (clientHttpResponse != null) { + try { + return new ResponseData(clientHttpResponse, null); + } + catch (IOException ignored) { + } + } + return response; + } + @Override public URI reconstructURI(ServiceInstance serviceInstance, URI original) { return LoadBalancerUriTools.reconstructURI(serviceInstance, original); From 0960c6aff0ab53383a5ba4f1702184c69f4c912a Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 17 Dec 2020 20:37:53 +0100 Subject: [PATCH 16/19] Refactor. --- .../main/asciidoc/spring-cloud-commons.adoc | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index 679e9e72c..e13040285 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -1118,15 +1118,38 @@ public class MyConfiguration { One type of bean that it may be useful to register using <> is `LoadBalancerLifecycle`. -The LoadBalancerLifecycle beans provide callback methods, named `onStart(Request request)` and `onComplete(CompletionContext completionContext)`, that you should implement to specify what actions should take place before and after load-balancing. +The LoadBalancerLifecycle beans provide callback methods, named `onStart(Request request)`, `onStartRequest(Request request, Response lbResponse)` and `onComplete(CompletionContext completionContext)`, that you should implement to specify what actions should take place before and after load-balancing. -`onStart(Request request)` takes a `Request` object as a parameter. It contains data that is used to select an appropriate instance, including the downstream client request and <>. On the other hand, a `CompletionContext` object is provided to the `onComplete(CompletionContext completionContext)` method. It contains the LoadBalancer `Response`, including the selected service instance, the `Status` of the request executed against that service instance and (if available) the response returned to the downstream client, and (if an exception has occurred) the corresponding `Throwable`. +`onStart(Request request)` takes a `Request` object as a parameter. It contains data that is used to select an appropriate instance, including the downstream client request and <>. `onStartRequest` also takes the `Request` object, and additionally the `Response` object as parameters. On the other hand, a `CompletionContext` object is provided to the `onComplete(CompletionContext completionContext)` method. It contains the LoadBalancer `Response`, including the selected service instance, the `Status` of the request executed against that service instance and (if available) the response returned to the downstream client, and (if an exception has occurred) the corresponding `Throwable`. The `supports(Class requestContextClass, Class responseClass, Class serverTypeClass)` method can be used to determine whether the processor in question handles objects of provided types. If not overridden by the user, it returns `true`. NOTE: In the preceding method calls, `RC` means `RequestContext` type, `RES` means client response type, and `T` means returned server type. +[[loadbalancer-micrometer-stats-lifecycle]] +=== Spring Cloud LoadBalancer Statistics + +A `LoadBalancerLifecycle` bean that we provide out of the box is `MicrometerStatsLoadBalancerLifecycle` that uses Micrometer to provide statistics for load-balanced calls. + +In order to get this bean added to your application context, +set the value of the `spring.cloud.loadbalancer.stats.micrometer.enabled` to `true` and have a `MeterRegistry` available (for example, by adding https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-features.html[Spring Boot Actuator] to your project). + +`MicrometerStatsLoadBalancerLifecycle` registers the following meters in `MeterRegistry`: + +* `loadbalancer.requests.active` gauge that allows you to monitor the number of currently active requests for any service instance (service instance data available via tags); +* `loadbalancer.requests.success` timer that measures the time of execution of any load-balanced requests that have ended in passing a response on to the underlying client; +* `loadbalancer.requests.failed` timer that measures the time of execution of any load-balanced requests that have ended with an exception; +* `loadbalancer.requests.discard` counter that measures the number of discarded load-balanced requests, i.e. requests where a service instance to run the request on has not been retrieved by the LoadBalancer. + +Additional information regarding the service instances, request data and response data is added to metrics via tags whenever available. + +NOTE: For some implementations, such as `BlockingLoadBalancerClient` request and response data might not be available, as we establish generic types from arguments and might not be able to determine the types and read the data. + +NOTE: The meters will be registered in the registry when at least one record is added for a given meter. + +TIP: You can further configure the behaviour of those metrics (for example, add response[publishing percentiles and histograms]) by https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-features.html#production-ready-metrics-per-meter-properties[adding `MeterFilters`]. + == Spring Cloud Circuit Breaker include::spring-cloud-circuitbreaker.adoc[leveloffset=+1] From 6df4398238c81fc1cc10b1d701f6d2e4bc4f136b Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 18 Dec 2020 12:45:08 +0100 Subject: [PATCH 17/19] Fix docs after review. --- .../main/asciidoc/spring-cloud-commons.adoc | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index e13040285..10a71e6fa 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -1118,9 +1118,9 @@ public class MyConfiguration { One type of bean that it may be useful to register using <> is `LoadBalancerLifecycle`. -The LoadBalancerLifecycle beans provide callback methods, named `onStart(Request request)`, `onStartRequest(Request request, Response lbResponse)` and `onComplete(CompletionContext completionContext)`, that you should implement to specify what actions should take place before and after load-balancing. +The `LoadBalancerLifecycle` beans provide callback methods, named `onStart(Request request)`, `onStartRequest(Request request, Response lbResponse)` and `onComplete(CompletionContext completionContext)`, that you should implement to specify what actions should take place before and after load-balancing. -`onStart(Request request)` takes a `Request` object as a parameter. It contains data that is used to select an appropriate instance, including the downstream client request and <>. `onStartRequest` also takes the `Request` object, and additionally the `Response` object as parameters. On the other hand, a `CompletionContext` object is provided to the `onComplete(CompletionContext completionContext)` method. It contains the LoadBalancer `Response`, including the selected service instance, the `Status` of the request executed against that service instance and (if available) the response returned to the downstream client, and (if an exception has occurred) the corresponding `Throwable`. +`onStart(Request request)` takes a `Request` object as a parameter. It contains data that is used to select an appropriate instance, including the downstream client request and <>. `onStartRequest` also takes the `Request` object and, additionally, the `Response` object as parameters. On the other hand, a `CompletionContext` object is provided to the `onComplete(CompletionContext completionContext)` method. It contains the LoadBalancer `Response`, including the selected service instance, the `Status` of the request executed against that service instance and (if available) the response returned to the downstream client, and (if an exception has occurred) the corresponding `Throwable`. The `supports(Class requestContextClass, Class responseClass, Class serverTypeClass)` method can be used to determine whether the processor in question handles objects of provided types. If not overridden by the user, it returns `true`. @@ -1130,25 +1130,25 @@ NOTE: In the preceding method calls, `RC` means `RequestContext` type, `RES` mea [[loadbalancer-micrometer-stats-lifecycle]] === Spring Cloud LoadBalancer Statistics -A `LoadBalancerLifecycle` bean that we provide out of the box is `MicrometerStatsLoadBalancerLifecycle` that uses Micrometer to provide statistics for load-balanced calls. +We provide a `LoadBalancerLifecycle` bean called `MicrometerStatsLoadBalancerLifecycle`, which uses Micrometer to provide statistics for load-balanced calls. In order to get this bean added to your application context, set the value of the `spring.cloud.loadbalancer.stats.micrometer.enabled` to `true` and have a `MeterRegistry` available (for example, by adding https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-features.html[Spring Boot Actuator] to your project). `MicrometerStatsLoadBalancerLifecycle` registers the following meters in `MeterRegistry`: -* `loadbalancer.requests.active` gauge that allows you to monitor the number of currently active requests for any service instance (service instance data available via tags); -* `loadbalancer.requests.success` timer that measures the time of execution of any load-balanced requests that have ended in passing a response on to the underlying client; -* `loadbalancer.requests.failed` timer that measures the time of execution of any load-balanced requests that have ended with an exception; -* `loadbalancer.requests.discard` counter that measures the number of discarded load-balanced requests, i.e. requests where a service instance to run the request on has not been retrieved by the LoadBalancer. +* `loadbalancer.requests.active`: A gauge that allows you to monitor the number of currently active requests for any service instance (service instance data available via tags); +* `loadbalancer.requests.success`: A timer that measures the time of execution of any load-balanced requests that have ended in passing a response on to the underlying client; +* `loadbalancer.requests.failed`: A timer that measures the time of execution of any load-balanced requests that have ended with an exception; +* `loadbalancer.requests.discard`: A counter that measures the number of discarded load-balanced requests, i.e. requests where a service instance to run the request on has not been retrieved by the LoadBalancer. -Additional information regarding the service instances, request data and response data is added to metrics via tags whenever available. +Additional information regarding the service instances, request data, and response data is added to metrics via tags whenever available. -NOTE: For some implementations, such as `BlockingLoadBalancerClient` request and response data might not be available, as we establish generic types from arguments and might not be able to determine the types and read the data. +NOTE: For some implementations, such as `BlockingLoadBalancerClient`, request and response data might not be available, as we establish generic types from arguments and might not be able to determine the types and read the data. -NOTE: The meters will be registered in the registry when at least one record is added for a given meter. +NOTE: The meters are registered in the registry when at least one record is added for a given meter. -TIP: You can further configure the behaviour of those metrics (for example, add response[publishing percentiles and histograms]) by https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-features.html#production-ready-metrics-per-meter-properties[adding `MeterFilters`]. +TIP: You can further configure the behavior of those metrics (for example, add https://micrometer.io/docs/concepts#_histograms_and_percentiles[publishing percentiles and histograms]) by https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-features.html#production-ready-metrics-per-meter-properties[adding `MeterFilters`]. == Spring Cloud Circuit Breaker From d89252b8d412584f0c552ada33a93d5ea75c34e4 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 18 Dec 2020 15:17:32 +0100 Subject: [PATCH 18/19] Make previousServiceInstanceMutable. --- .../cloud/client/loadbalancer/RetryableRequestContext.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java index 79ec07a3b..a2ad0c0cb 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryableRequestContext.java @@ -29,7 +29,7 @@ */ public class RetryableRequestContext extends RequestDataContext { - private final ServiceInstance previousServiceInstance; + private ServiceInstance previousServiceInstance; public RetryableRequestContext(ServiceInstance previousServiceInstance) { this.previousServiceInstance = previousServiceInstance; @@ -50,6 +50,10 @@ public ServiceInstance getPreviousServiceInstance() { return previousServiceInstance; } + public void setPreviousServiceInstance(ServiceInstance previousServiceInstance) { + this.previousServiceInstance = previousServiceInstance; + } + @Override public String toString() { ToStringCreator to = new ToStringCreator(this); From 5d1ec8c4c0cf9fb8f0b8128b7ecb4aa47c2dabfc Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Fri, 18 Dec 2020 16:52:14 +0100 Subject: [PATCH 19/19] Change argument order for CompletionContext constructors. Remove duplicated start time setting. --- .../loadbalancer/CompletionContext.java | 24 +++++++++---------- .../RetryLoadBalancerInterceptor.java | 6 +++-- ...torLoadBalancerExchangeFilterFunction.java | 6 ++--- ...bleLoadBalancerExchangeFilterFunction.java | 6 ++--- .../RetryLoadBalancerInterceptorTests.java | 2 +- .../client/BlockingLoadBalancerClient.java | 12 ++++------ ...ometerStatsLoadBalancerLifecycleTests.java | 12 +++++----- 7 files changed, 33 insertions(+), 35 deletions(-) diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java index 639ded145..dfbdb7c36 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/CompletionContext.java @@ -38,30 +38,30 @@ public class CompletionContext { private final Request loadBalancerRequest; public CompletionContext(Status status, Request loadBalancerRequest) { - this(status, null, null, null, loadBalancerRequest); + this(status, null, loadBalancerRequest, null, null); } - public CompletionContext(Status status, Response response, Request loadBalancerRequest) { - this(status, null, response, null, loadBalancerRequest); + public CompletionContext(Status status, Request loadBalancerRequest, Response response) { + this(status, null, loadBalancerRequest, response, null); } - public CompletionContext(Status status, Throwable throwable, Response loadBalancerResponse, - Request loadBalancerRequest) { - this(status, throwable, loadBalancerResponse, null, loadBalancerRequest); + public CompletionContext(Status status, Throwable throwable, Request loadBalancerRequest, + Response loadBalancerResponse) { + this(status, throwable, loadBalancerRequest, loadBalancerResponse, null); } - public CompletionContext(Status status, Response loadBalancerResponse, RES clientResponse, - Request loadBalancerRequest) { - this(status, null, loadBalancerResponse, clientResponse, loadBalancerRequest); + public CompletionContext(Status status, Request loadBalancerRequest, Response loadBalancerResponse, + RES clientResponse) { + this(status, null, loadBalancerRequest, loadBalancerResponse, clientResponse); } - public CompletionContext(Status status, Throwable throwable, Response loadBalancerResponse, RES clientResponse, - Request loadBalancerRequest) { + public CompletionContext(Status status, Throwable throwable, Request loadBalancerRequest, + Response loadBalancerResponse, RES clientResponse) { this.status = status; this.throwable = throwable; + this.loadBalancerRequest = loadBalancerRequest; this.loadBalancerResponse = loadBalancerResponse; this.clientResponse = clientResponse; - this.loadBalancerRequest = loadBalancerRequest; } public Status status() { diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java index 75111a536..e56922b72 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java @@ -116,8 +116,10 @@ public ClientHttpResponse intercept(final HttpRequest request, final byte[] body if (serviceInstance == null) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle .onComplete(new CompletionContext( - CompletionContext.Status.DISCARD, lbResponse, new DefaultRequest<>( - new RetryableRequestContext(null, new RequestData(request), hint))))); + CompletionContext.Status.DISCARD, + new DefaultRequest<>( + new RetryableRequestContext(null, new RequestData(request), hint)), + lbResponse))); } } LoadBalancerRequestAdapter lbRequest = new LoadBalancerRequestAdapter<>( diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java index f63bd6c0e..256f0bb66 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunction.java @@ -94,7 +94,7 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction LOG.warn(message); } supportedLifecycleProcessors.forEach(lifecycle -> lifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse, lbRequest))); + .onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbRequest, lbResponse))); return Mono.just(ClientResponse.create(HttpStatus.SERVICE_UNAVAILABLE) .body(serviceInstanceUnavailableMessage(serviceId)).build()); } @@ -111,10 +111,10 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction return next.exchange(newRequest) .doOnError(throwable -> supportedLifecycleProcessors.forEach(lifecycle -> lifecycle .onComplete(new CompletionContext( - CompletionContext.Status.FAILED, throwable, lbResponse, lbRequest)))) + CompletionContext.Status.FAILED, throwable, lbRequest, lbResponse)))) .doOnSuccess(clientResponse -> supportedLifecycleProcessors.forEach( lifecycle -> lifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, - lbResponse, new ResponseData(clientResponse, requestData), lbRequest)))); + lbRequest, lbResponse, new ResponseData(clientResponse, requestData))))); }); } diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java index b8dbdf20a..4d4d7233e 100644 --- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java +++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java @@ -117,7 +117,7 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction } supportedLifecycleProcessors.forEach(lifecycle -> lifecycle .onComplete(new CompletionContext( - CompletionContext.Status.DISCARD, lbResponse, lbRequest))); + CompletionContext.Status.DISCARD, lbRequest, lbResponse))); return Mono.just(ClientResponse.create(HttpStatus.SERVICE_UNAVAILABLE) .body(serviceInstanceUnavailableMessage(serviceId)).build()); } @@ -134,10 +134,10 @@ public Mono filter(ClientRequest clientRequest, ExchangeFunction return next.exchange(newRequest) .doOnError(throwable -> supportedLifecycleProcessors.forEach(lifecycle -> lifecycle .onComplete(new CompletionContext( - CompletionContext.Status.FAILED, throwable, lbResponse, lbRequest)))) + CompletionContext.Status.FAILED, throwable, lbRequest, lbResponse)))) .doOnSuccess(clientResponse -> supportedLifecycleProcessors.forEach( lifecycle -> lifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, - lbResponse, new ResponseData(clientResponse, requestData), lbRequest)))) + lbRequest, lbResponse, new ResponseData(clientResponse, requestData))))) .map(clientResponse -> { loadBalancerRetryContext.setClientResponse(clientResponse); if (shouldRetrySameServiceInstance(loadBalancerRetryContext)) { diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java index 7bd033d49..3a8126c9b 100644 --- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java +++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTests.java @@ -514,7 +514,7 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala T response = (T) new MockClientHttpResponse(new byte[] {}, HttpStatus.OK); supportedLoadBalancerProcessors .forEach(lifecycle -> lifecycle.onComplete(new CompletionContext(CompletionContext.Status.SUCCESS, - new DefaultResponse(defaultServiceInstance()), new DefaultRequest<>()))); + new DefaultRequest<>(), new DefaultResponse(defaultServiceInstance())))); return response; } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java index f23564885..2119b4025 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java @@ -38,7 +38,6 @@ import org.springframework.cloud.client.loadbalancer.Request; import org.springframework.cloud.client.loadbalancer.Response; import org.springframework.cloud.client.loadbalancer.ResponseData; -import org.springframework.cloud.client.loadbalancer.TimedRequestContext; import org.springframework.cloud.client.loadbalancer.reactive.ReactiveLoadBalancer; import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; import org.springframework.http.client.ClientHttpResponse; @@ -79,7 +78,7 @@ public T execute(String serviceId, LoadBalancerRequest request) throws IO ServiceInstance serviceInstance = choose(serviceId, lbRequest); if (serviceInstance == null) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.DISCARD, new EmptyResponse(), lbRequest))); + new CompletionContext<>(CompletionContext.Status.DISCARD, lbRequest, new EmptyResponse()))); throw new IllegalStateException("No instances available for " + serviceId); } return execute(serviceId, serviceInstance, lbRequest); @@ -94,9 +93,6 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala loadBalancerClientFactory.getInstances(serviceId, LoadBalancerLifecycle.class), DefaultRequestContext.class, Object.class, ServiceInstance.class); Request lbRequest = request instanceof Request ? (Request) request : new DefaultRequest<>(); - if (lbRequest.getContext() instanceof TimedRequestContext) { - ((TimedRequestContext) lbRequest.getContext()).setRequestStartTime(System.nanoTime()); - } supportedLifecycleProcessors .forEach(lifecycle -> lifecycle.onStartRequest(lbRequest, new DefaultResponse(serviceInstance))); try { @@ -104,17 +100,17 @@ public T execute(String serviceId, ServiceInstance serviceInstance, LoadBala Object clientResponse = getClientResponse(response); supportedLifecycleProcessors .forEach(lifecycle -> lifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, - defaultResponse, clientResponse, lbRequest))); + lbRequest, defaultResponse, clientResponse))); return response; } catch (IOException iOException) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.FAILED, iOException, defaultResponse, lbRequest))); + new CompletionContext<>(CompletionContext.Status.FAILED, iOException, lbRequest, defaultResponse))); throw iOException; } catch (Exception exception) { supportedLifecycleProcessors.forEach(lifecycle -> lifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.FAILED, exception, defaultResponse, lbRequest))); + new CompletionContext<>(CompletionContext.Status.FAILED, exception, lbRequest, defaultResponse))); ReflectionUtils.rethrowRuntimeException(exception); } return null; 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 c2495b6ee..49d8714c7 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 @@ -68,7 +68,7 @@ void shouldRecordSuccessfulTimedRequest() { assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); statsLifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + new CompletionContext<>(CompletionContext.Status.SUCCESS, lbRequest, lbResponse, responseData)); assertThat(meterRegistry.getMeters()).hasSize(2); assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0); @@ -91,7 +91,7 @@ void shouldRecordFailedTimedRequest() { assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); statsLifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.FAILED, new IllegalStateException(), - lbResponse, lbRequest)); + lbRequest, lbResponse)); assertThat(meterRegistry.getMeters()).hasSize(2); assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0); @@ -111,7 +111,7 @@ void shouldNotRecordDiscardedRequest() { Response lbResponse = new EmptyResponse(); statsLifecycle.onStartRequest(lbRequest, lbResponse); - statsLifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbResponse, lbRequest)); + statsLifecycle.onComplete(new CompletionContext<>(CompletionContext.Status.DISCARD, lbRequest, lbResponse)); assertThat(meterRegistry.getMeters()).hasSize(1); assertThat(meterRegistry.get("loadbalancer.requests.discard").counter().count()).isEqualTo(1); } @@ -127,7 +127,7 @@ void shouldNotRecordUnTimedRequest() { assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); statsLifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + new CompletionContext<>(CompletionContext.Status.SUCCESS, lbRequest, lbResponse, responseData)); assertThat(meterRegistry.getMeters()).hasSize(1); assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0); @@ -141,7 +141,7 @@ void shouldNotCreateNullTagsWhenNullDataObjects() { assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); statsLifecycle - .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, null, lbRequest)); + .onComplete(new CompletionContext<>(CompletionContext.Status.SUCCESS, lbRequest, lbResponse, null)); assertThat(meterRegistry.getMeters()).hasSize(2); assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0); @@ -163,7 +163,7 @@ void shouldNotCreateNullTagsWhenEmptyDataObjects() { assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(1); statsLifecycle.onComplete( - new CompletionContext<>(CompletionContext.Status.SUCCESS, lbResponse, responseData, lbRequest)); + new CompletionContext<>(CompletionContext.Status.SUCCESS, lbRequest, lbResponse, responseData)); assertThat(meterRegistry.getMeters()).hasSize(2); assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0);