Align WebFluxTags#uri() with WebMvcTags#uri()

[dreis2211]

Hi,

this PR is an attempt to fix #15608 by aligning WebFluxTags#uri() with WebMvcTags#uri().

Let me know what you think.
Cheers,
Christoph

[youzipi]

So you map all the other service's url to root and unknown.
Is it possible to collect other service's mapping in gateway?
for monitoring requests in one place, as there may be many services and each service may have several instances.

[spencergibb]

This doesn't know about the gateway

[dreis2211]

@youzipi This should get rid of the tag explosion you see, at least. And it makes sense to align it with the variant of WebMvcTags, if you ask me. Having that said, I understand your conceirns that more URLs will be potentially unknown, if the best matching pattern is not available. But then again, WebMvcTags had the same issue and it was addressed similarly.

@bclozel What do you think? (Given you self-assigned this)

Cheers

[bclozel]

Thanks a bunch @dreis2211 !

@youzipi Spencer and Christoph are right, there's no way to have both the full uri information without being aware of the pattern and avoid the tag explosion at the same time. I guess a gateway should be more concerned about producing local metrics, and each application should publish its own to a shared repository.

[kishoreprakash7]

Where is this code available? Or which version of springBoot is it available?

[wilkinsona]

As shown by the milestone to which this issue is assigned, the changes we made in Spring Boot 2.0.8. You can also see in the commit that closed this PR all of the tags that include the change.

[favetelinguis]

I just posted this https://stackoverflow.com/questions/58724623/webfluxtagsuri-returning-wrong-tags before finding this. Maybe you can give some clarification there what exchange.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); does and what one has to do for the request to always get a match here since now lots of my request get null back here even when they have a path.

[veerumallavarapu]

I am using Spring boot actuator 2.3.0 but still seeing the same issue. Please fix it asap as it is blocking my metrics in grafana dashboard.

http_server_requests_seconds_bucket{exception="None",method="GET",outcome="CLIENT_ERROR",status="401",uri="UNKNOWN",le="22.906492245",} 2.0

or else atleast give me some work around to fix this.

@ALL - Did anyone overwritten this jar to get the correct URL instead of Unknown ???

[bclozel]

@veerumallavarapu Your question is very similar to #16208 - without this change, you might get a case of tag cardinality explosion: this would make things even worse in your grafana dashboard (or worse, OOM errors). To work around this problem, please ensure that the handler is mapped by a pattern.

For more guidance about your particular case, please create a question on StackOverflow with more information, such as the HTTP request and the code handling the request.

[veerumallavarapu]

I did not get "To work around this problem, please ensure that the handler is mapped by a pattern." where should I do this ?

[bclozel]

@veerumallavarapu the raw metric data doesn't tell much about this. There might be no way to work around this. It really depends on what's handling the request. A question on StackOverflow with the requested details would be more productive.

[veerumallavarapu]

Is this not possible to set the Pattern again ?

[PyvesB]

Could someone please clarify why this change was needed? Several people claim that it gets rid of tag explosion, but didn't #14173 take care of that already? In particular the following bean should ensure that a configurable maximum number of different uri values are emitted:

spring-boot/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfiguration.java

Line 80 in 9460d74

