spring-cloud / spring-cloud-commons

Common classes used in different Spring Cloud implementations
Apache License 2.0
704 stars 700 forks source link

WebClient's URI_TEMPLATE_ATTRIBUTE ignored by MicrometerStatsLoadBalancerLifecycle #1302

Open JaroslawDembek opened 9 months ago

JaroslawDembek commented 9 months ago

Version: 4.0.4

Problem

When spring.cloud.loadbalancer.stats.micrometer.enabled: true using WebClient with loadbalancing can lead to generation of huge number of metrics when path contains params, e.g. /test/{somePerCallId}/... what eventually with high workload can cause even OOM.

Common approach

Spring metrics collectors for WebClient uses URI_TEMPLATE_ATTRIBUTE (which contains placeholder instead of actual param value) to overcome problem mention above.

Solution:

Make MicrometerStatsLoadBalancerLifecycle URI_TEMPLATE_ATTRIBUTE aware. Now this logic is hidden in LoadBalancerTags utility class.

private static String getPath(RequestData requestData) {
     return requestData.getUrl() != null ? requestData.getUrl().getPath() : UNKNOWN;
}

Possibly MicrometerStatsLoadBalancerLifecycle could be aware of client type.

Alternative:

Set spring.cloud.loadbalancer.stats.micrometer.enabled: false and deliver you own metrics collector as bean implementing LoadBalancerLifecycle to override LoadBalancerTags.getPath with e.g.

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;
}
OlgaMaciaszek commented 7 months ago

Hi @JaroslawDembek, thanks for raising the issue. Would you like to submit a PR with the changes? If yes, please keep in mind that since this introduces a possibly breaking behaviour change, then method would need to use a flag to go the old path or the new one, with the flag already being deprecated since we'd transition to the new behaviour as the only one with the next major release. If you don't want to work on the PR, no worries - we'll take care of it.

JaroslawDembek commented 7 months ago

I am already using overrided version of metric collector with SpringBoot3 so it shouldn't be a big task. I will jump on it next week.

JaroslawDembek commented 7 months ago

@OlgaMaciaszek I've created a PR to 4.0.x. I need some advice how to add a feature flag. It looks like a new @ConfigurationProperties is needed.