URI_TEMPLATE_ATTRIBUTE used when available with feature flag. Fixes g… #1328
Conversation
|
@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. |
|
@marcingrzejszczak FYI this is a continuation of this: #1327 (comment). |
|
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? |
There was a problem hiding this comment.
@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.
| * @since 3.0.0 | ||
| */ | ||
| final class LoadBalancerTags { | ||
|
|
||
| static final String UNKNOWN = "UNKNOWN"; | ||
|
|
||
| static final String URI_TEMPLATE_ATTRIBUTE = WebClient.class.getName() + ".uriTemplate"; |
There was a problem hiding this comment.
Load-balanced requests can come from WebClient, but also RestClient and RestTemplate, so we need a way to handle all those scenarios.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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}/...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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):
BlockingLoadBalancerClientTestsRetryLoadBalancerInterceptorTestsReactorLoadBalancerExchangeFilterFunctionTestsRetryableLoadBalancerExchangeFilterFunctionIntegrationTests
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.
There was a problem hiding this comment.
Please update the license comment to Copyright 2012-2024 in all the files you've changed.
| * @since 3.0.0 | ||
| */ | ||
| final class LoadBalancerTags { | ||
|
|
||
| static final String UNKNOWN = "UNKNOWN"; | ||
|
|
||
| static final String URI_TEMPLATE_ATTRIBUTE = WebClient.class.getName() + ".uriTemplate"; |
There was a problem hiding this comment.
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?
| * @since 3.0.0 | ||
| */ | ||
| final class LoadBalancerTags { | ||
|
|
||
| static final String UNKNOWN = "UNKNOWN"; | ||
|
|
||
| static final String URI_TEMPLATE_ATTRIBUTE = WebClient.class.getName() + ".uriTemplate"; |
There was a problem hiding this comment.
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):
BlockingLoadBalancerClientTestsRetryLoadBalancerInterceptorTestsReactorLoadBalancerExchangeFilterFunctionTestsRetryableLoadBalancerExchangeFilterFunctionIntegrationTests
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.
|
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 |
|
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 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. |
|
Hi @JaroslawDembek, I guess you do not have time to finish this up. I'd like to get this into our upcoming |
|
I have also discussed using the |
Fixes gh-1302.