public MeterFilter metricsHttpServerUriTagFilter() {

Is this a case of users having not upgraded to the Spring release containing #14173 when they noticed this issue, leading to two different fixes for the same problem only a few months apart?

Having layered this on top of the existing implementation has several disadvantages and leads to various problems:

• if a server router does not use a pattern, but still has reasonable cardinality as defined by getMaxUriTags, all uri metrics are lost due to this change.
• if the input request path has infinite cardinality, you cannot extract a specific path sub-element with finite cardinality using Micrometer's MeterFilter mapping and replacing functions. Indeed, the metrics are lost at the very beginning instead of being filtered downstream by WebFluxMetricsAutoConfiguration. For example for client metrics, one can do things such as MetricFilter.replaceTagValues​("uri", ...);.
• following from the previous point, there is now an inconsistency in behaviour between WebFlux client and server metrics for the same uri tag.
• emitting UNKNOWN is confusing (@veerumallavarapu comments above highlight this, my engineering team struggled with this a well), as the URI of the server request is known, the implementation from this PR just decides to label it as unknown if there is no pattern in the router function.
• this behaviour is obscure and not configurable. The WebFluxMetricsAutoConfiguration approach on the other hand is configurable and logs a useful message when filtering kicks in.
• this is more code to maintain and execute for each server request. Arguably the overhead is small, but it feels wrong to try to limit URI tag explosion twice in different places of the code base.

If other people agree this should be reverted, I'm happy to submit a pull request. I'm also happy to be enlightened if #14173 and this PR happen to be somehow mutually exclusive. :)

[wilkinsona]

Thanks for the feedback, @PyvesB. The change was made to align the behaviour of WebFlux's metrics with those of MVC. The meter filter and emitting UNKNOWN aren't really equivalent with the former stopping request metrics being recorded entirely when the limit is reached.

Emitting UNKNOWN is a default. It can be changed by providing a WebFluxTagsContributor @Bean that overwrites the default URI tag. Alternatively, you can provide you own WebFluxTagsProvider @Bean and take complete control over the tags.

[Illutax]

We wanted to add the URI to all requests, so we added this Bean:

/**
 * The URI {@link io.micrometer.core.instrument.Tag} is occasionally set as <b>UNKNOWN</b> when the {@link HandlerMapping#BEST_MATCHING_PATTERN_ATTRIBUTE}
 * is not set in the exchange attributes. To counter that we added this custom WebFluxTagsContributor.
 */
@Bean
WebFluxTagsContributor uriTagContributorForConnectionProblems()
{
	return (exchange, ex) -> willExchangeUriBeingParsed(exchange)
	                         ? Tags.empty()
	                         : Tags.of("uri", exchange.getRequest().getPath().value());
}

private static boolean willExchangeUriBeingParsed(final ServerWebExchange exchange)
{
	return exchange.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE) != null;
}

[sw-fox]

Thank you @Illutax this works, but WebFluxTagsContributor is deprecated since 3.0.0
Is there another future proof solution?

[wilkinsona]

@sw-fox As mentioned in the javadoc the replacement is ServerRequestObservationConvention. You can learn more about how to use this interface in the documentation.

[massahud]

I don't know if I was having the same problem, but none of my Path predicates were being matched as uri in http_server_requests_*, so I created a ServerRequestObservationConvention bean that matches all Path predicates in the configuration, maybe this can help someone with this problem as a workaround.

/**
 * This class is a workaround because spring cloud gateway is not setting the pathPattern to the context, which makes
 * metrics of known patterns have UNKNOWN uri.
 * <p>
 * This implementation uses the Path pattern predicates defined in {@link GatewayProperties}, so it probably won't work for
 * programmatically defined path predicates.
 */
@NonNullApi
@Component
public class PathMatchingObservationConvention extends DefaultServerRequestObservationConvention {
    private static final Logger LOGGER = LoggerFactory.getLogger(
            com.tripadvisor.dapr.gateway.PathMatchingObservationConvention.class);
    private final List<Tuple2<String, PathPattern>> pathPatterns;

    @Autowired
    public PathMatchingObservationConvention(final GatewayProperties gatewayProperties) {
        pathPatterns = new ArrayList<>();
        gatewayProperties.getRoutes().forEach(routeDefinition -> {
            pathPatterns.addAll(routeDefinition.getPredicates().stream().filter(
                    predicateDefinition -> "Path".equals(predicateDefinition.getName())).flatMap(
                    predicateDefinition -> predicateDefinition.getArgs().values().stream()).map(pattern -> {
                PathPatternParser parser = PathPatternParser.defaultInstance;
                pattern = parser.initFullPathPattern(pattern);
                return Tuple.of(pattern, parser.parse(pattern));
            }).toList());
        });
    }

