RestClient observations are stopped before ResponseSpec calls

[hadjiski]

Debugging a RestTemplate shows that execute() bad responses are properly throwing exceptions, which when caught, are setting the error to the observation:

response = request.execute();
			observationContext.setResponse(response);
			handleResponse(url, method, response);
			return (responseExtractor != null ? responseExtractor.extractData(response) : null);
		}
		catch (IOException ex) {
			ResourceAccessException accessEx = createResourceAccessException(url, method, ex);
			observation.error(accessEx);
			throw accessEx;
		}
		catch (Throwable ex) {
			observation.error(ex);
			throw ex;
		}

Doing the same for the DefaultRestClient retrieve method shows that the clientRequest.execute() call is not throwing any exception, thus no error is recorded to the observation (later during the toEntity call the exception is thrown, but at that time the client observation is already stopped).

clientResponse = clientRequest.execute();
				observationContext.setResponse(clientResponse);
				ConvertibleClientHttpResponse convertibleWrapper = 
new DefaultConvertibleClientHttpResponse(clientResponse);
				return exchangeFunction.exchange(clientRequest, convertibleWrapper);
			}
			catch (IOException ex) {
				ResourceAccessException resourceAccessException = 
createResourceAccessException(uri, this.httpMethod, ex);
				if (observation != null) {
					observation.error(resourceAccessException);
				}
				throw resourceAccessException;
			}
			catch (Throwable error) {
				if (observation != null) {
					observation.error(error);
				}
				throw error;
			}

What is the idea of the new RestClient implementation, how is it supposed to propagate the exception to the observation?

[bclozel]

Can you share a minimal code snippet showing a case (for example against httpbin.org) of an error not being recorded in the observation?

[hadjiski]

Currently on the road, will provide something after some hours, but in general nothing special, just following any of the trivial examples out there

The observation monitoring appears to reside within the retrieve() method, but the http exceptions are thrown in the toEntity() method

[hadjiski]

@bclozel here a running scratch:

import org.springframework.http.ResponseEntity;
import org.springframework.web.client.RestClient;

class Scratch {

    public static void main(String[] args) {
        //curl -X POST "https://httpbin.org/status/400" -H "accept: text/plain"

        ResponseEntity<String> entity = RestClient.create("https://httpbin.org/status/400")
                .post()
                .retrieve()
                .toEntity(String.class);
    }
}

As mentioned above, Inside retrieve() the observation is started and stopped, including exception catching and setting of the error, but the exception is thrown inside the toEntity method

[bclozel]

I think this behavior differs from RestTemplate because we're not using the template pattern here. RestTemplate offers many method variants directly on its type - they all perform the request and all are terminal operations. With RestClient, retrieve() returns a ResponseSpec that you can use to extract information in various fashion and even perform error handling. We instrumented WebClient in the exact same way (in the retrieve() method).

In general, we try to instrument client and servers as close as possible to the transport level:

• for servers, this includes the entire web framework, error handling, right down to the Servlet level
• for clients, this means being as close as possible to the client call; the template-style API of RestTemplate doesn't give much flexibility here

I'm declining this issue as a result.

[hadjiski]

Thanks for the explanation, I understand, but then do you see any condition, under which any of the catch blocks are ever triggered inside the retrieve method?
We switched our code from rest template to the "more modern" RestClient - now this limitation.
Are you actually saying that officially the http.client.requests metric will not get the error/exception tag properly populated when using WebClient or RestClient, but it will work when using the "old" RestTemplate?

[bclozel]

Thanks for the explanation, I understand, but then do you see any condition, under which any of the catch blocks are ever triggered inside the retrieve method?

Yes, those are checked exceptions thrown by the code in the retrieve method. This can happen while serializing the body or during the actual HTTP exchange for any other reason.

We switched our code from rest template to the "more modern" RestClient - now this limitation.

I understand that the behavior is different, but the API is different too. If RestTemplate is working fine for your case you should stick to it, we are not deprecating it.

Are you actually saying that officially the http.client.requests metric will not get the error/exception tag properly populated when using WebClient or RestClient, but it will work when using the "old" RestTemplate?

It depends on the behavior. As you can see in the code, it can be populated.

I think we could extend the error handling to the ResponseSpec here, but other community members could argue that this would make things inconsistent with. WebClient as they would have a different behavior.

[hadjiski]

This would solve it, otherwise all the 4xx/5xx are never recorded right. That appears surprising to me

[bclozel]

@hadjiski this is now available in 6.1.7-SNAPSHOT versions, it would be great if you could test it with your application before we cut the release next month. Thanks!

[hadjiski]

@bclozel just tried it and unfortunately it did not work, it goes out earlier with an exception and it does not reach the statements. I was using the spring-web-6.1.7-20240418.195003-12.jar so your fix should be inside.
The use case I tested was throwing standard 4xx exception, which was coming from here and the exception is not caught to set the error to the observation.
Maybe your fix would work, when the callee side is not well known and the content does not fit any message converter, not sure.

Update:

[bclozel]

@hadjiski Thanks for testing SNAPSHOTs! I think I got it right this time, the tests setup was initially too invasive and hiding this case. This should be available in SNAPSHOTs shortly.

[hadjiski]

yes, I can confirm, this catch block made it, thanks

[shakhar]

@poutsma Since this change I started to get no http.client.requests metrics for my RestClient when I call retrieve().
When I debugged the code, I noticed that on version 6.1.6 the observation stopped in the finally block and now nothing stops it and we don't get any metrics.

[bclozel]

@shakhar have you tried the latest SNAPSHOT version? If so, please create a new issue with a minimal application reproducing the problem. Thanks!

[shakhar]

Yes, tried the latest SNAPSHOT version. I found out that the issue is that I am using retrieve() without toEntity() and just after calling toEntity() it works.
Before I am opening an issue, I just want to ask if it is an expected behavior? Can't I use retrieve() without toEntity() and still get metrics?

[bclozel]

Let's ask this question differently: what are you expecting from a ResponseSpec instance if you're not calling any method on it? What is the goal?

[shakhar]

If I need just to send a warmup request without a need to get the actual response data for example