feat: add aws xray header context extraction #3898

wconti27 · 2023-12-20T18:13:14Z

What does this PR do?

Add code to extract Trace Context Propagation from AWSTraceHeader, which is used by dd-trace-java to propagate context for some AWS services.

Motivation

Improving AWS propagation & dd-trace-js / dd-trace-java compatability.

Plugin Checklist

Additional Notes

Security

Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

…extraction-aws-kinesis

github-actions · 2023-12-20T18:14:02Z

Overall package size

Self size: 8.26 MB
Deduped: 94.55 MB
No deduping: 95.07 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.2.2 | 29.27 MB | 29.27 MB | | @datadog/native-appsec | 8.3.0 | 19.37 MB | 19.38 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.6.0 | 2.58 MB | 2.72 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.0.1 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

codecov · 2023-12-20T18:14:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.67%. Comparing base (de47a4b) to head (7d99016).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3898      +/-   ##
==========================================
- Coverage   85.23%   83.67%   -1.57%     
==========================================
  Files         247      112     -135     
  Lines       10961     4563    -6398     
  Branches       33       33              
==========================================
- Hits         9343     3818    -5525     
+ Misses       1618      745     -873

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pr-commenter · 2023-12-20T18:23:22Z

Benchmarks

Benchmark execution time: 2024-12-13 20:33:13

Comparing candidate commit 33ee7de in PR branch conti/add-aws-xray-header-context-extraction with baseline commit 83c6928 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 264 metrics, 2 unstable metrics.

Qard

Needs a test to verify the injection works correctly. Also, is support for this something we actually have consensus on and is expected to exist across all languages, or are we just implementing this because Java happened to have it?

wconti27 · 2023-12-27T16:35:40Z

Needs a test to verify the injection works correctly. Also, is support for this something we actually have consensus on and is expected to exist across all languages, or are we just implementing this because Java happened to have it?

There's no consensus on which should be used by default at the moment. This is mainly being added since Java is the most popular language for APM, and if we want propagation between a Java producer, we need to be able to extract context using the AwsTraceHeader

rochdev · 2023-12-28T21:19:32Z

packages/datadog-plugin-aws-sdk/src/services/sqs.js