    /**
     * Method overriden to set the pathPattern to the context based on the defined Path predicates in the configuration.
     * This makes 'uri' return matches patterns when getting the low cardinality tags.
     *
     * @param context the context
     * @return the uri
     * @see <a href="https://github.com/spring-projects/spring-boot/pull/15609#issuecomment-709987823">github issue</a>
     */
    @Override
    public KeyValues getLowCardinalityKeyValues(final ServerRequestObservationContext context) {
        if (context.getPathPattern() == null) {
            pathPatterns.stream().filter(
                    tuple -> tuple.map2(pattern -> pattern.matches(context.getCarrier().getPath()))._2()).map(
                    Tuple2::_1).findFirst().ifPresent(context::setPathPattern);
        }
        return super.getLowCardinalityKeyValues(context);
    }
}

[esteban-coder]

@massahud Thanks it works, however it only shows the predicate but not the whole uri. For instance if gateway enroutes to a microservice that exposes 5 endpoints, and gateway lets access to that microservice through its predicate, the metrics only shows for that predicate not distinguishing the calls for which of the 5 endpoints.

[massahud]

@massahud Thanks it works, however it only shows the predicate but not the whole uri. For instance if gateway enroutes to a microservice that exposes 5 endpoints, and gateway lets access to that microservice through its predicate, the metrics only shows for that predicate not distinguishing the calls for which of the 5 endpoints.

@esteban-coder maybe you are looking at the server metrics (http_server_requests_*) , upstream endpoints usually appear in the http_client_requests_* metrics.

[danparisi]

Issue is still there in Spring Boot 3.3.0. Here is the solution I implemented:

@Configuration
public class MicrometerConfiguration {
    private static final String KEY_URI = "uri";
    private static final String UNKNOWN = "UNKNOWN";

