RestClient observation uri tag handling of query parameters

[cj-florian-akos-szabo]

I have a project where I noticed a behavior in the http.client.requests metric coming from RestClient that's defined via such interface:

public interface ApiServiceClient {

    @GetExchange("/some/api/with/{variable}/and/params")
    ApiResultPojo makeApiRequest(
            @PathVariable String variable,
            @RequestParam(required = false, name=foo) String foo,
            @RequestParam(required = false, name=bar) String bar
    );
}

resulting in http.client.requests metric with such uri tags:

{
  "tag": "uri",
  "values": [
    "/some/api/with/{variable}/and/params",
    "/some/api/with/{variable}/and/params?{queryParam0}={queryParam0[0]}",
    "/some/api/with/{variable}/and/params?{queryParam0}={queryParam0[0]}&{queryParam1}={queryParam1[0]}"
  ]
},

depending on how many of the optional query params are non-null.

My issue is mainly with the cryptic names of the query params: queryParam0 & queryParam1. Could it be somehow possible to default these to use the name or value passed in the RequestParam annotation, and only use these basic placeholder names if custom name/value is not available? So in this case I'd expect the uri tag to include such items:

{
  "tag": "uri",
  "values": [
    "/some/api/with/{variable}/and/params",
    "/some/api/with/{variable}/and/params?foo={foo}",
    "/some/api/with/{variable}/and/params?foo={foo}&bar={bar}"
  ]
},

And currently, to work around this issue I am considering adding such a custom ClientRequestObservationConvention when I build the RestClient:

public class ExtendedClientRequestObservationConvention extends DefaultClientRequestObservationConvention {

    @Override
    public KeyValues getLowCardinalityKeyValues(ClientRequestObservationContext context) {
        return super.getLowCardinalityKeyValues(context).and(additionalTags(context));
    }

    protected KeyValues additionalTags(ClientRequestObservationContext context) {
        String fullUri = Objects.requireNonNull(context.getCarrier()).getURI().toString();
        String paramKeys = UriComponentsBuilder
                .fromUriString(fullUri).build()
                .getQueryParams().keySet()
                .stream()
                .sorted()
                .map(key -> key + "={" + key + "}")
                .collect(Collectors.joining("&"));
        String uriTemplate = Objects.requireNonNull(context.getUriTemplate()).split("\\?")[0];
        String uri = paramKeys.isEmpty() ? uriTemplate : uriTemplate + "?" + paramKeys;

        return KeyValues.of(KeyValue.of(URI, uri));
    }
}

[v-perfilev]

Hi Florian,
nice workaround, it should work! I just believe that the right long-term solution would involve addressing this directly in the uri methods of DefaultRestClient:

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

This is where the URI_TEMPLATE_ATTRIBUTE is set, and the only place it's used is for metrics in observatonContext.
I'd suggest introducing new interfaces: TemplateBuilder and TemplateBuilderFactory (in the org.springframework.web.util package), along with their default implementations. These could then be used in DefaultRestClient like:

@Override
public RequestBodySpec uri(String uriTemplate, Map<String, ?> uriVariables) {
    TemplateBuilder templateBuilder = templateBuilderFactory.uriTemplateWithVars(uriTemplate, uriVariables); // new template builder instead of uriBuilder
    attribute(URI_TEMPLATE_ATTRIBUTE, templateBuilder.toUriString());
    return uri(DefaultRestClient.this.uriBuilderFactory.expand(uriTemplate, uriVariables));
}

With this approach the URI_TEMPLATE_ATTRIBUTE would look like /api/response/{id}?bar={bar} instead of /api/response/{id}?{queryParam0}={queryParam0[0]}.

@bclozel Do you think this feature is necessary? If so, I'd be happy to contribute and implement it.