Spring WebClient metrics with uri template when using UriBuilder

Hi,

Here are the versions this discussion is related to :

Spring 5.1.4.RELEASE
SpringBoot 2.1.2.RELEASE
Micrometer 1.1.2

Using reactive WebClient with micrometer give the ability to expose a lot of interesting metrics (such as number of requests, duration, ...). As describe in the official documentation, metrics generated by an instrumented client are tagged with the request’s URI template prior to variable substitution, if possible (for example, /api/person/{id}).

Indeed, it appears that the URI template cannot be used when specifying uri using UriBuilder. So doing this :

webclient.get().uri("/api/person/{id}", "1234")...

will generate a metric with the expected uri template (eg : /api/person/{id}), but using uri builder like this :

webclient.get().uri(uriBuilder -> uriBuilder.path("/api/person/{id}").build("1234"))

will generate metrics using the substituted uri (eg : /api/person/1234).

This is annoying cause we can have lot of possible uris and we loose precious information (and I think the uri builder is the smartest way to forge uris).

Looking closer, it appears that there is no URI_TEMPLATE_ATTRIBUTE attribute set when using UriBuilder with WebClient.

Digging deeper, I discovered that the current implementation does not take care about baseUri set on the WebClient. So doing this :

webClientBuilder().baseUri("https://foo.com/api").build().get().uri("/person/{id}", "1234")...

Will generate a metric using /person/{id} instead of /api/person/{id}.

I suggest to make few changes on UriBuilder to be able to get access to the original uri template (add a getUriTemplate method). This way we could adapt the DefaultWebClient like this :

    @Override
    public RequestBodySpec uri(String uriTemplate, Object... uriVariables) {
        UriBuilder uriBuilder = uriBuilderFactory.uriString(uriTemplate);
        attribute(URI_TEMPLATE_ATTRIBUTE, uriBuilder.getUriTemplate());
        return uri(uriBuilder.build(uriVariables));
    }

    @Override
    public RequestBodySpec uri(String uriTemplate, Map<String, ?> uriVariables) {
        UriBuilder uriBuilder = uriBuilderFactory.uriString(uriTemplate);
        attribute(URI_TEMPLATE_ATTRIBUTE, uriBuilder.getUriTemplate());
        return uri(uriBuilder.build(uriVariables));
    }

    @Override
    public RequestBodySpec uri(Function<UriBuilder, URI> uriFunction) {
        UriBuilder uriBuilder = uriBuilderFactory.builder();
        RequestBodySpec uri = uri(uriFunction.apply(uriBuilder));
        attribute(URI_TEMPLATE_ATTRIBUTE, uriBuilder.getUriTemplate());
        return uri;
    }

This solution seems very simple but I would like to know if you think this is a good approach and if it make sense for you ?

If you want I can make a pull request (around 10 lines of code).

Keep me in touch.

Comment From: rstoyanchev

Digging deeper, I discovered that the current implementation does not take care about baseUri... webClientBuilder().baseUri("https://foo.com/api").build().get().uri("/person/{id}", "1234")

The code snippet works for me. Provide a sample that demonstrates the issue.

I would like to know if you think this is a good approach and if it make sense for you ?

Thanks for asking but the approach is not clear. This does not compile:

@Override
public RequestBodySpec uri(Function<UriBuilder, URI> uriFunction) {
    UriBuilder uriBuilder = uriBuilderFactory.builder();
    RequestBodySpec uri = uri(uriFunction.apply(uriBuilder));
    attribute(URI_TEMPLATE_ATTRIBUTE, uriBuilder.getUriTemplate());  // Cannot resolve `getUriTemplate'
    return uri;
}

Comment From: jhaeyaert

Hi,

I don't know what I can provide to demonstrate that the current implementation does not take care about baseUri. If you take a look at the current implementation of the uri(String uriTemplate, Object... uriVariables) method on DefaultWebClient

    @Override
    public RequestBodySpec uri(String uriTemplate, Object... uriVariables) {
        attribute(URI_TEMPLATE_ATTRIBUTE, uriTemplate);
        return uri(uriBuilderFactory.expand(uriTemplate, uriVariables));
    }

You can see, the that attribute(URI_TEMPLATE_ATTRIBUTE, uriTemplate); only take the uriTemplate value. This basically demonstrate that if you do this webClientBuilder().baseUri("https://foo.com/api").build().get().uri("/person/{id}", "1234") you will have URI_TEMPLATE_ATTRIBUTE set to /person/{id} instead of /api/person/{id} right ?

