Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

URI_TEMPLATE_ATTRIBUTE used when available with feature flag. Fixes g… #1328

Closed
wants to merge 3 commits into from
Closed

URI_TEMPLATE_ATTRIBUTE used when available with feature flag. Fixes g… #1328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3106f0
URI_TEMPLATE_ATTRIBUTE used when available with feature flag. Fixes g…
Jan 26, 2024
bf4bcb9
Revert usage of feature flag.
Jan 31, 2024
3466f66
Merge branch 'spring-cloud:main' into main
JaroslawDembek Feb 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...balancer/src/main/java/org/springframework/cloud/loadbalancer/stats/LoadBalancerTags.java
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update the license comment to Copyright 2012-2024 in all the files you've changed.

Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,21 @@
import org.springframework.cloud.client.loadbalancer.RequestDataContext;
import org.springframework.cloud.client.loadbalancer.ResponseData;
import org.springframework.util.StringUtils;
import org.springframework.web.reactive.function.client.WebClient;

/**
* Utility class for building metrics tags for load-balanced calls.
*
* @author Olga Maciaszek-Sharma
* @author Jaroslaw Dembek
* @since 3.0.0
*/
final class LoadBalancerTags {

static final String UNKNOWN = "UNKNOWN";

static final String URI_TEMPLATE_ATTRIBUTE = WebClient.class.getName() + ".uriTemplate";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Load-balanced requests can come from WebClient, but also RestClient and RestTemplate, so we need a way to handle all those scenarios.

Copy link
Author

Choose a reason for hiding this comment

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

RestClient is new to me and I almost forget about RestTemplate existence.
WebClient has two impls: MVC and WebFlux.
My change is for sure working in SpringBoot 3.1 + WebFlux.
I would start with writing test for all 4 scenarios. Could you advise me on pattern (code example of integration test setup e.g. some old or new PR) I should follow?

Copy link
Author

Choose a reason for hiding this comment

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

spring-projects/spring-framework#31882
There is a change in Spring about value of URI_TEMPLATE_ATTRIBUTE - now it will include baseUrl. Which for load balanced calls is http://{service-name}/...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since for a given client the baseUrl part of the template is going to remain the same anyhow, it should not be an issue when it comes to measuring the requests - the metrics will be effectively differentiated by the path. @marcingrzejszczak wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what you mean by "WebClient has two implementations"? Spring supports blocking or MVC style communication, which is implemented by RestTemplate and RestClient, and reactive communication, implemented by WebClient.

We have integration tests that verify that LoadBalancerLifecycle calls are correctly executed. I'm thinking you could use a similar pattern to work on your integration test. Here are the test classes (they cover blocking load-balancing with and without retries and reactive load-balancing with and without retries):

  • BlockingLoadBalancerClientTests
  • RetryLoadBalancerInterceptorTests
  • ReactorLoadBalancerExchangeFilterFunctionTests
  • RetryableLoadBalancerExchangeFilterFunctionIntegrationTests
    Let me know if you need more help with this. If you do not have time and would like us to take over the PR to work on the non-reactive part, also no worries - let me know.


private LoadBalancerTags() {
throw new UnsupportedOperationException("Cannot instantiate utility class");
}
Expand Down Expand Up @@ -70,6 +74,12 @@ private static int statusValue(ResponseData responseData) {
}

private static String getPath(RequestData requestData) {
if (requestData.getAttributes() != null) {
var uriTemplate = (String) requestData.getAttributes().get(URI_TEMPLATE_ATTRIBUTE);
if (uriTemplate != null) {
return uriTemplate;
}
}
return requestData.getUrl() != null ? requestData.getUrl().getPath() : UNKNOWN;
}

Expand Down
31 changes: 31 additions & 0 deletions ...g/springframework/cloud/loadbalancer/stats/MicrometerStatsLoadBalancerLifecycleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.net.URI;
import java.util.HashMap;
import java.util.Map;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Tag;
Expand All @@ -43,11 +44,13 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.UNKNOWN;
import static org.springframework.cloud.loadbalancer.stats.LoadBalancerTags.URI_TEMPLATE_ATTRIBUTE;

/**
* Tests for {@link MicrometerStatsLoadBalancerLifecycle}.
*
* @author Olga Maciaszek-Sharma
* @author Jaroslaw Dembek
*/
class MicrometerStatsLoadBalancerLifecycleTests {

Expand Down Expand Up @@ -80,6 +83,34 @@ void shouldRecordSuccessfulTimedRequest() {
Tag.of("serviceInstance.port", "8080"), Tag.of("status", "200"), Tag.of("uri", "/test"));
}

@Test
void shouldRecordSuccessfulTimedRequestWithUriTemplate() {
Map<String, Object> attributes = new HashMap<>();
String uriTemplate = "/test/{pathParam}/test";
attributes.put(URI_TEMPLATE_ATTRIBUTE, uriTemplate);
RequestData requestData = new RequestData(HttpMethod.GET, URI.create("http://test.org/test/123/test"),
new HttpHeaders(), new HttpHeaders(), attributes);
Request<Object> lbRequest = new DefaultRequest<>(new RequestDataContext(requestData));
Response<ServiceInstance> 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, lbRequest, lbResponse, responseData));

assertThat(meterRegistry.getMeters()).hasSize(2);
assertThat(meterRegistry.get("loadbalancer.requests.active").gauge().value()).isEqualTo(0);
assertThat(meterRegistry.get("loadbalancer.requests.success").timers()).hasSize(1);
assertThat(meterRegistry.get("loadbalancer.requests.success").timer().count()).isEqualTo(1);
assertThat(meterRegistry.get("loadbalancer.requests.success").timer().getId().getTags()).contains(
Tag.of("method", "GET"), Tag.of("outcome", "SUCCESS"), Tag.of("serviceId", "test"),
Tag.of("serviceInstance.host", "test.org"), Tag.of("serviceInstance.instanceId", "test-1"),
Tag.of("serviceInstance.port", "8080"), Tag.of("status", "200"), Tag.of("uri", uriTemplate));
}

@Test
void shouldRecordFailedTimedRequest() {
RequestData requestData = new RequestData(HttpMethod.GET, URI.create("http://test.org/test"), new HttpHeaders(),
Expand Down
Loading