Skip to content

RestClient exchange allow the extension of ClientRequestObservationContext #32153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
davebarda opened this issue Jan 29, 2024 · 3 comments
Closed
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply theme: observability An issue related to observability and tracing

Comments

@davebarda
Copy link

davebarda commented Jan 29, 2024

Hi guys, I'm trying to use RestClient together with Micrometer's Observation API,
what i'm trying to do is to supply some additional context, the "senderServiceName", that i'd like to exist as a tag in my metrics.
Today, the internalExchange looks the following way:

...
ClientRequestObservationContext observationContext = new ClientRequestObservationContext(clientRequest);
observationContext.setUriTemplate(this.uriTemplate);
observation = ClientHttpObservationDocumentation.HTTP_CLIENT_EXCHANGES.observation(observationConvention,
		DEFAULT_OBSERVATION_CONVENTION, () -> observationContext, observationRegistry).start();
...

As you can see, the observationContext is being composed from the request carrier, and directly sent to the observation.
As the logic of the request creation has its complexity, I've thought of just passing an UnaryOperator which will allow me the take the initialized context, modify it (maybe to an extension of ClientRequestObservationContext) and return the modified version.

Something like:

private <T> T exchangeInternal(ExchangeFunction<T> exchangeFunction, ExchangeOptions exchangeOptions) {
	Assert.notNull(exchangeFunction, "ExchangeFunction must not be null");
	Assert.notNull(exchangeOptions, "ExchangeOptions must not be null");

	ClientHttpResponse clientResponse = null;
	Observation observation = null;
	URI uri = null;
	try {
		if (DefaultRestClient.this.defaultRequest != null) {
			DefaultRestClient.this.defaultRequest.accept(this);
		}
		uri = initUri();
		HttpHeaders headers = initHeaders();
		ClientHttpRequest clientRequest = createRequest(uri);
		clientRequest.getHeaders().addAll(headers);
		ClientRequestObservationContext observationContext = exchangeOptions.observationContextOperator()
				.apply(new ClientRequestObservationContext(clientRequest));
		observationContext.setUriTemplate(this.uriTemplate);
		observation = ClientHttpObservationDocumentation.HTTP_CLIENT_EXCHANGES.observation(observationConvention,
				DEFAULT_OBSERVATION_CONVENTION, () -> observationContext, observationRegistry).start();
...
}

Should I issue a PR which allows it?

Thanks.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 29, 2024
@bclozel
Copy link
Member

bclozel commented Jan 29, 2024

I don't think this would make sense from an API perspective. You can already configure a custom convention and this should have everything it needs from the request. I would suggest looking into #32027 as a possible way out for this would be to use a request attribute or a request context holder. This senderServiceName information could be then extracted by your custom convention without changing this internal contract.

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Jan 29, 2024
@bclozel bclozel added in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply theme: observability An issue related to observability and tracing and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 29, 2024
@davebarda
Copy link
Author

davebarda commented Jan 29, 2024

I don't think this would make sense from an API perspective. You can already configure a custom convention and this should have everything it needs from the request. I would suggest looking into #32027 as a possible way out for this would be to use a request attribute or a request context holder. This senderServiceName information could be then extracted by your custom convention without changing this internal contract.

In my context, which isn't an Http request, I don't find the usage of random globals a safer nor cleaner way of setting context for the observability, afterall, it is what it is, an observation context...

@bclozel
Copy link
Member

bclozel commented Jan 29, 2024

@davebarda this could be an argument for a request attribute feature, then. Can you make this case in the linked issue and explain how you're solving this problem with RestTemplate please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply theme: observability An issue related to observability and tracing
Projects
None yet
Development

No branches or pull requests

3 participants