    @Bean
    public ServerRequestObservationConvention uriTagContributorForObservationApi() {

        /**
         * Fixes the URI {@link org.springframework.http.server.reactive.observation.ServerHttpObservationDocumentation} value when set as <b>UNKNOWN</b> in /metrics API
         * by Spring framework issue. See https://github.com/spring-cloud/spring-cloud-gateway/issues/891
         */
        return new DefaultServerRequestObservationConvention() {
            @Override
            public KeyValues getLowCardinalityKeyValues(ServerRequestObservationContext context) {
                KeyValues lowCardinalityKeyValues = super.getLowCardinalityKeyValues(context);

                if (isUriTagNullOrUnknown(context, lowCardinalityKeyValues)) {
                    return lowCardinalityKeyValues
                            .and(KeyValue.of(KEY_URI, context.getCarrier().getPath().value()));
                }

                return lowCardinalityKeyValues;
            }

            private static boolean isUriTagNullOrUnknown(ServerRequestObservationContext context, KeyValues lowCardinalityKeyValues) {
                Optional<KeyValue> uriKeyValue = lowCardinalityKeyValues.stream()
                        .filter(keyValue -> URI.name()
                                .equals(keyValue.getKey())).findFirst();

                return (uriKeyValue.isEmpty() || UNKNOWN.equals(uriKeyValue.get().getValue()));
            }
        };

[bclozel]

@danparisi I believe this is a Spring Cloud Gateway issue, probably a different one than spring-cloud/spring-cloud-gateway/issues#891 though. I believe that this might be done on purpose, if the gateway doesn't know of specific uri patterns. Please create a new Spring Cloud Gateway issue to discuss this.

Note that your workaround is vulnerable to denial of service attacks on your app because it creates cardinality explosion for metrics.

[srinivasboini]

@danparisi , Thanks for the solution. This is exactly what I have been looking for..

[wilkinsona]

@srinivasboini Please ensure you're aware of Brian's concern about denial of service:

Note that your workaround is vulnerable to denial of service attacks on your app because it creates cardinality explosion for metrics.

[srinivasboini]

Hi @wilkinsona ,
Yes. I'm aware of Brian's concern, as its logging the exact uri information.

My requirement is bit different. I need to pass the gateway route path predicate in the uri attribute. I don't need to pass the exact url that is being called.

gateway route

-id: test 
uri: lb://test-service
predicate:
 - Path=/test/app/{id}

if I'm calling with /test/app/100 i need to get the uri as /test/app/{id} and not the exact high cardinality value.

I have changed the solution a bit according to my need.

 return new DefaultServerRequestObservationConvention() {
            @Override
            public KeyValues getLowCardinalityKeyValues(ServerRequestObservationContext context) {
                KeyValues lowCardinalityKeyValues = super.getLowCardinalityKeyValues(context);

                if (isUriTagNullOrUnknown(context, lowCardinalityKeyValues)) {
     
                String uriPath = (String) context.getAttributes.getOrDefault("gatewayPredicateRouteAttr", "UNKNOWN");

                    return lowCardinalityKeyValues
                            .and(KeyValue.of(KEY_URI, uriPath));
                }


                return lowCardinalityKeyValues;
            }

            private static boolean isUriTagNullOrUnknown(ServerRequestObservationContext context, KeyValues lowCardinalityKeyValues) {
                Optional<KeyValue> uriKeyValue = lowCardinalityKeyValues.stream()
                        .filter(keyValue -> URI.name()
                                .equals(keyValue.getKey())).findFirst();

                return (uriKeyValue.isEmpty() || UNKNOWN.equals(uriKeyValue.get().getValue()));
            }

@bclozel, could you have a look at this please ? and thanks @wilkinsona for highlighting this.

[bclozel]

@srinivasboini if you can get a route pattern as an attribute and use this to avoid cardinality explosion it's fine.

[Priyamomer]

Above solution was showing problem in the context for the reactive springboot gateway this I have personally used to fix it

Code:

@Configuration
@NonNullApi
public class MicrometerConfiguration {
   private static final String KEY_URI = "uri";
   private static final String UNKNOWN = "UNKNOWN";
   
   @Bean
   public DefaultServerRequestObservationConvention uriTagContributorForObservationApi() {
       return new DefaultServerRequestObservationConvention() {
           @Override
           public KeyValues getLowCardinalityKeyValues(ServerRequestObservationContext context) {
               KeyValues lowCardinalityKeyValues = super.getLowCardinalityKeyValues(context);
               ServerHttpRequest request = context.getCarrier();

               if (isUriTagNullOrUnknown(context, lowCardinalityKeyValues)) {
                   return lowCardinalityKeyValues
                           .and(KeyValue.of(KEY_URI, request.getPath().value()));
               }
               return lowCardinalityKeyValues;
           }
           
           private static boolean isUriTagNullOrUnknown(ServerRequestObservationContext context, KeyValues lowCardinalityKeyValues) {
               Optional<KeyValue> uriKeyValue = lowCardinalityKeyValues.stream()
                       .filter(keyValue -> ServerHttpObservationDocumentation.LowCardinalityKeyNames.URI.name()
                               .equals(keyValue.getKey()))
                       .findFirst();
               return (uriKeyValue.isEmpty() || UNKNOWN.equals(uriKeyValue.get().getValue()));
           }
       };
   }
}

IMPORTS

import io.micrometer.common.KeyValue;
import io.micrometer.common.KeyValues;
import io.micrometer.common.lang.NonNullApi;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.server.reactive.observation.DefaultServerRequestObservationConvention;
import org.springframework.http.server.reactive.observation.ServerRequestObservationContext;
import org.springframework.http.server.reactive.observation.ServerHttpObservationDocumentation;
import org.springframework.http.server.reactive.ServerHttpRequest;
import java.util.Optional;

[massahud]

                   return lowCardinalityKeyValues
                           .and(KeyValue.of(KEY_URI, request.getPath().value()));

@Priyamomer I don't recommend blindly adding the request path, it can generate too many metrics if you have parameterized path patterns, also an attack with random paths will affect your metrics collector.
Try to match only the existing path patterns that you have and leave unknown for the unmatched ones.

This example shows how to use PathPatternMatcher to create a PathPattern and match with the request path.

[Priyamomer]

Yes that we should add you are right