Skip to content

Commit

Permalink
Fix RestTemplateInterceptor so that it calls endExceptionally() on ex…
Browse files Browse the repository at this point in the history
…ception (#2516)
  • Loading branch information
Mateusz Rzeszutek authored Mar 8, 2021
1 parent fe4d95a commit 3dff448
Show file tree
Hide file tree
Showing 22 changed files with 290 additions and 53 deletions.
7 changes: 6 additions & 1 deletion gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ ext {
prometheus : "0.9.0",
assertj : '3.19.0',
awaitility : '4.0.3',
mockito : '3.6.0',
// Caffeine 2.x to support Java 8+. 3.x is 11+.
caffeine : '2.9.0',
testcontainers : '1.15.2'
Expand Down Expand Up @@ -101,6 +102,10 @@ ext {
coroutines : dependencies.create(group: 'org.jetbrains.kotlinx', name: 'kotlinx-coroutines-core', version: "${versions.coroutines}"),
junitApi : "org.junit.jupiter:junit-jupiter-api:${versions.junit5}",
assertj : "org.assertj:assertj-core:${versions.assertj}",
awaitility : "org.awaitility:awaitility:${versions.awaitility}"
awaitility : "org.awaitility:awaitility:${versions.awaitility}",
mockito : [
"org.mockito:mockito-core:${versions.mockito}",
"org.mockito:mockito-junit-jupiter:${versions.mockito}"
]
]
}
2 changes: 1 addition & 1 deletion instrumentation-api/instrumentation-api.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ dependencies {
annotationProcessor deps.autoValue

testImplementation project(':testing-common')
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito
testImplementation deps.assertj
testImplementation deps.awaitility
}
2 changes: 1 addition & 1 deletion instrumentation-core/servlet-2.2/servlet-2.2.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ dependencies {
compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.2'

testImplementation group: 'javax.servlet', name: 'servlet-api', version: '2.2'
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito
testImplementation deps.assertj
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,6 @@ dependencies {
testImplementation deps.guava

testImplementation project(':instrumentation:aws-lambda-1.0:testing')
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito
testImplementation deps.assertj
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ dependencies {
testImplementation project(':instrumentation:aws-sdk:aws-sdk-2.2:testing')

testImplementation deps.assertj
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito
}

test {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ dependencies {
testLibrary group: 'org.springframework.kafka', name: 'spring-kafka-test', version: '1.3.3.RELEASE'
testImplementation group: 'javax.xml.bind', name: 'jaxb-api', version: '2.2.3'
testLibrary group: 'org.assertj', name: 'assertj-core', version: '2.9.+'
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito

// Include latest version of kafka itself along with latest version of client libs.
// This seems to help with jar compatibility hell.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ dependencies {
testLibrary group: 'org.springframework.kafka', name: 'spring-kafka-test', version: '1.3.3.RELEASE'
testImplementation group: 'javax.xml.bind', name: 'jaxb-api', version: '2.2.3'
testLibrary group: 'org.assertj', name: 'assertj-core', version: '2.9.+'
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito


// Include latest version of kafka itself along with latest version of client libs.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ dependencies {

testImplementation deps.opentelemetrySdkMetrics
testImplementation project(':testing-common')
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import static io.opentelemetry.api.trace.SpanKind.CLIENT;
import static io.opentelemetry.api.trace.SpanKind.INTERNAL;
import static io.opentelemetry.api.trace.SpanKind.SERVER;
import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace;
import static io.opentelemetry.instrumentation.testing.util.TraceUtils.withClientSpan;
import static io.opentelemetry.instrumentation.testing.util.TraceUtils.withServerSpan;
import static io.opentelemetry.instrumentation.testing.util.TraceUtils.withSpan;
import static io.opentelemetry.sdk.testing.assertj.TracesAssert.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

Expand Down Expand Up @@ -49,18 +51,12 @@ public String testWithSpanWithException() throws Exception {
}

@WithSpan(kind = CLIENT)
public String testWithClientSpan(Runnable nestedCall) {
if (nestedCall != null) {
nestedCall.run();
}
public String testWithClientSpan() {
return "Span with name testWithClientSpan and SpanKind.CLIENT was created";
}

@WithSpan(kind = SpanKind.SERVER)
public String testWithServerSpan(Runnable nestedCall) {
if (nestedCall != null) {
nestedCall.run();
}
public String testWithServerSpan() {
return "Span with name testWithServerSpan and SpanKind.SERVER was created";
}
}
Expand All @@ -83,14 +79,9 @@ static void tearDown() {

@Test
@DisplayName("when method is annotated with @WithSpan should wrap method execution in a Span")
void withSpan() throws Throwable {
void withSpanWithDefaults() throws Throwable {
// when
runUnderTrace(
"parent",
() -> {
withSpanTester.testWithSpan();
return null;
});
withSpan("parent", () -> withSpanTester.testWithSpan());

// then
List<List<SpanData>> traces = instrumentation.waitForTraces(1);
Expand All @@ -112,12 +103,7 @@ void withSpan() throws Throwable {
"when @WithSpan value is set should wrap method execution in a Span with custom name")
void withSpanName() throws Throwable {
// when
runUnderTrace(
"parent",
() -> {
withSpanTester.testWithSpanWithValue();
return null;
});
withSpan("parent", () -> withSpanTester.testWithSpanWithValue());

// then
List<List<SpanData>> traces = instrumentation.waitForTraces(1);
Expand Down Expand Up @@ -155,12 +141,7 @@ void withSpanError() throws Throwable {
"when method is annotated with @WithSpan(kind=CLIENT) should build span with the declared SpanKind")
void withSpanKind() throws Throwable {
// when
runUnderTrace(
"parent",
() -> {
withSpanTester.testWithClientSpan(null);
return null;
});
withSpan("parent", () -> withSpanTester.testWithClientSpan());

// then
List<List<SpanData>> traces = instrumentation.waitForTraces(1);
Expand All @@ -180,32 +161,30 @@ void withSpanKind() throws Throwable {
"when method is annotated with @WithSpan(kind=CLIENT) and context already contains a CLIENT span should suppress span")
void suppressClientSpan() throws Throwable {
// when
withSpanTester.testWithClientSpan(() -> withSpanTester.testWithClientSpan(null));
withClientSpan("parent", () -> withSpanTester.testWithClientSpan());

// then
List<List<SpanData>> traces = instrumentation.waitForTraces(1);
assertThat(traces)
.hasTracesSatisfyingExactly(
trace ->
trace.hasSpansSatisfyingExactly(
parentSpan ->
parentSpan.hasName("WithSpanTester.testWithClientSpan").hasKind(CLIENT)));
parentSpan -> parentSpan.hasName("parent").hasKind(CLIENT)));
}

@Test
@DisplayName(
"when method is annotated with @WithSpan(kind=SERVER) and context already contains a SERVER span should suppress span")
void suppressServerSpan() throws Throwable {
// when
withSpanTester.testWithServerSpan(() -> withSpanTester.testWithServerSpan(null));
withServerSpan("parent", () -> withSpanTester.testWithServerSpan());

// then
List<List<SpanData>> traces = instrumentation.waitForTraces(1);
assertThat(traces)
.hasTracesSatisfyingExactly(
trace ->
trace.hasSpansSatisfyingExactly(
parentSpan ->
parentSpan.hasName("WithSpanTester.testWithServerSpan").hasKind(SERVER)));
parentSpan -> parentSpan.hasName("parent").hasKind(SERVER)));
}
}
10 changes: 5 additions & 5 deletions instrumentation/spring/spring-web-3.1/library/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Replace `SPRING_VERSION` with the version of spring you're using.
`Minimum version: 3.1`

Replace `OPENTELEMETRY_VERSION` with the latest stable [release](https://mvnrepository.com/artifact/io.opentelemetry).
`Minimum version: 0.8.0`
`Minimum version: 0.17.0`

For Maven add to your `pom.xml`:
```xml
Expand Down Expand Up @@ -60,8 +60,8 @@ interface. An example is shown below:

```java

import io.opentelemetry.instrumentation.spring.httpclients.RestTemplateInterceptor
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.instrumentation.spring.httpclients.RestTemplateInterceptor;
import io.opentelemetry.api.OpenTelemetry;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
Expand All @@ -73,10 +73,10 @@ import org.springframework.web.client.RestTemplate;
public class RestTemplateConfig {

@Bean
public RestTemplate restTemplate(Tracer tracer) {
public RestTemplate restTemplate(OpenTelemetry openTelemetry) {

RestTemplate restTemplate = new RestTemplate();
RestTemplateInterceptor restTemplateInterceptor = new RestTemplateInterceptor(tracer);
RestTemplateInterceptor restTemplateInterceptor = new RestTemplateInterceptor(openTelemetry);
restTemplate.getInterceptors().add(restTemplateInterceptor);

return restTemplate;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,11 @@ apply from: "$rootDir/gradle/instrumentation-library.gradle"

dependencies {
compileOnly "org.springframework:spring-web:3.1.0.RELEASE"

testImplementation "org.springframework:spring-web:3.1.0.RELEASE"

testImplementation project(':testing-common')
testImplementation deps.assertj
testImplementation deps.mockito
testImplementation deps.opentelemetrySdkTesting
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ public ClientHttpResponse intercept(
ClientHttpResponse response = execution.execute(request, body);
tracer.end(context, response);
return response;
} catch (Throwable t) {
tracer.endExceptionally(context, t);
throw t;
}
// TODO: endExceptionally?
}
}
Original file line number Diff line number Diff line change
@@ -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<String, String> 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
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.spring.httpclients;

import static io.opentelemetry.instrumentation.testing.util.TraceUtils.withClientSpan;
import static io.opentelemetry.sdk.testing.assertj.TracesAssert.assertThat;
import static org.mockito.BDDMockito.then;

import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.instrumentation.testing.junit.LibraryInstrumentationExtension;
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.HttpRequest;
import org.springframework.http.client.ClientHttpRequestExecution;

@ExtendWith(MockitoExtension.class)
class RestTemplateInterceptorTest {
@RegisterExtension
static final LibraryInstrumentationExtension instrumentation =
LibraryInstrumentationExtension.create();

@Mock HttpRequest httpRequestMock;
@Mock ClientHttpRequestExecution requestExecutionMock;
byte[] requestBody = new byte[0];

@Test
void shouldSkipWhenContextHasClientSpan() throws Exception {
// given
RestTemplateInterceptor interceptor =
new RestTemplateInterceptor(instrumentation.getOpenTelemetry());

// when
withClientSpan(
"parent",
() -> {
interceptor.intercept(httpRequestMock, requestBody, requestExecutionMock);
});

// then
then(requestExecutionMock).should().execute(httpRequestMock, requestBody);

assertThat(instrumentation.waitForTraces(1))
.hasTracesSatisfyingExactly(
trace ->
trace.hasSpansSatisfyingExactly(
span -> span.hasName("parent").hasKind(SpanKind.CLIENT)));
}
}
2 changes: 1 addition & 1 deletion javaagent-api/javaagent-api.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ dependencies {
implementation project(':instrumentation-api')

testImplementation project(':testing-common')
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito
testImplementation deps.assertj
}
2 changes: 1 addition & 1 deletion javaagent-bootstrap/javaagent-bootstrap.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,6 @@ dependencies {
implementation project(':instrumentation-api')

testImplementation project(':testing-common')
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito
testImplementation deps.assertj
}
2 changes: 1 addition & 1 deletion javaagent-tooling/javaagent-tooling.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ dependencies {

testImplementation project(':testing-common')
testImplementation deps.assertj
testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
testImplementation deps.mockito

instrumentationMuzzle sourceSets.main.output
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ import io.opentelemetry.instrumentation.test.asserts.AttributesAssert
import io.opentelemetry.instrumentation.test.asserts.TraceAssert
import io.opentelemetry.instrumentation.test.server.ServerTraceUtils
import io.opentelemetry.sdk.trace.data.SpanData

import java.util.concurrent.Callable
import java.util.concurrent.ExecutionException

// TODO: convert all usages of this class to the Java TraceUtils one
class TraceUtils {

private static final Tracer tracer = GlobalOpenTelemetry.getTracer("test")
Expand Down
Loading

0 comments on commit 3dff448

Please sign in to comment.