-    const parsedAttributes = this.parseDatadogAttributes(datadogAttribute)
-    if (parsedAttributes) return this.tracer.extract('text_map', parsedAttributes)
+    if (message.Attributes && message.Attributes.AWSTraceHeader) {


How does this interact with W3C tracestate? We have just started providing guarantees that the tracestate is always kept even if we use our own extraction mechanism. For example, tracer.extract will now look at both Datadog and W3C headers to ensure the tracestate is kept. This doesn't seem to be handled here.

In general, I agree with @Qard that this change is significant enough that it shouldn't be done without proper consensus and a specification. Any changes to propagation are extremely difficult to change after the fact since any change would break compatibility with previous versions, including other languages.

Follow up question to this; were the same concerns considered for the Java dd-trace-java support for this functionality? (I believe it was this PR: DataDog/dd-trace-java#4730)

Language and platform agnostic interoperability between dd-trace libraries would deliver significant immediate value.

If we presume that the tracestate issue is resolved and the value is persisted, are there other concerns with the AWS-specific logic encapsulated in an SQS-specific implementation?

I'm obviously not the original PR author but I'd like to do what I can to help incorporate this functionality, even if it means pretending I'm a qualified project manager. 😆

vandonr · 2024-01-22T13:34:17Z

packages/datadog-plugin-aws-sdk/src/services/sqs.js

+      if (!request.params.AttributeNames) {
+        request.params.AttributeNames = ['AWSTraceHeader']
+      } else if (
+        !request.params.AttributeNames.includes('AWSTraceHeader') &&
+        !request.params.AttributeNames.includes('All')
+      ) {
+        request.params.AttributeNames.push('AWSTraceHeader')
+      }


Can you explain why you request an attribute with key 'AWSTraceHeader' while I see on the Java side that it's written with the key "X-Amzn-Trace-Id"? Shouldn't it be the same key on both sides ?

Ok wait I think I found the answer here : https://docs.aws.amazon.com/xray/latest/devguide/xray-services-sqs.html
If I understand correctly, what's written on produce to the http request in the X-Amzn-Trace-Id gets added to the message system attributes under the key AWSTraceHeader

rbonestell · 2024-04-04T19:52:00Z

I've been searching for this solution for a while, and anxiously await the merge and resolution to this! This will help many of us to implement proper tracing for our headless NodeJS services which use SQS!

This reverts commit abf28ca.

jeebay · 2024-04-16T20:48:05Z

I've similarly been waiting for this to land as it would clean up disconnected spans in DataDog. We have a polyglot environment and the proposed changes above work great for Java. It would be helpful to have the same behavior across languages.

@Qard @rochdev are there any blockers to validating or merging the proposed changes? I only see linting issues in the diff. I'd love to contribute to get this over the line.

ConnorSinnott · 2024-05-24T00:02:06Z

Is this still alive? We're really looking forward to this implementation. Its a bit glaring that the lambdas support this feature but containerized services don't.

jsonmorris · 2024-11-08T21:29:08Z

Hi all - any movement on this PR?

wconti27 and others added 30 commits December 11, 2023 14:08

fix aws tests

2d39509

Merge branch 'master' into conti/fix-aws-sdk-tests

a3f31e7

fix kinesis timeouts

8f7f569

fix error

bbc5897

increase timeout

51fe166

try again

107e02b

no timeout test

af16096

add dsm for sns sqs and kinesis

aa84b7e

fix failing tests

1923d87

add debugging statemtn

b3b35b6

fix tests

c118fd3

fix sns tests

afcefb1

fix again

f014b2c

try env variable to enable

eb13676

fix lint

b0cb24b

Merge branch 'master' into conti/add-dsm-for-aws-services

6449e96

add message payload size tests for sns / kinesis

62aad55

add more payloadSize tests

43bb3f1

fix kinesis payload size

3f38244

fix kinesis tests

d8e7bff

Merge branch 'master' into conti/add-dsm-for-aws-services

2b88ce5

add context propagation to kinesis

564bff3

more tests

3a692aa

increase timeout

0e9010d

rewrite SQS to set DSM checkpoint for each message

c4a7d54

change payloadSize calculations

8fc0843

fix payload size error

8c5f653

use stream name for checkpoint if arn unavailable

73519f3

Merge branch 'conti/add-dsm-for-aws-services' into conti/add-context-…

5a11c04

…extraction-aws-kinesis

add xray propagation

6e0d695

wconti27 requested review from a team as code owners December 20, 2023 18:13

wconti27 requested a review from jbertran December 20, 2023 18:13

wconti27 changed the base branch from master to conti/add-context-extraction-aws-kinesis December 20, 2023 18:13

remove test that i don't remember adding

8cb98ef

fix failing tests

b9c8bf3

Qard suggested changes Dec 21, 2023

View reviewed changes

rochdev requested changes Dec 28, 2023

View reviewed changes

vandonr reviewed Jan 22, 2024

View reviewed changes

Base automatically changed from conti/add-context-extraction-aws-kinesis to master January 25, 2024 19:15

wconti27 added 6 commits April 4, 2024 18:50

clean up code

bea6673

Merge branch 'master' into conti/add-aws-xray-header-context-extraction

c210127

Revert all changes except propagation class

8528904

add baggage tags

2b58872

change encoding / decoding of hashes to match other tracers

abf28ca

Revert "change encoding / decoding of hashes to match other tracers"

7d99016

This reverts commit abf28ca.

wconti27 added 3 commits December 13, 2024 14:07

Merge branch 'master' into conti/add-aws-xray-header-context-extraction

03008e6

add inject method

f7a542f

clean up code

33ee7de

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add aws xray header context extraction #3898

feat: add aws xray header context extraction #3898

wconti27 commented Dec 20, 2023

github-actions bot commented Dec 20, 2023 •

edited

Loading

codecov bot commented Dec 20, 2023 •

edited

Loading

pr-commenter bot commented Dec 20, 2023 •

edited

Loading

Qard left a comment

wconti27 commented Dec 27, 2023

rochdev Dec 28, 2023

rbonestell Apr 4, 2024

vandonr Jan 22, 2024

vandonr Jan 22, 2024 •

edited

Loading

rbonestell commented Apr 4, 2024

jeebay commented Apr 16, 2024

ConnorSinnott commented May 24, 2024

jsonmorris commented Nov 8, 2024

feat: add aws xray header context extraction #3898

Are you sure you want to change the base?

feat: add aws xray header context extraction #3898

Conversation

wconti27 commented Dec 20, 2023

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

Security

github-actions bot commented Dec 20, 2023 • edited Loading

Overall package size

codecov bot commented Dec 20, 2023 • edited Loading

Codecov Report

pr-commenter bot commented Dec 20, 2023 • edited Loading

Benchmarks

Qard left a comment

Choose a reason for hiding this comment

wconti27 commented Dec 27, 2023

rochdev Dec 28, 2023

Choose a reason for hiding this comment

rbonestell Apr 4, 2024

Choose a reason for hiding this comment

vandonr Jan 22, 2024

Choose a reason for hiding this comment

vandonr Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

rbonestell commented Apr 4, 2024

jeebay commented Apr 16, 2024

ConnorSinnott commented May 24, 2024

jsonmorris commented Nov 8, 2024

github-actions bot commented Dec 20, 2023 •

edited

Loading

codecov bot commented Dec 20, 2023 •

edited

Loading

pr-commenter bot commented Dec 20, 2023 •

edited

Loading

vandonr Jan 22, 2024 •

edited

Loading