search

spring-cloud / spring-cloud-commons

Common classes used in different Spring Cloud implementations
Apache License 2.0
701 stars 697 forks source link

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

Open JaroslawDembek opened 6 months ago

JaroslawDembek commented 6 months ago

Fixes gh-1302.

JaroslawDembek commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@marcingrzejszczak FYI this is a continuation of this: https://github.com/spring-cloud/spring-cloud-commons/pull/1327#issuecomment-1911913665.

OlgaMaciaszek commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.