WebClient metrics with uri template when using UriBuilder

[jhaeyaert]

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.

[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;
}

[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 ?

[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.

[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 ?

[jhaeyaert]

Hi,

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

[rstoyanchev]

Sure give it a go.

[rstoyanchev]

This is now superseded by the #22705.