Unclear documentation for the uri observability metric in HTTP clients

[Mattias-Sehlstedt]

Description

The current documentation for the uri metric for HTTP clients is:
URI template used for HTTP request, or "none" if none was provided. Only the path part of the URI is considered.
(found here: https://docs.spring.io/spring-framework/reference/integration/observability.html#observability.http-client)

It was last updated in relation to #29885, where the second sentence was added as a description for removing the protocol, host, and port part of the template.

I find "path" to be a semi-incorrect description, since the current behavior is that it retains query values (which are separate from path https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax).

Example

TL;DR:

When executing:
RestTemplate.exchange(https://example.org/resource/{id}?queryKey={queryValue}, GET, null, Response.class, "1", "query")
the metric that is register has uri as /resource/{id}?queryKey={queryValue}, which contains more than just the path of the uri.

Longer version

When calling RestTemplate execute with one of the signatures that support uriTemplate (e.g., https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/web/client/RestTemplate.java#L797), then I would expect that only the path part of the uriTemplate would be registered to the uri metric.

So if my template is https://example.org/resource/{id}?queryKey0={queryValue0}&queryKey1={queryValue1} then I would expect the registered value to be /resource/{id}. But the actual registered value is /resource/{id}?queryKey0={queryValue0}&queryKey1={queryValue1}.

This is since the default behavior of the observation convention is to only removes the protocol, host and port (it is done here: https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientRequestObservationConvention.java#L109).

The issue

The low-cardinality metric might get a quite high cardinality if the client endpoints have a large variety of query parameters, which is not apparent in the documentation since it states that only the path is considered.

Suggested solution

I would find it beneficial if the description was more specific and explicitly stated what the uri metric will contain by default.

Preferably test(s) could be introduced as well to illustrate that it is intended that the query parameters remain as part of the default metric.

Sidenote

Is it intentional that the low cardinality metrics should include the templated query parameters? I would have expected the base behavior to strip the uri of everything except the path, and that the user then would have to extend the templating if they were to be interested in specific queries and to what degree they were present? Especially since the metric also comes with an complete path metric too with its high cardinality value (http.url).

[bclozel]

I'm not sure I understand. Can you share a code snippet showing how the client is used and what metric/trace you are getting?

[Mattias-Sehlstedt]

I have updated the original comment with a better description.

Please specify if actual semi-runnable code is preferred (I currently do not have a project that uses this, but could create one if it would better illustrate the case).

[bclozel]

Thanks for the added details. I think the current behavior is expected, but we should improve the documentation.
The goal here is to capture the template given by the developer, but to leave out the scheme+authority parts because those are captured in other key value and they can vary.

I'll close this one in favour of your PR in #34116 which addresses the problem.

[bclozel]

One additional bit.

The low-cardinality metric might get a quite high cardinality if the client endpoints have a large variety of query parameters, which is not apparent in the documentation since it states that only the path is considered.

I think we assume that the number of RestTemplate.exchange itself in an application will be bounded and not too high. If an application needs more variations of URIs, I suspect they would use vairants like RestClient.uri(String uri, Function<UriBuilder, URI> uriFunction) which won't be a problem.