About the suggested implementation you noticed :

    @Override
    public RequestBodySpec uri(String uriTemplate, Object... uriVariables) {
        UriBuilder uriBuilder = uriBuilderFactory.uriString(uriTemplate);
        attribute(URI_TEMPLATE_ATTRIBUTE, uriBuilder.getUriTemplate());
        return uri(uriBuilder.build(uriVariables));
    }

It is normal this code does not compile cause it was just an example of what the code could be if the UriBuilder interface were modified with a new getUriTemplate method to give access to the not expanded uri.

May be a complete pull request could be more clear ?

Comment From: rstoyanchev

Okay, I took "does not take care about baseUri set on the WebClient" to mean the baseUri doesn't work in general. I understand now you meant it for the URI_TEMPLATE_ATTRIBUTE only.

I'm not keen on automatically creating as many objects on every request for an attribute that may or may not be needed. It would also require an accessor method on UriBuilder which is an interface for building. And we'd have to somehow implement it on UriComponentsBuilder which may have been created from other inputs besides a URI template (e.g. URI, ServerHttpRequest).

The URI template vs the builder options are somewhat mutually exclusive I would say. Can you comment on your use of the builder with a URI template? The given example could also be done as a URI template or with builder methods, e.g. path("/api/person").pathSegment("1234"). One reason to ask is we could consider adding uri(String uriTemplate, Function<UriBuilder, URI>) where you provide the URI template and then finish up with the builder but it'd be useful to have something concrete in mind for that.

As for the baseUrl issue can be addressed independently in any case by extracting the path from the baseUrl up front and then prepending it later to the uriTemplate value.

Comment From: jhaeyaert

Hi,

I used to use the UriBuilder cause I think this is the most elegant and safe solution to prepand an uri with all dynamic path segments and query params (whatever the type of the parameters).

For example, using pathSegment with a value other than String is not possible. So doing this is not possible path("/api/person").pathSegment(1234)

Also, using UriBuilder is always better if I want to automatically manage query parameters of different types (String, Int, List, Array, null values, ...) instead of doing it by myself.

That's why I always prefer construct the uri this way (even for simple cases) :

Integer anInteger = 12345;
double anDouble = 0.2;
List<String> aListOfValues = ...;

webclient.get().uri(uriBuilder -> uriBuilder.path("/api/person/{personId}")
    .queryParam("param1", aDouble)
    .queryParam("param2", "A string value with spaces")
    .queryParam("param3", aListOfValues)
    .queryParam("param4", null)
    .build(anInteger))

As explain on my first post, doing like this will not add the URI_TEMPLATE_ATTRIBUTE (cause the uriTemplate is inaccessible) and will unfortunately generate metrics with /api/person/12345 instead of /api/person/{personId}. When you have a lot of possibilities such as a person identifier, this could lead to generate a lot of useless metrics.

I can only agree that modifying the UriBuilder interface is not a good idea. I think your suggestion about adding uri(String uriTemplate, Function<UriBuilder, URI>) is a very interesting track to explore and could be effectively a simple solution to add the URI_TEMPLATE_ATTRIBUTE attribute.

A possible implementation could be :

    @Override
    public RequestBodySpec uri(String uriTemplate, Function<UriBuilder, URI>) {
        attribute(URI_TEMPLATE_ATTRIBUTE, uriTemplate);
        return uri(uriFunction.apply(uriBuilderFactory.uriString(uriTemplate)));
    }

Do you think it could be interesting to go further and try to provide a working solution based on uri(String uriTemplate, Function<UriBuilder, URI>) suggestion ?

Comment From: jhaeyaert

Hi,

I make a little up if you want me to provide a working solution.

Comment From: rstoyanchev

Sure give it a go.

Comment From: rstoyanchev

This is now superseded by the #22705.

Comment From: nhmarujo

@rstoyanchev can this be reopened? I see you superseded this ticket with itself (maybe some mistake?). I also still observe the issues described on this ticket. Thanks

Comment From: bclozel

@nhmarujo it's been superseded by the PR, which has been merged in 5.2. See #22705 If you're still experiencing the problem, please create a new issue with a sample project reproducing the problem.

Comment From: nhmarujo

Thanks @bclozel. I opened https://github.com/spring-projects/spring-boot/issues/22832 and referenced this one.