spring-cloud
URI_TEMPLATE_ATTRIBUTE used when available with feature flag. Fixes g…
@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.
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: https://github.com/spring-cloud/spring-cloud-commons/pull/1327#issuecomment-1911913665.
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?
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?
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.
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.
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.
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.
Closing in favour of https://github.com/spring-cloud/spring-cloud-commons/pull/1422.