From 61f1e82cc6b479acf7007782a8410c277a93d932 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Mon, 8 Mar 2021 12:30:24 +0100 Subject: [PATCH] Code review comments --- .../RestTemplateInstrumentationTest.groovy | 48 ++++++++++ .../RestTemplateInterceptorTest.java | 88 ------------------- 2 files changed, 48 insertions(+), 88 deletions(-) create mode 100644 instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy diff --git a/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy b/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy new file mode 100644 index 000000000000..1f585a3ac4d0 --- /dev/null +++ b/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy @@ -0,0 +1,48 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +import io.opentelemetry.instrumentation.spring.httpclients.RestTemplateInterceptor +import io.opentelemetry.instrumentation.test.LibraryTestTrait +import io.opentelemetry.instrumentation.test.base.HttpClientTest +import org.springframework.http.HttpMethod +import org.springframework.web.client.ResourceAccessException +import org.springframework.web.client.RestTemplate +import spock.lang.Shared + +class RestTemplateInstrumentationTest extends HttpClientTest implements LibraryTestTrait { + @Shared + RestTemplate restTemplate + + def setupSpec() { + if (restTemplate == null) { + restTemplate = new RestTemplate() + restTemplate.getInterceptors().add(new RestTemplateInterceptor(getOpenTelemetry())) + } + } + + @Override + int doRequest(String method, URI uri, Map headers, Closure callback) { + try { + return restTemplate.execute(uri, HttpMethod.valueOf(method), { request -> + headers.forEach(request.getHeaders().&add) + }, { response -> + callback?.call() + response.statusCode.value() + }) + } catch (ResourceAccessException exception) { + throw exception.getCause() + } + } + + @Override + boolean testCircularRedirects() { + false + } + + @Override + boolean testRemoteConnection() { + false + } +} diff --git a/instrumentation/spring/spring-web-3.1/library/src/test/java/io/opentelemetry/instrumentation/spring/httpclients/RestTemplateInterceptorTest.java b/instrumentation/spring/spring-web-3.1/library/src/test/java/io/opentelemetry/instrumentation/spring/httpclients/RestTemplateInterceptorTest.java index 06a5e6b9451e..224bdc0c8c3d 100644 --- a/instrumentation/spring/spring-web-3.1/library/src/test/java/io/opentelemetry/instrumentation/spring/httpclients/RestTemplateInterceptorTest.java +++ b/instrumentation/spring/spring-web-3.1/library/src/test/java/io/opentelemetry/instrumentation/spring/httpclients/RestTemplateInterceptorTest.java @@ -6,32 +6,18 @@ package io.opentelemetry.instrumentation.spring.httpclients; import static io.opentelemetry.instrumentation.testing.util.TraceUtils.withClientSpan; -import static io.opentelemetry.instrumentation.testing.util.TraceUtils.withSpan; import static io.opentelemetry.sdk.testing.assertj.TracesAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertSame; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.then; import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.instrumentation.testing.junit.LibraryInstrumentationExtension; -import io.opentelemetry.sdk.trace.data.SpanData; -import io.opentelemetry.sdk.trace.data.StatusData; -import java.io.IOException; -import java.util.List; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.RegisterExtension; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpMethod; import org.springframework.http.HttpRequest; -import org.springframework.http.HttpStatus; import org.springframework.http.client.ClientHttpRequestExecution; -import org.springframework.http.client.ClientHttpResponse; @ExtendWith(MockitoExtension.class) class RestTemplateInterceptorTest { @@ -40,9 +26,7 @@ class RestTemplateInterceptorTest { LibraryInstrumentationExtension.create(); @Mock HttpRequest httpRequestMock; - @Mock HttpHeaders httpHeadersMock; @Mock ClientHttpRequestExecution requestExecutionMock; - @Mock ClientHttpResponse httpResponseMock; byte[] requestBody = new byte[0]; @Test @@ -67,76 +51,4 @@ void shouldSkipWhenContextHasClientSpan() throws Exception { trace.hasSpansSatisfyingExactly( span -> span.hasName("parent").hasKind(SpanKind.CLIENT))); } - - @Test - void shouldStartAndEndSpan() throws Exception { - // given - RestTemplateInterceptor interceptor = - new RestTemplateInterceptor(instrumentation.getOpenTelemetry()); - - given(httpRequestMock.getMethod()).willReturn(HttpMethod.GET); - given(httpRequestMock.getHeaders()).willReturn(httpHeadersMock); - - given(requestExecutionMock.execute(httpRequestMock, requestBody)).willReturn(httpResponseMock); - - given(httpResponseMock.getStatusCode()).willReturn(HttpStatus.OK); - - // when - ClientHttpResponse actual = - withSpan( - "parent", - () -> interceptor.intercept(httpRequestMock, requestBody, requestExecutionMock)); - - // then - assertSame(httpResponseMock, actual); - - then(httpHeadersMock).should().set(eq("traceparent"), anyString()); - - List> traces = instrumentation.waitForTraces(1); - assertThat(traces) - .hasTracesSatisfyingExactly( - trace -> - trace.hasSpansSatisfyingExactly( - parentSpan -> parentSpan.hasName("parent").hasKind(SpanKind.INTERNAL), - span -> span.hasName("HTTP GET").hasKind(SpanKind.CLIENT))); - } - - @Test - void shouldStartAndEndSpanWithException() throws Exception { - // given - RestTemplateInterceptor interceptor = - new RestTemplateInterceptor(instrumentation.getOpenTelemetry()); - - given(httpRequestMock.getMethod()).willReturn(HttpMethod.GET); - given(httpRequestMock.getHeaders()).willReturn(httpHeadersMock); - - Exception thrown = new IOException("boom"); - given(requestExecutionMock.execute(httpRequestMock, requestBody)).willThrow(thrown); - - // when - IOException actual = - assertThrows( - IOException.class, - () -> - withSpan( - "parent", - () -> - interceptor.intercept(httpRequestMock, requestBody, requestExecutionMock))); - - // then - assertSame(thrown, actual); - - then(httpHeadersMock).should().set(eq("traceparent"), anyString()); - - List> traces = instrumentation.waitForTraces(1); - assertThat(traces) - .hasTracesSatisfyingExactly( - trace -> - trace.hasSpansSatisfyingExactly( - parentSpan -> parentSpan.hasName("parent").hasKind(SpanKind.INTERNAL), - span -> - span.hasName("HTTP GET") - .hasKind(SpanKind.CLIENT) - .hasStatus(StatusData.error()))); - } }