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.

Sign up for GitHub

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

Extract library instrumentation for AWS SDK v1 #2525

Merged
merged 6 commits into from
Mar 10, 2021
Merged

Extract library instrumentation for AWS SDK v1 #2525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1a645e0
Extract library instrumentation for AWS SDK v1
Mar 8, 2021
f586134
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 9, 2021
f76ce68
Drift
Mar 9, 2021
b4c0141
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
anuraaga Mar 9, 2021
9b1b1a4
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
Mar 10, 2021
fe97a1f
Merge branch 'aws-sdk-v1-library' of github.com:anuraaga/opentelemetr…
Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions instrumentation/aws-sdk/aws-sdk-1.11/javaagent/aws-sdk-1.11-javaagent.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ testSets {
}

// We test SQS separately since we have special logic for it and want to make sure the presence of
// SQS on the classpath doesn't conflict with tests for usage of the core SDK.
// SQS on the classpath doesn't conflict with tests for usage of the core SDK. This only affects
// the agent.
testSqs
}

Expand All @@ -52,6 +53,8 @@ configurations {
dependencies {
compileOnly deps.opentelemetryExtAws

implementation project(':instrumentation:aws-sdk:aws-sdk-1.11:library')

library group: 'com.amazonaws', name: 'aws-java-sdk-core', version: '1.11.0'

testLibrary group: 'com.amazonaws', name: 'aws-java-sdk-s3', version: '1.11.106'
Expand All @@ -61,9 +64,9 @@ dependencies {
testLibrary group: 'com.amazonaws', name: 'aws-java-sdk-dynamodb', version: '1.11.106'
testLibrary group: 'com.amazonaws', name: 'aws-java-sdk-sns', version: '1.11.106'

testImplementation project(':instrumentation:aws-sdk:aws-sdk-1.11:testing')

testSqsImplementation group: 'com.amazonaws', name: 'aws-java-sdk-sqs', version: '1.11.106'
// needed for SQS - using emq directly as localstack references emq v0.15.7 ie WITHOUT AWS trace header propagation
testSqsImplementation group: 'org.elasticmq', name: 'elasticmq-rest-sqs_2.12', version: '1.0.0'

// Include httpclient instrumentation for testing because it is a dependency for aws-sdk.
testInstrumentation project(':instrumentation:apache-httpclient:apache-httpclient-4.0:javaagent')
Expand Down
28 changes: 17 additions & 11 deletions ...io/opentelemetry/javaagent/instrumentation/awssdk/v1_11/AwsHttpClientInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,19 @@

package io.opentelemetry.javaagent.instrumentation.awssdk.v1_11;

import static io.opentelemetry.javaagent.instrumentation.awssdk.v1_11.AwsSdkClientTracer.CONTEXT_SCOPE_PAIR_CONTEXT_KEY;
import static io.opentelemetry.javaagent.instrumentation.awssdk.v1_11.AwsSdkClientTracer.tracer;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.not;
import static net.bytebuddy.matcher.ElementMatchers.returns;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;

import com.amazonaws.AmazonClientException;
import com.amazonaws.Request;
import com.amazonaws.Response;
import com.amazonaws.handlers.RequestHandler2;
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
import java.util.Map;
import net.bytebuddy.asm.Advice;
Expand All @@ -38,22 +40,26 @@ public ElementMatcher<TypeDescription> typeMatcher() {
@Override
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
return singletonMap(
isMethod().and(not(isAbstract())).and(named("doExecute")),
isMethod()
.and(not(isAbstract()))
.and(named("doExecute"))
.and(takesArgument(0, named("com.amazonaws.Request")))
.and(returns(named("com.amazonaws.Response"))),
AwsHttpClientInstrumentation.class.getName() + "$HttpClientAdvice");
}

public static class HttpClientAdvice {
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void methodExit(
@Advice.Argument(value = 0, optional = true) Request<?> request,
@Advice.Argument(value = 0) Request<?> request,
@Advice.Return Response<?> response,
@Advice.Thrown Throwable throwable) {
if (throwable != null) {
ContextScopePair scope = request.getHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY);
if (scope != null) {
request.addHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY, null);
tracer().endExceptionally(scope.getContext(), throwable);
scope.closeScope();
}
if (throwable instanceof Exception) {
Copy link
Contributor Author

@anuraaga anuraaga Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There is a small regression in my desire to delegate the actual tracing to the library instrumentation - Error will result in unclosed traces (but not uncleaned threadlocal). I feel this should be rare enough to live with until we get a report of problems.

TracingRequestHandler.tracingHandler.afterError(request, response, (Exception) throwable);
}
Scope scope = request.getHandlerContext(TracingRequestHandler.SCOPE);
if (scope != null) {
scope.close();
}
}
}
Expand Down
27 changes: 0 additions & 27 deletions ...c/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v1_11/ContextScopePair.java
View file
Open in desktop

This file was deleted.

27 changes: 16 additions & 11 deletions .../opentelemetry/javaagent/instrumentation/awssdk/v1_11/RequestExecutorInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@

package io.opentelemetry.javaagent.instrumentation.awssdk.v1_11;

import static io.opentelemetry.javaagent.instrumentation.awssdk.v1_11.AwsSdkClientTracer.CONTEXT_SCOPE_PAIR_CONTEXT_KEY;
import static io.opentelemetry.javaagent.instrumentation.awssdk.v1_11.AwsSdkClientTracer.tracer;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.not;
import static net.bytebuddy.matcher.ElementMatchers.returns;

import com.amazonaws.Request;
import com.amazonaws.Response;
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
import java.util.Map;
import net.bytebuddy.asm.Advice;
Expand All @@ -35,21 +36,25 @@ public ElementMatcher<TypeDescription> typeMatcher() {
@Override
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
return singletonMap(
isMethod().and(not(isAbstract())).and(named("doExecute")),
isMethod()
.and(not(isAbstract()))
.and(named("doExecute"))
.and(returns(named("com.amazonaws.Response"))),
RequestExecutorInstrumentation.class.getName() + "$RequestExecutorAdvice");
}

public static class RequestExecutorAdvice {
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void methodExit(
@Advice.FieldValue("request") Request<?> request, @Advice.Thrown Throwable throwable) {
if (throwable != null) {
ContextScopePair scope = request.getHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY);
if (scope != null) {
request.addHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY, null);
tracer().endExceptionally(scope.getContext(), throwable);
scope.closeScope();
}
@Advice.FieldValue("request") Request<?> request,
@Advice.Return Response<?> response,
@Advice.Thrown Throwable throwable) {
if (throwable instanceof Exception) {
TracingRequestHandler.tracingHandler.afterError(request, response, (Exception) throwable);
}
Scope scope = request.getHandlerContext(TracingRequestHandler.SCOPE);
if (scope != null) {
scope.close();
}
}
}
Expand Down
102 changes: 40 additions & 62 deletions ...n/java/io/opentelemetry/javaagent/instrumentation/awssdk/v1_11/TracingRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,94 +5,72 @@

package io.opentelemetry.javaagent.instrumentation.awssdk.v1_11;

import static io.opentelemetry.javaagent.instrumentation.awssdk.v1_11.AwsSdkClientTracer.CONTEXT_SCOPE_PAIR_CONTEXT_KEY;
import static io.opentelemetry.javaagent.instrumentation.awssdk.v1_11.AwsSdkClientTracer.tracer;

import com.amazonaws.AmazonWebServiceRequest;
import com.amazonaws.Request;
import com.amazonaws.Response;
import com.amazonaws.handlers.HandlerContextKey;
import com.amazonaws.handlers.RequestHandler2;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import java.util.List;
import io.opentelemetry.instrumentation.api.config.Config;
import io.opentelemetry.instrumentation.awssdk.v1_11.AwsSdkTracing;

/** Tracing Request Handler. */
/**
* A {@link RequestHandler2} for use in the agent. Unlike library instrumentation, the agent will
* also instrument the underlying HTTP client, and we must set the context as current to be able to
* suppress it. Also unlike library instrumentation, we are able to instrument the SDK's internal
* classes to handle buggy behavior related to exceptions that can cause scopes to never be closed
* otherwise which would be disastrous. We hope there won't be anymore significant changes to this
* legacy SDK that would cause these workarounds to break in the future.
*/
// NB: If the error-handling workarounds stop working, we should consider introducing the same
// x-amzn-request-id header check in Apache instrumentation for suppressing spans that we have in
// Netty instrumentation.
public class TracingRequestHandler extends RequestHandler2 {

@Override
public void beforeRequest(Request<?> request) {
public static final HandlerContextKey<Scope> SCOPE =
new HandlerContextKey<>(Scope.class.getName());

AmazonWebServiceRequest originalRequest = request.getOriginalRequest();
SpanKind kind = (isSqsProducer(originalRequest) ? SpanKind.PRODUCER : SpanKind.CLIENT);
public static final RequestHandler2 tracingHandler =
AwsSdkTracing.newBuilder(GlobalOpenTelemetry.get())
.setCaptureExperimentalSpanAttributes(
Config.get()
.getBooleanProperty(
"otel.instrumentation.aws-sdk.experimental-span-attributes", false))
.build()
.newRequestHandler();

Context parentContext = Context.current();
if (!tracer().shouldStartSpan(parentContext)) {
return;
}
Context context = tracer().startSpan(kind, parentContext, request);
@Override
public void beforeRequest(Request<?> request) {
tracingHandler.beforeRequest(request);
Context context = AwsSdkTracing.getOpenTelemetryContext(request);
Scope scope = context.makeCurrent();
request.addHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY, new ContextScopePair(context, scope));
}

private boolean isSqsProducer(AmazonWebServiceRequest request) {
return request
.getClass()
.getName()
.equals("com.amazonaws.services.sqs.model.SendMessageRequest");
request.addHandlerContext(SCOPE, scope);
}

@Override
public AmazonWebServiceRequest beforeMarshalling(AmazonWebServiceRequest request) {
if (SqsReceiveMessageRequestAccess.isInstance(request)) {
if (!SqsReceiveMessageRequestAccess.getAttributeNames(request)
.contains(SqsParentContext.AWS_TRACE_SYSTEM_ATTRIBUTE)) {
SqsReceiveMessageRequestAccess.withAttributeNames(
request, SqsParentContext.AWS_TRACE_SYSTEM_ATTRIBUTE);
}
}
return request;
return tracingHandler.beforeMarshalling(request);
}

@Override
public void afterResponse(Request<?> request, Response<?> response) {
if (SqsReceiveMessageRequestAccess.isInstance(request.getOriginalRequest())) {
afterConsumerResponse(request, response);
}
// close outstanding "client" span
ContextScopePair scope = request.getHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY);
if (scope == null) {
return;
}
request.addHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY, null);
scope.closeScope();
tracer().end(scope.getContext(), response);
}

