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

Jump to bottom

[Bugfix] Remove binary _datadog attribute if present in JMS SQS instrumentation to avoid crash #7283

Merged
merged 8 commits into from
Jul 29, 2024
+72 −17
Merged

[Bugfix] Remove binary _datadog attribute if present in JMS SQS instrumentation to avoid crash #7283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
12cbc16
remove binary datadog attribute if present in JMS SQS instrumentation
vandonr Jul 5, 2024
c61452a
Merge remote-tracking branch 'origin/master' into vandonr/fix
vandonr Jul 18, 2024
d24977e
trying to add some dependencies
vandonr Jul 23, 2024
64fbf8d
Merge remote-tracking branch 'origin/master' into vandonr/fix
vandonr Jul 23, 2024
1555e91
bump version of elasticmq
vandonr Jul 23, 2024
6101a9c
change javax version
vandonr Jul 23, 2024
a280baf
remove extra queue creation that I needed locally
vandonr Jul 23, 2024
e02b841
cleanup in v2 test
vandonr Jul 23, 2024
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
4 changes: 3 additions & 1 deletion dd-java-agent/instrumentation/aws-java-sqs-1.0/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ dependencies {
testImplementation project(':dd-java-agent:instrumentation:jms')

// SQS<->JMS testing:
testImplementation group: 'org.elasticmq', name: 'elasticmq-rest-sqs_2.13', version: '1.4.7'
testImplementation "jakarta.xml.bind:jakarta.xml.bind-api:2.3.2"
testImplementation "org.glassfish.jaxb:jaxb-runtime:2.3.2"
Comment on lines +27 to +28
Copy link
Contributor Author

Choose a reason for hiding this comment

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

without those, we get a JAXBException in com.amazonaws.util.StringUtils.fromByteBuffer

testImplementation group: 'org.elasticmq', name: 'elasticmq-rest-sqs_2.13', version: '1.6.5'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to bump this otherwise we sere getting an error 500 when sending binary message attributes

testImplementation group: 'com.amazonaws', name: 'amazon-sqs-java-messaging-lib', version: '1.0.8'

latestDepTestImplementation group: 'com.amazonaws', name: 'aws-java-sdk-sqs', version: '+'
Expand Down
23 changes: 21 additions & 2 deletions .../src/main/java/datadog/trace/instrumentation/aws/v1/sqs/SqsJmsMessageInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@

import com.amazon.sqs.javamessaging.message.SQSMessage;
import com.amazonaws.services.sqs.model.Message;
import com.amazonaws.services.sqs.model.MessageAttributeValue;
import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.InstrumenterModule;
import datadog.trace.api.Config;
import java.util.HashMap;
import java.util.Map;
import javax.jms.JMSException;
import net.bytebuddy.asm.Advice;
Expand All @@ -28,10 +30,27 @@ public String instrumentedType() {
public void methodAdvice(MethodTransformer transformer) {
transformer.applyAdvice(
isConstructor().and(takesArgument(2, named("com.amazonaws.services.sqs.model.Message"))),
getClass().getName() + "$CopyTracePropertyAdvice");
getClass().getName() + "$JmsSqsMessageConstructorAdvice");
}

public static class CopyTracePropertyAdvice {
public static class JmsSqsMessageConstructorAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void onEnter(@Advice.Argument(2) Message sqsMessage) {
Map<String, MessageAttributeValue> messageAttributes = sqsMessage.getMessageAttributes();
MessageAttributeValue ddAttribute = messageAttributes.get("_datadog");
if (ddAttribute != null && "Binary".equals(ddAttribute.getDataType())) {
// binary message attributes are not supported by amazon-sqs-java-messaging-lib, and there
// is a chance we might introduce one, either when the message was sent from SNS or from a
// DD-instrumented Javascript app.
// When we reach this point, the value would already have been used by the aws-sqs
// instrumentation, so we can safely remove it.
Map<String, MessageAttributeValue> messageAttributesCopy = new HashMap<>(messageAttributes);
// need to copy to remove because the original is an UnmodifiableMap
messageAttributesCopy.remove("_datadog");
sqsMessage.withMessageAttributes(messageAttributesCopy);
}
}

@Advice.OnMethodExit(suppress = Throwable.class)
public static void onExit(
@Advice.Argument(2) Message sqsMessage, @Advice.FieldValue("properties") Map properties)
Expand Down
14 changes: 11 additions & 3 deletions dd-java-agent/instrumentation/aws-java-sqs-1.0/src/test/groovy/SqsClientTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import com.amazonaws.client.builder.AwsClientBuilder
import com.amazonaws.services.sqs.AmazonSQSClientBuilder
import com.amazonaws.services.sqs.model.Message
import com.amazonaws.services.sqs.model.MessageAttributeValue
import com.amazonaws.services.sqs.model.SendMessageRequest
import datadog.trace.agent.test.naming.VersionedNamingTestBase
import datadog.trace.agent.test.utils.TraceUtils
import datadog.trace.api.Config
Expand All @@ -24,6 +25,8 @@ import spock.lang.IgnoreIf
import spock.lang.Shared

import javax.jms.Session
import java.nio.ByteBuffer
import java.nio.charset.Charset

import static datadog.trace.agent.test.utils.TraceUtils.basicSpan
import static java.nio.charset.StandardCharsets.UTF_8
Expand Down Expand Up @@ -286,10 +289,14 @@ abstract class SqsClientTest extends VersionedNamingTestBase {
TEST_WRITER.clear()

when:
def ddMsgAttribute = new MessageAttributeValue()
.withBinaryValue(ByteBuffer.wrap("hello world".getBytes(Charset.defaultCharset())))
.withDataType("Binary")
connection.start()
TraceUtils.runUnderTrace('parent', {
client.sendMessage(queue.queueUrl, 'sometext')
})
TraceUtils.runUnderTrace('parent') {
client.sendMessage(new SendMessageRequest(queue.queueUrl, 'sometext')
.withMessageAttributes([_datadog: ddMsgAttribute]))
}
def message = consumer.receive()
consumer.receiveNoWait()

Expand Down Expand Up @@ -418,6 +425,7 @@ abstract class SqsClientTest extends VersionedNamingTestBase {
def expectedTraceProperty = 'X-Amzn-Trace-Id'.toLowerCase(Locale.ENGLISH).replace('-', '__dash__')
assert message.getStringProperty(expectedTraceProperty) =~
/Root=1-[0-9a-f]{8}-00000000${sendSpan.traceId.toHexStringPadded(16)};Parent=${DDSpanId.toHexStringPadded(sendSpan.spanId)};Sampled=1/
assert !message.propertyExists("_datadog")

cleanup:
session.close()
Expand Down
23 changes: 21 additions & 2 deletions .../src/main/java/datadog/trace/instrumentation/aws/v2/sqs/SqsJmsMessageInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.InstrumenterModule;
import datadog.trace.api.Config;
import java.util.HashMap;
import java.util.Map;
import javax.jms.JMSException;
import net.bytebuddy.asm.Advice;
import software.amazon.awssdk.services.sqs.model.Message;
import software.amazon.awssdk.services.sqs.model.MessageAttributeValue;

@AutoService(InstrumenterModule.class)
public class SqsJmsMessageInstrumentation extends AbstractSqsInstrumentation
Expand All @@ -29,10 +31,27 @@ public void methodAdvice(MethodTransformer transformer) {
transformer.applyAdvice(
isConstructor()
.and(takesArgument(2, named("software.amazon.awssdk.services.sqs.model.Message"))),
getClass().getName() + "$CopyTracePropertyAdvice");
getClass().getName() + "$JmsSqsMessageConstructorAdvice");
}

public static class CopyTracePropertyAdvice {
public static class JmsSqsMessageConstructorAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void onEnter(@Advice.Argument(value = 2, readOnly = false) Message sqsMessage) {
Map<String, MessageAttributeValue> messageAttributes = sqsMessage.messageAttributes();
MessageAttributeValue ddAttribute = messageAttributes.get("_datadog");
if (ddAttribute != null && "Binary".equals(ddAttribute.dataType())) {
// binary message attributes are not supported by amazon-sqs-java-messaging-lib, and there
// is a chance we might introduce one, either when the message was sent from SNS or from a
// DD-instrumented Javascript app.
// When we reach this point, the value would already have been used by the aws-sqs
// instrumentation, so we can safely remove it.
Map<String, MessageAttributeValue> messageAttributesCopy = new HashMap<>(messageAttributes);
// need to copy to remove because the original is an UnmodifiableMap
messageAttributesCopy.remove("_datadog");
sqsMessage = sqsMessage.toBuilder().messageAttributes(messageAttributesCopy).build();
}
}

@Advice.OnMethodExit(suppress = Throwable.class)
public static void onExit(
@Advice.Argument(2) Message sqsMessage, @Advice.FieldValue("properties") Map properties)
Expand Down
25 changes: 16 additions & 9 deletions dd-java-agent/instrumentation/aws-java-sqs-2.0/src/test/groovy/SqsClientTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
import static datadog.trace.agent.test.utils.TraceUtils.basicSpan
import static java.nio.charset.StandardCharsets.UTF_8

import com.amazon.sqs.javamessaging.ProviderConfiguration
import com.amazon.sqs.javamessaging.SQSConnectionFactory
import datadog.trace.agent.test.naming.VersionedNamingTestBase
import datadog.trace.agent.test.utils.TraceUtils
import datadog.trace.api.Config
import datadog.trace.api.DDTags
import datadog.trace.api.DDSpanId
import datadog.trace.api.DDSpanTypes
import datadog.trace.api.DDTags
import datadog.trace.api.config.GeneralConfig
import datadog.trace.api.naming.SpanNaming
import datadog.trace.bootstrap.instrumentation.api.InstrumentationTags
Expand All @@ -22,15 +19,18 @@ import software.amazon.awssdk.core.SdkSystemSetting
import software.amazon.awssdk.regions.Region
import software.amazon.awssdk.services.sqs.SqsClient
import software.amazon.awssdk.services.sqs.model.CreateQueueRequest
import software.amazon.awssdk.services.sqs.model.ReceiveMessageRequest
import software.amazon.awssdk.services.sqs.model.SendMessageRequest
import software.amazon.awssdk.services.sqs.model.Message
import software.amazon.awssdk.services.sqs.model.MessageAttributeValue
import software.amazon.awssdk.services.sqs.model.ReceiveMessageRequest
import software.amazon.awssdk.services.sqs.model.SendMessageRequest
import spock.lang.IgnoreIf
import spock.lang.Shared

import javax.jms.Session

import static datadog.trace.agent.test.utils.TraceUtils.basicSpan
import static java.nio.charset.StandardCharsets.UTF_8

abstract class SqsClientTest extends VersionedNamingTestBase {

def setup() {
Expand Down Expand Up @@ -297,10 +297,16 @@ abstract class SqsClientTest extends VersionedNamingTestBase {
TEST_WRITER.clear()

when:
def ddMsgAttribute = MessageAttributeValue.builder()
.dataType("Binary")
.binaryValue(SdkBytes.fromUtf8String("hello world")).build()
connection.start()
TraceUtils.runUnderTrace('parent', {
client.sendMessage(SendMessageRequest.builder().queueUrl(queue.queueUrl).messageBody('sometext').build())
})
TraceUtils.runUnderTrace('parent') {
client.sendMessage(SendMessageRequest.builder()
.queueUrl(queue.queueUrl)
.messageBody('sometext')
.messageAttributes([_datadog: ddMsgAttribute]).build())
}
def message = consumer.receive()
consumer.receiveNoWait()

Expand Down Expand Up @@ -428,6 +434,7 @@ abstract class SqsClientTest extends VersionedNamingTestBase {
def expectedTraceProperty = 'X-Amzn-Trace-Id'.toLowerCase(Locale.ENGLISH).replace('-', '__dash__')
assert message.getStringProperty(expectedTraceProperty) =~
/Root=1-[0-9a-f]{8}-00000000${sendSpan.traceId.toHexStringPadded(16)};Parent=${DDSpanId.toHexStringPadded(sendSpan.spanId)};Sampled=1/
assert !message.propertyExists("_datadog")

cleanup:
session.close()
Expand Down
Loading