Skip to content
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

Fix RestTemplateInterceptor so that it calls endExceptionally() on exception #2516

Merged

Conversation

mateuszrzeszutek
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek commented Mar 5, 2021

Resolves #2479

I've added a few testing changes in this PR because I wouldn't be able to unit-test my change otherwise:

  • implemented a new TraceUtils class in testing-common; contrary to the old one it uses BaseTracer to create spans, so we can now test span suppression;
  • added mockito to dependencies.gradle

@laurit
Copy link
Contributor

laurit commented Mar 5, 2021

Maybe we could also test it the same way as we do for other http clients.

import io.opentelemetry.api.GlobalOpenTelemetry
import io.opentelemetry.instrumentation.spring.httpclients.RestTemplateInterceptor
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.base.HttpClientTest
import org.springframework.http.HttpEntity
import org.springframework.http.HttpHeaders
import org.springframework.http.HttpMethod
import org.springframework.http.client.SimpleClientHttpRequestFactory
import org.springframework.web.client.RestTemplate

class RestTemplateTest extends HttpClientTest implements AgentTestTrait {

  @Override
  int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {
    SimpleClientHttpRequestFactory clientHttpRequestFactory = new SimpleClientHttpRequestFactory()
    clientHttpRequestFactory.setConnectTimeout(CONNECT_TIMEOUT_MS)
    RestTemplate restTemplate = new RestTemplate(clientHttpRequestFactory)
    RestTemplateInterceptor restTemplateInterceptor = new RestTemplateInterceptor(GlobalOpenTelemetry.get())
    restTemplate.getInterceptors().add(restTemplateInterceptor)

    HttpHeaders httpHeaders = new HttpHeaders()
    headers.each {
      httpHeaders.add(it.key, it.value)
    }
    HttpEntity<?> requestEntity = new HttpEntity<>("body", httpHeaders)
    def status = restTemplate.exchange(uri, HttpMethod.resolve(method), requestEntity, String).getStatusCodeValue()
    callback?.call()
    return status
  }
}

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could also test it the same way as we do for other http clients.

👍

@mateuszrzeszutek
Copy link
Member Author

Maybe we could also test it the same way as we do for other http clients.

Yeah, definitely 🤦‍♂️ I don't know how I did not think of this earlier 😅

@mateuszrzeszutek
Copy link
Member Author

Done!
I've left only one unit test with mocks: the one that verifies that we do not create a seconds CLIENT span. This test should also be added to the HttpClientTest, but let's do it in another PR.

@mateuszrzeszutek mateuszrzeszutek force-pushed the fix-RestTemplateInterceptor branch from 61f1e82 to ff0645d Compare March 8, 2021 12:14
@trask trask merged commit 3dff448 into open-telemetry:main Mar 8, 2021
@mateuszrzeszutek mateuszrzeszutek deleted the fix-RestTemplateInterceptor branch November 18, 2022 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RestTemplateInterceptor does not call endExceptionally() when throwable is thrown
4 participants