/** Create and close CONSUMER span for each message consumed. */
private void afterConsumerResponse(Request<?> request, Response<?> response) {
Object receiveMessageResult = response.getAwsResponse();
List<?> messages = SqsReceiveMessageResultAccess.getMessages(receiveMessageResult);
for (Object message : messages) {
createConsumerSpan(message, request, response);
}
}

private void createConsumerSpan(Object message, Request<?> request, Response<?> response) {
Context parentContext =
SqsParentContext.ofSystemAttributes(SqsMessageAccess.getAttributes(message));
Context context = tracer().startSpan(SpanKind.CONSUMER, parentContext, request);
tracer().end(context, response);
tracingHandler.afterResponse(request, response);
}

@Override
public void afterError(Request<?> request, Response<?> response, Exception e) {
ContextScopePair scope = request.getHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY);
tracingHandler.afterError(request, response, e);
finish(request);
}

private static void finish(Request<?> request) {
Scope scope = request.getHandlerContext(SCOPE);
if (scope == null) {
return;
}
request.addHandlerContext(CONTEXT_SCOPE_PAIR_CONTEXT_KEY, null);
scope.closeScope();
tracer().endExceptionally(scope.getContext(), e);
scope.close();
request.addHandlerContext(SCOPE, null);
}
}
2 changes: 1 addition & 1 deletion instrumentation/aws-sdk/aws-sdk-1.11/javaagent/src/test/groovy/SnsTracingTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import org.testcontainers.utility.DockerImageName
import spock.lang.Ignore
import spock.lang.Shared

@Ignore("Requires https://github.com/localstack/localstack/issues/3669 to work with localstack")
class SnsTracingTest extends AgentInstrumentationSpecification {

@Shared
Expand Down Expand Up @@ -76,7 +77,6 @@ class SnsTracingTest extends AgentInstrumentationSpecification {
return ctr.getTopicArn()
}

@Ignore("Requires https://github.com/localstack/localstack/issues/3669 to work with localstack")
def "simple SNS producer - SQS consumer services"() {
setup:
String queueName = "snsToSqsTestQueue"
Expand Down
Loading