Spring Improve query params in uri KeyValue with HTTP interface client

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

Comment From: 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.

Comment From: bclozel

Thanks for the report @cj-florian-akos-szabo - I have reproduced this successfully with the HTTP interface client support.

This does not happen when using the RestClient directly, as the 'uri' KeyValue is given directly as the URI template. As long as the RequestBodySpec.uri() method is given the right URI template, this should show correctly in the observation metadata. Thanks for the offer @v-perfilev , but the problem isn't located in DefaultRestClient nor in the ClientRequestObservationConvention.

The HTTP interface client support creates proxies that look at interface method signatures and create the relevant HTTP requests accordingly. In this case, the HttpRequestValues is responsible for extracting values from @HttpExchange methods.

There are several key aspects to consider here: * we must support collection values like "/params?spring=framework&spring=boot" * param names can be provided as method arguments or in Map/MultiValueMap * encoding

I have prepared a commit that will change the generated URI templates to: * "/some/api/with/{variable}/and/params?{foo}={foo[0]}" * "/some/api/with/{variable}/and/params?{foo}={foo[0]}&{bar}={bar[0]}" * "/some/api/with/{variable}/and/params?{foo}={foo[0]}&{foo}={foo[1]}&{bar}={bar[0]}"

This still adds a bit of ceremony but this should work. I have considered changing the implementation to producing foo={foo} when possible, but again, query params can be provided as maps and one could do Map.of("query param", "value") (notice the unencoded space). If we encode it before writing it, developers would lose flexibility for encoding options. If we don't encode it, metrics systems and dashboards might get confused. I think this is a good tradeoff.

Comment From: cj-florian-akos-szabo

Thanks @bclozel for the quick turnaround on this issue, and for reproducing the issue locally. In hindsight, I could have provided more originally to help with that part of the effort.

With this change you implemented, do I get it right that it would also affect and fix this for WebClient, since ReactiveHttpRequestValues extends HttpRequestValues?

Comment From: bclozel

No worries your issue was complete enough. Yes, this should cover WebClient as well.

You can test SNAPSHOT versions if you would like to ensure that this is working for you before the release.