URI_TEMPLATE_ATTRIBUTE used when available with feature flag. Fixes g…

[JaroslawDembek]

Fixes gh-1302.

[JaroslawDembek]

@OlgaMaciaszek
I've added a feature flag.
TBH Now it doesn't look very neat now. getPath is very low as private method in utility class.
I do not think that somebody really needs old behaviour. Metrics are aggregated usually on path and IMO this is a memory leak rather than a feature.

[OlgaMaciaszek]

Thanks @JaroslawDembek, the idea would be to remove the old behaviour completely from the new release. I agree this is bug rather than a feature, and normally we would just change the behaviour, but I think when it comes to metric tags, it might be better to be very conservative with any changes, but I'm going to request @marcingrzejszczak to weigh in on this.

[OlgaMaciaszek]

@marcingrzejszczak FYI this is a continuation of this: #1327 (comment).

[OlgaMaciaszek]

Hi @JaroslawDembek, after some team chat, we think we can go forward with this fix without the flag after all, since there will bo no changes in the actual tag names; sorry for the confusion. Will you update the PR?

[OlgaMaciaszek]

@JaroslawDembek Thanks for updating the PR. I have noticed that the solution only seems to handle WebClient requests. Have added a comment, and another cosmetic one. Please take a look.

[OlgaMaciaszek]

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

[JaroslawDembek]

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?

[JaroslawDembek]

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}/...

[OlgaMaciaszek]

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?

[OlgaMaciaszek]

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.

[OlgaMaciaszek]

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

[OlgaMaciaszek]

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?

[OlgaMaciaszek]

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.

[OlgaMaciaszek]

Hello @JaroslawDembek - have you seen my last comment? Are those tests what you were looking for? Do you need more help or just need some more time?

[JaroslawDembek]

Hello @JaroslawDembek - have you seen my last comment? Are those tests what you were looking for? Do you need more help or just need some more time?

Hints were ok. It was a good starting point. I was able to work it out for reactive scenario, but unfortunatelly I hit a wall with RestClient + BlockingLoadbalancingClient - no easy access to uriTemplate field.
TBH I believe the old way should be abandoned and the whole thing should be moved to Observation API - low/high cardinality aspect is perfect for this.
Wdyt?
I've started this (looking as great learning oportunity), but definetely I need more time.

[OlgaMaciaszek]

Hi @JaroslawDembek - thanks a lot. Yes, it definitely makes sense to switch to Observation API - if you'd like to work on it, that sounds great. Also looking at the blocking implementation. In fact, since we're operating at the level of interceptor, we only get to work with org.springframework.http.HttpRequest, and specifically org.springframework.http.client.InterceptingClientHttpRequest, and we do not have access from the uriTemplate from there, as it doesn't pass it on.

Given that the issue that you've indicated may, in fact, cause considerable problems, I think, we should at least allow the users of the blocking implementation to avoid facing it by allowing a possibility to avoid tagging for path altogether. That, again, could be achieved by setting a flag. While, I agree it's not the most elegant solution, it may be better than not having a way to avoid the possible memory leak.

[OlgaMaciaszek]

Hi @JaroslawDembek, I guess you do not have time to finish this up. I'd like to get this into our upcoming RC-1, so I'm going to cherry-pick your commits and add the remaining changes.

[OlgaMaciaszek]

I have also discussed using the Observation with @marcingrzejszczak and it seems that unless we want to create spans, it's better to stick with Meter and using URI templates, as a high cardinality tag will not really give us any additional insight.