-
Notifications
You must be signed in to change notification settings - Fork 293
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
Inject trace context into AWS Step Functions input #7585
Open
DylanLovesCoffee
wants to merge
24
commits into
master
Choose a base branch
from
dylan/sfn-trace-ctx
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+352
−0
Open
Changes from 14 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
7f8721d
inject step functions trace context
DylanLovesCoffee a974597
Merge branch 'master' into dylan/sfn-trace-ctx
DylanLovesCoffee 7ecfa8c
remove previous logic
DylanLovesCoffee fa36832
aws sfn instrumentation
DylanLovesCoffee 22c652b
Merge branch 'master' into dylan/sfn-trace-ctx
DylanLovesCoffee eb642b0
muzzle
DylanLovesCoffee 0778f3d
single char
DylanLovesCoffee f9d0eea
Merge branch 'master' into dylan/sfn-trace-ctx
DylanLovesCoffee 421856b
update muzzle
DylanLovesCoffee 895341a
refactor input attr injector
DylanLovesCoffee 9d76dec
refactor interceptor
DylanLovesCoffee aba1119
cleanup test
DylanLovesCoffee e56f2d3
Merge branch 'master' into dylan/sfn-trace-ctx
DylanLovesCoffee 3039780
Merge branch 'master' into dylan/sfn-trace-ctx
DylanLovesCoffee 370df6d
use JsonBuffer
DylanLovesCoffee a644a8b
catch exceptions
DylanLovesCoffee fc6e570
Merge branch 'master' into dylan/sfn-trace-ctx
nhulston ee83940
Merge branch 'master' into dylan/sfn-trace-ctx
nhulston 973cd79
add build.gradle comment
nhulston 96f7f39
fix SfnClientInstrumentation class header
nhulston fdb01a2
update InputAttributeInjector to use `datadog.json` component https:/…
nhulston 8417f6e
fix injection
nhulston 0aac239
test edge cases
nhulston 2992508
Merge branch 'master' into dylan/sfn-trace-ctx
nhulston File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
29 changes: 29 additions & 0 deletions
29
dd-java-agent/instrumentation/aws-java-sfn-2.0/build.gradle
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
muzzle { | ||
pass { | ||
group = "software.amazon.awssdk" | ||
module = "sfn" | ||
versions = "[2.15.35,)" | ||
assertInverse = true | ||
} | ||
} | ||
|
||
apply from: "$rootDir/gradle/java.gradle" | ||
|
||
addTestSuiteForDir('latestDepTest', 'test') | ||
addTestSuiteExtendingForDir('latestDepForkedTest', 'latestDepTest', 'test') | ||
|
||
dependencies { | ||
compileOnly group: 'software.amazon.awssdk', name: 'sfn', version: '2.15.35' | ||
|
||
// Include httpclient instrumentation for testing because it is a dependency for aws-sdk. | ||
testImplementation project(':dd-java-agent:instrumentation:apache-httpclient-4') | ||
testImplementation project(':dd-java-agent:instrumentation:aws-java-sdk-2.2') | ||
testImplementation 'software.amazon.awssdk:sfn:2.15.35' | ||
testImplementation libs.testcontainers | ||
|
||
latestDepTestImplementation group: 'software.amazon.awssdk', name: 'sfn', version: '+' | ||
} | ||
|
||
tasks.withType(Test).configureEach { | ||
usesService(testcontainersLimit) | ||
} |
44 changes: 44 additions & 0 deletions
44
...fn-2.0/src/main/java/datadog/trace/instrumentation/aws/v2/sfn/InputAttributeInjector.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package datadog.trace.instrumentation.aws.v2.sfn; | ||
|
||
import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
|
||
public class InputAttributeInjector { | ||
public static String buildTraceContext(AgentSpan span) { | ||
// Extract span tags | ||
StringBuilder spanTagsJSON = new StringBuilder(); | ||
spanTagsJSON.append('{'); | ||
span.getTags() | ||
.forEach( | ||
(tagKey, tagValue) -> | ||
spanTagsJSON | ||
.append('"') | ||
.append(tagKey) | ||
.append("\":\"") | ||
.append(tagValue) | ||
.append("\",")); | ||
spanTagsJSON.setLength(spanTagsJSON.length() - 1); // remove trailing comma | ||
spanTagsJSON.append('}'); | ||
|
||
// Build DD trace context object | ||
String ddTraceContextJSON = | ||
String.format( | ||
"\"_datadog\": { \"x-datadog-trace-id\": \"%s\",\"x-datadog-parent-id\":\"%s\", \"x-datadog-tags\": %s }", | ||
span.getTraceId().toString(), span.getSpanId(), spanTagsJSON); | ||
|
||
return ddTraceContextJSON; | ||
} | ||
|
||
public static String getModifiedInput(String request, String ddTraceContextJSON) { | ||
StringBuilder modifiedInput = new StringBuilder(request); | ||
int startPos = modifiedInput.indexOf("{"); | ||
nhulston marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int endPos = modifiedInput.lastIndexOf("}"); | ||
String inputContent = modifiedInput.substring(startPos + 1, endPos); | ||
if (inputContent.isEmpty()) { | ||
modifiedInput.insert(endPos, ddTraceContextJSON); | ||
} else { | ||
modifiedInput.insert( | ||
endPos, ",".concat(ddTraceContextJSON)); // prepend comma to existing input | ||
} | ||
return modifiedInput.toString(); | ||
} | ||
} |
50 changes: 50 additions & 0 deletions
50
...-2.0/src/main/java/datadog/trace/instrumentation/aws/v2/sfn/SfnClientInstrumentation.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package datadog.trace.instrumentation.aws.v2.sfn; | ||
|
||
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; | ||
import static net.bytebuddy.matcher.ElementMatchers.isMethod; | ||
|
||
import com.google.auto.service.AutoService; | ||
import datadog.trace.agent.tooling.Instrumenter; | ||
import datadog.trace.agent.tooling.InstrumenterModule; | ||
import java.util.List; | ||
import net.bytebuddy.asm.Advice; | ||
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; | ||
|
||
/** AWS SDK v2 Step Function instrumentation */ | ||
@AutoService(InstrumenterModule.class) | ||
public final class SfnClientInstrumentation extends InstrumenterModule.Tracing | ||
implements Instrumenter.ForSingleType { | ||
|
||
public SfnClientInstrumentation() { | ||
super("sfn", "aws-sdk"); | ||
} | ||
|
||
@Override | ||
public String instrumentedType() { | ||
return "software.amazon.awssdk.core.client.builder.SdkDefaultClientBuilder"; | ||
} | ||
|
||
@Override | ||
public void methodAdvice(MethodTransformer transformer) { | ||
transformer.applyAdvice( | ||
isMethod().and(named("resolveExecutionInterceptors")), | ||
SfnClientInstrumentation.class.getName() + "$AwsSfnBuilderAdvice"); | ||
} | ||
|
||
@Override | ||
public String[] helperClassNames() { | ||
return new String[] {packageName + ".SfnInterceptor", packageName + ".InputAttributeInjector"}; | ||
} | ||
|
||
public static class AwsSfnBuilderAdvice { | ||
@Advice.OnMethodExit(suppress = Throwable.class) | ||
public static void addHandler(@Advice.Return final List<ExecutionInterceptor> interceptors) { | ||
for (ExecutionInterceptor interceptor : interceptors) { | ||
if (interceptor instanceof SfnInterceptor) { | ||
return; | ||
} | ||
} | ||
interceptors.add(new SfnInterceptor()); | ||
} | ||
} | ||
} |
63 changes: 63 additions & 0 deletions
63
...s-java-sfn-2.0/src/main/java/datadog/trace/instrumentation/aws/v2/sfn/SfnInterceptor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package datadog.trace.instrumentation.aws.v2.sfn; | ||
|
||
import datadog.trace.bootstrap.InstanceStore; | ||
import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
import software.amazon.awssdk.core.SdkRequest; | ||
import software.amazon.awssdk.core.interceptor.Context; | ||
import software.amazon.awssdk.core.interceptor.ExecutionAttribute; | ||
import software.amazon.awssdk.core.interceptor.ExecutionAttributes; | ||
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; | ||
import software.amazon.awssdk.services.sfn.model.StartExecutionRequest; | ||
import software.amazon.awssdk.services.sfn.model.StartSyncExecutionRequest; | ||
|
||
public class SfnInterceptor implements ExecutionInterceptor { | ||
|
||
public static final ExecutionAttribute<AgentSpan> SPAN_ATTRIBUTE = | ||
InstanceStore.of(ExecutionAttribute.class) | ||
.putIfAbsent("DatadogSpan", () -> new ExecutionAttribute<>("DatadogSpan")); | ||
|
||
public SfnInterceptor() {} | ||
|
||
@Override | ||
public SdkRequest modifyRequest( | ||
nhulston marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Context.ModifyRequest context, ExecutionAttributes executionAttributes) { | ||
final AgentSpan span = executionAttributes.getAttribute(SPAN_ATTRIBUTE); | ||
// StartExecutionRequest | ||
if (context.request() instanceof StartExecutionRequest) { | ||
StartExecutionRequest request = (StartExecutionRequest) context.request(); | ||
if (request.input() == null) { | ||
return request; | ||
} | ||
return injectTraceContext(span, request); | ||
} | ||
|
||
// StartSyncExecutionRequest | ||
if (context.request() instanceof StartSyncExecutionRequest) { | ||
StartSyncExecutionRequest request = (StartSyncExecutionRequest) context.request(); | ||
if (request.input() == null) { | ||
return request; | ||
} | ||
return injectTraceContext(span, request); | ||
} | ||
|
||
return context.request(); | ||
} | ||
|
||
private SdkRequest injectTraceContext(AgentSpan span, StartExecutionRequest request) { | ||
String ddTraceContextJSON = InputAttributeInjector.buildTraceContext(span); | ||
// Inject the trace context into the Step Function input | ||
String modifiedInput = | ||
InputAttributeInjector.getModifiedInput(request.input(), ddTraceContextJSON); | ||
|
||
return request.toBuilder().input(modifiedInput).build(); | ||
} | ||
|
||
private SdkRequest injectTraceContext(AgentSpan span, StartSyncExecutionRequest request) { | ||
String ddTraceContextJSON = InputAttributeInjector.buildTraceContext(span); | ||
// Inject the trace context into the Step Function input | ||
String modifiedInput = | ||
InputAttributeInjector.getModifiedInput(request.input(), ddTraceContextJSON); | ||
|
||
return request.toBuilder().input(modifiedInput).build(); | ||
} | ||
} |
135 changes: 135 additions & 0 deletions
135
dd-java-agent/instrumentation/aws-java-sfn-2.0/src/test/groovy/SfnClientTest.groovy
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
import datadog.trace.agent.test.naming.VersionedNamingTestBase | ||
import datadog.trace.agent.test.utils.TraceUtils | ||
import datadog.trace.api.DDSpanTypes | ||
import datadog.trace.bootstrap.instrumentation.api.Tags | ||
import groovy.json.JsonSlurper | ||
import org.testcontainers.containers.GenericContainer | ||
import org.testcontainers.utility.DockerImageName | ||
import software.amazon.awssdk.services.sfn.SfnClient | ||
import software.amazon.awssdk.services.sfn.model.StartExecutionResponse | ||
import software.amazon.awssdk.regions.Region | ||
import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider | ||
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials | ||
import spock.lang.Shared | ||
|
||
import java.time.Duration | ||
|
||
import static datadog.trace.agent.test.utils.TraceUtils.basicSpan | ||
|
||
|
||
abstract class SfnClientTest extends VersionedNamingTestBase { | ||
@Shared GenericContainer localStack | ||
@Shared SfnClient sfnClient | ||
DylanLovesCoffee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@Shared String testStateMachineARN | ||
@Shared Object endPoint | ||
|
||
def setupSpec() { | ||
localStack = new GenericContainer(DockerImageName.parse("localstack/localstack")) | ||
.withExposedPorts(4566) | ||
.withEnv("SERVICES", "stepfunctions") | ||
.withReuse(true) | ||
.withStartupTimeout(Duration.ofSeconds(120)) | ||
localStack.start() | ||
endPoint = "http://" + localStack.getHost() + ":" + localStack.getMappedPort(4566) | ||
sfnClient = SfnClient.builder() | ||
.endpointOverride(URI.create(endPoint)) | ||
.region(Region.US_EAST_1) | ||
.credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("test", "test"))) | ||
.build() | ||
|
||
def response = sfnClient.createStateMachine { builder -> | ||
builder.name("testStateMachine") | ||
.definition("{\"StartAt\": \"HelloWorld\", \"States\": {\"HelloWorld\": {\"Type\": \"Pass\", \"End\": true}}}") | ||
.build() | ||
} | ||
testStateMachineARN = response.stateMachineArn() | ||
} | ||
|
||
def cleanupSpec() { | ||
sfnClient.close() | ||
localStack.stop() | ||
} | ||
|
||
def "Step Functions span is created"() { | ||
when: | ||
StartExecutionResponse response | ||
TraceUtils.runUnderTrace('parent', { | ||
response = sfnClient.startExecution { builder -> | ||
builder.stateMachineArn(testStateMachineARN) | ||
.input("{\"key\": \"value\"}") | ||
.build() | ||
} | ||
}) | ||
|
||
then: | ||
assertTraces(1) { | ||
trace(2) { | ||
basicSpan(it, "parent") | ||
span { | ||
serviceName service() | ||
operationName operation() | ||
resourceName "Sfn.StartExecution" | ||
spanType DDSpanTypes.HTTP_CLIENT | ||
errored false | ||
measured true | ||
childOf(span(0)) | ||
tags { | ||
"$Tags.COMPONENT" "java-aws-sdk" | ||
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT | ||
"$Tags.HTTP_URL" endPoint+'/' | ||
"$Tags.HTTP_METHOD" "POST" | ||
"$Tags.HTTP_STATUS" 200 | ||
"$Tags.PEER_PORT" localStack.getMappedPort(4566) | ||
"$Tags.PEER_HOSTNAME" localStack.getHost() | ||
"aws.service" "Sfn" | ||
"aws.operation" "StartExecution" | ||
"aws.agent" "java-aws-sdk" | ||
"aws.requestId" response.responseMetadata().requestId() | ||
"aws_service" "Sfn" | ||
defaultTags() | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
def "Trace context is injected to Step Functions input"() { | ||
when: | ||
StartExecutionResponse response | ||
TraceUtils.runUnderTrace('parent', { | ||
response = sfnClient.startExecution { builder -> | ||
builder.stateMachineArn(testStateMachineARN) | ||
.input("{\"key\": \"value\"}") | ||
.build() | ||
} | ||
}) | ||
|
||
then: | ||
def execution = sfnClient.describeExecution { builder -> | ||
builder.executionArn(response.executionArn()) | ||
.build() | ||
} | ||
def input = new JsonSlurper().parseText(execution.input()) | ||
input["key"] == "value" | ||
input["_datadog"]["x-datadog-trace-id"] != null | ||
input["_datadog"]["x-datadog-parent-id"] != null | ||
input["_datadog"]["x-datadog-tags"] != null | ||
} | ||
} | ||
|
||
class SfnClientV0Test extends SfnClientTest { | ||
@Override | ||
int version() { | ||
0 | ||
} | ||
|
||
@Override | ||
String service() { | ||
return "java-aws-sdk" | ||
} | ||
|
||
@Override | ||
String operation() { | ||
return "aws.http" | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worries me because it is a potential injection vector. I would prefer the use of a JsonBuffer that handles escaping properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I updated this with the JsonBuffer but when I run muzzle locally it fails with
Any ideas on if I did something very obviously wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into the same muzzle issue trying to use a JSON parsing library when doing the EventBridge instrumentation. I was basically told we can't use additional libraries in instrumentations and to do the JSON parsing manually with stringbuilder.
There are obviously security concerns with this, so it looks like we're blocked until there is a standardized way to parse JSONs in instrumentations in this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just merge #7973 It should provide API to build JSON payload from instrumentations (see
JsonWriter
andJsonMapper
).