Align WebClient uri metric tag with RestTemplate's

[nhmarujo]

I think that http.client.requests metric has a wrong behaviour with query parameters. The query parameters should not be considered as part of the uri tag.

If you have 2 calls to http://test123.io/api and http://test123.io/api?q=1 I wouldn't expect them to be counted as different uris. Not only I think it is conceptually wrong, but also there is no way to group them later.

On top of that I can observe some inconsistencies on the result you get by using different approaches with either RestTemplate and WebClient.

Some examples:

1. Will produce the tag uri="/api/{pathParam}?queryParam={queryParam}"

webClient.get()
    .uri("http://test123.io/api/{pathParam}?queryParam={queryParam}", "foobar", "xpto")
    .retrieve()
    .toEntity(Void.class)
    .block();

2. Will produce the tag uri="/test123.io/api/foobar"

webClient.get()
    .uri(uriBuilder -> uriBuilder.path("http://test123.io/api/{pathParam}")
        .queryParam("queryParam", "xpto")
        .build("foobar"))
    .retrieve()
    .toEntity(Void.class)
    .block();

3. Will produce the tag uri="/api/foobar"

webClient.get()
    .uri(uriBuilder -> uriBuilder.host("http://test123.io")
        .path("/api/{pathParam}")
        .queryParam("queryParam", "xpto")
        .build("foobar"))
    .retrieve()
    .toEntity(Void.class)
    .block();

4. Will produce the tag uri="/api/{pathParam}?queryParam={queryParam}"

restTemplate.getForObject("http://test123.io/api/{pathParam}?queryParam={queryParam}", Void.class, "foobar", "xpto");

5. Will produce the tag uri="/api/foobar?queryParam=xpto"

URI uri=UriComponentsBuilder.fromHttpUrl("http://test123.io/api/{pathParam}")
    .queryParam("queryParam", "xpto")
    .build("foobar");

restTemplate.getForObject(uri, Void.class);

In any of these cases I believe the correct value, for the tag, would be uri="/api/{pathParam}"

This was all tested using SB2.3.2 using the spring-boot-starter-webflux and spring-boot-starter-web.

I believe this is related to what is reported on this issue spring-projects/spring-framework#22371 , which was closed and marked as superseded, but references itself.

[wilkinsona]

5 is working as expected. From RestTemplate's perspective, there's no variable substitution being performed as you've passed in a URI where the substitution has already taken place. 2 and 3 are quite similar, although I wonder if we may be able to do something there as there isn't quite the same degree of separation between the client and the URI creation.

For 2 and 3, the URI template can be captured and the URIBuilder still used if you call uri(String, Function<UriBuilder, URI>) rather than the uri(Function<UriBuilder, URI>) which you're currently calling.

[nhmarujo]

Well I see your point regarding 5.. I was not sure about how URI handles the substitution and thought that those could be extracted from it separately. That said, I still would expect query parameters to be trimmed off.

[wilkinsona]

I still would expect query parameters to be trimmed off

You said above that "it is conceptually wrong", but did not explain why. Could you please do so? Query parameters are part of the URI so it feels correct to me that they should be included in a URI tag when available.

[nhmarujo]

We are just talking about metrics here and in that scope I think it makes no sense. I did said "there is no way to group them later", but let me explain better 😄

Imagine you are monitoring an app and want to know, for instance, which endpoints have most calls. Now imagine you have an endpoint that can take like 5 optional query parameters. This means you will have 1 different uri value for each possible combination of those optional query parameters (C_(5,1) + C_(5,2) + C_(5,3) + C_(5,4) + C_(5,5) = 31 combinations). You will not be able to sum (or any other type of aggregation) those metrics because the uri tag is different, but in fact is a call to the same endpoint.

[nhmarujo]

You may say that you may want to know about the query parameters for the metrics in some cases. And though I will agree with you, I think you could put the query parameters part in another tag and that would cover those cases too.

[wilkinsona]

That doesn't sound conceptually wrong to me, rather it sounds like a situation where it isn't possible to provide default behaviour that meets everyone's needs. Some will want the query parameters in the URI tag, others like you won't. If the default tagging behaviour doesn't meet your needs, you can provide a WebClientExchangeTagsProvider or a RestTemplateExchangeTagsProvider bean to take control.

[nhmarujo]

Well, if it is provided all in one tag, it will serve one group of people. But if it is provided separately, it will serve both groups :)

So I think the default behaviour should be the one that satisfies the majority (and in this case, the second approach actually serves the purpose of the first too, so there is no downside to it).

I still think it is conceptually wrong in the scope of metrics tagging.

[bclozel]

Let's summarize the behavior we're seeing here.

Whenever a String template is given to the client, the *ExchangeTags classes will get everything after the protocol+host part and use it for the uri tag. We do not only consider the path, but also all query parameters present in the string template. This is by design; we're using the template provided by the developer.

Imagine you are monitoring an app and want to know, for instance, which endpoints have most calls. Now imagine you have an endpoint that can take like 5 optional query parameters. This means you will have 1 different uri value for each possible combination of those optional query parameters (C(5,1) + C(5,2) + C(5,3) + C(5,4) + C(5,5) = 31 combinations). You will not be able to sum (or any other type of aggregation) those metrics because the uri tag is different, but in fact is a call to the same endpoint.

Regarding the statement above, I'm not sure I understand this point: no matter what combination of query parameter values are provided to the request, the template will remain the same and will be reused for all metrics. I guess your statement was referring to expanded URI instances, not String templates. In this case, note that you don't need query parameters to run into cardinality explosion problems, a simple path variable is enough!

For the samples where the URI instance is built and then given to the client (2,3 and 5), we cannot get the original template string since the information is gone at the URI stage. This part of the behavior is expected.

But we do see an inconsistency here: when the expanded URI is given to the client, WebClient is using the URL path only to create the tag, whereas RestTemplate is using the full URI minus the protocol+host. I believe this is a mistake done in the original implementation of the WebClient tags.

The team has discussed this issue and we came to the following conclusions:

1. we need to fix this inconsistency and align WebClient uri tag with the RestTemplate uri tag behavior, when the client is given an expanded URI
2. while some might consider that query parameters should not be part of the tag and can lead to cardinality explision, the core problem in this case is that an URI template is not used in the first place. This is a well-known problem and can lead to cardinality explosion with simple path variables in routes.
3. we could consider removing the query parameters in the cases we're given the expanded URIs, but this has been the behavior for RestTemplate for a very long time and WebClient is inconsistent in this case.

So this issue is now about aligning the WebClient metrics behavior with what we have already with RestTemplate.