[serverless] Add S3 Span Pointers #4875

nhulston · 2024-11-11T18:34:05Z

What does this PR do?

Adds span pointers in S3 for putObject, copyObject, and completeMultipartUpload requests.

Span pointers are similar to Span Links, but for cases when it is impossible to pass the Trace ID and Span ID between the spans that need to be linked.

When the calculated hashes for the upstream and downstream lambdas match, the Datadog backend will automatically link the two traces together.

When clicking on the linked span, a new tab opens linking to the downstream Lambda function that was triggered by this S3 object update.

Motivation

This feature already exists in Python, and I'm working on adding it to all other runtimes (Node, .NET, Java, Golang).

Testing Guidelines

Easy: Checkout this span, enable the feature flag, and you will see that it's pointing to the downstream Lambda.

Run this Lambda function with the event payload

{
  "shouldPutObject": false,
  "shouldCopyObject": false,
  "shouldMultipartUpload": false,
  "useV2": true
}

and change one of the bools to true. Also, try both with AWS SDK v2 and v3. Enable the span pointers feature flag, and check Datadog to ensure that the spans are properly linked.

I also added tests:

# Unit tests
yarn test packages/dd-trace/test/span_pointers.spec.js

# Integration tests
docker run -d -p 4566:4566 localstack/localstack
PLUGINS=aws-sdk SERVICES=localstack DD_DATA_STREAMS_ENABLED=true yarn test:plugins:ci

Additional Notes

This handles the 'upstream' case, for Lambdas that make S3 requests. I have another PR in datadog-lambda-js to handle the 'downstream' case, for Lambdas that are triggered by S3 updates. Add S3 Downstream Span Pointers datadog-lambda-js#587
S3 and Dynamo are supported in Python. I will soon open another PR to add support for Dynamo.
packages/dd-trace/src/span_pointers.js is a public API, since datadog-lambda-js will reuse the exported fields.

github-actions · 2024-11-11T18:34:49Z

Overall package size

Self size: 8.01 MB
Deduped: 94.3 MB
No deduping: 94.64 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.2.2 | 29.27 MB | 29.27 MB | | @datadog/native-appsec | 8.2.1 | 19.18 MB | 19.19 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.5.0 | 2.51 MB | 2.65 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 | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 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 | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 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 | | 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 | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 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}

pr-commenter · 2024-11-11T18:42:30Z

Benchmarks

Benchmark execution time: 2024-11-15 21:56:03

Comparing candidate commit 6e7d38f in PR branch nicholas.hulston/s3-span-pointers with baseline commit 25ae8e7 in branch master.

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

nhulston · 2024-11-11T20:31:35Z

packages/datadog-plugin-aws-sdk/test/s3.spec.js

          AWS = require(`../../../versions/${s3ClientName}@${version}`).get()

-          s3 = new AWS.S3({ endpoint: 'http://127.0.0.1:4567', s3ForcePathStyle: true, region: 'us-east-1' })
+          s3 = new AWS.S3({ endpoint: 'http://127.0.0.1:4566', s3ForcePathStyle: true, region: 'us-east-1' })


The port here was wrong

codecov · 2024-11-11T20:53:23Z

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 58.76%. Comparing base (9794630) to head (44785f7).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/dd-trace/src/opentelemetry/span.js	33.33%	2 Missing ⚠️
packages/dd-trace/src/opentracing/span.js	0.00%	2 Missing ⚠️
packages/dd-trace/src/noop/span.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4875      +/-   ##
============================================
  Coverage          ?   58.76%              
============================================
  Files             0      280     +280     
  Lines             0    13017   +13017     
============================================
+ Hits              0     7649    +7649     
- Misses            0     5368    +5368

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

nhulston · 2024-11-11T20:55:13Z

packages/datadog-plugin-aws-sdk/test/s3.spec.js

+          // Fix for LocationConstraint issue - only for SDK v2
+          if (s3ClientName === 'aws-sdk') {
+            s3.api.globalEndpoint = '127.0.0.1'
+          }


Fixes an issue with localstack. I followed the 'quick workaround' mentioned in this LocalStack comment

nhulston · 2024-11-12T19:32:24Z

packages/dd-trace/src/span_pointers.js

+/**
+ * Generates a unique hash for an S3 object using its bucket name, key, and ETag
+ * https://github.com/DataDog/dd-span-pointer-rules/blob/main/AWS/S3/Object/README.md
+ * @param {string} bucketName - The name of the S3 bucket containing the object
+ * @param {string} objectKey - The full path/key of the object in the bucket
+ * @param {string} eTag - The ETag value from S3, which may be wrapped in quotes
+ * @returns {string|null} A hash uniquely identifying the S3 request, or null if missing parameters.
+ */


Adding detailed comments here, since this function is public API and will be used in datadog-lambda-js

nhulston · 2024-11-12T19:36:14Z

packages/datadog-plugin-aws-sdk/test/s3.spec.js

-        after(done => {
-          s3.deleteBucket({ Bucket: bucketName }, () => {
-            done()
-          })


No need to do this, since we already call

after(async () => { await resetLocalStackS3()

…ultipartUpload` requests.

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

apiarian-datadog

some notes, though mostly on the span pointers and their api. the javascript looks fine to my inexpert eyes

packages/datadog-plugin-aws-sdk/src/base.js

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

packages/datadog-plugin-aws-sdk/test/s3.spec.js

packages/dd-trace/src/span_pointers.js

…s for trace+span id; add debug logs

packages/datadog-plugin-aws-sdk/src/base.js

apiarian-datadog

looks good from the span pointer perspective! i'll leave the js-specific things to other reviewers, though.

khanayan123 · 2024-11-14T21:27:34Z

Mainly looks good to me just need some unit tests for addSpanPointer to increase code coverage in these files:

packages/dd-trace/test/noop.spec.js
packages/dd-trace/test/opentracing/span.spec.js
packages/dd-trace/test/opentracing/span.spec.js

khanayan123

Sorry, accidentally approved the changes. Just need unit tests in the above files and then it should be good

Dismissing as per previous comment

nhulston · 2024-11-15T15:26:47Z

@khanayan123

Mainly looks good to me just need some unit tests for addSpanPointer to increase code coverage in these files:

packages/dd-trace/test/noop.spec.js
packages/dd-trace/test/opentracing/span.spec.js
packages/dd-trace/test/opentracing/span.spec.js

Added tests for two files, but I don't think it makes sense to add tests to noop.spec.js

apiarian-datadog

still looks fine, though i've added some structural comments after seeing how this is used in the lambda layer

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

packages/dd-trace/src/span_pointers.js

apiarian-datadog · 2024-11-15T15:29:32Z

packages/dd-trace/src/span_pointers.js

+ * @param {string} eTag - The ETag value from S3, which may be wrapped in quotes
+ * @returns {string} A hash uniquely identifying the S3 request.
+ */
+function generateS3PointerHash (bucketName, objectKey, eTag) {


a little weird that we have an aws s3-specifc funciton here in a file that seems to be general span pointers code?

Agreed, I will create a single generateHash function and separate whatever parameters are provided with a pipe. That way I can reuse it once I get to dynamo

packages/dd-trace/src/opentelemetry/span.js

…ion.

…wnstream callers don't have to worry about passing it.

khanayan123

Some minor comments, everything else looks good

khanayan123 · 2024-11-15T20:48:15Z

packages/dd-trace/src/span_pointers.js

+const SPAN_POINTER_DIRECTION = Object.freeze({
+  UPSTREAM: 'u',
+  DOWNSTREAM: 'd'
+})


NIT: can we move these constants to: packages/dd-trace/src/constants.js

khanayan123 · 2024-11-15T20:49:21Z

packages/dd-trace/src/span_pointers.js

+ * @param {string[]} components - Array of strings to hash
+ * @returns {string} A 32-character hash uniquely identifying the components
+ */
+function generatePointerHash (components) {


NIT: rather then adding a new file, would it make more sense for this function to be in:

packages/datadog-plugin-aws-sdk/src/base.js
Or
packages/dd-trace/src/util.js

We can't do base.js because we need to access it from dd-trace in https://github.com/DataDog/datadog-lambda-js, but we can move it to util.js!

nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 40640b8 to 6094019 Compare November 11, 2024 18:34

nhulston added the semver-minor label Nov 11, 2024

nhulston commented Nov 11, 2024

View reviewed changes

nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 492496e to 7cba2af Compare November 11, 2024 20:37

nhulston commented Nov 11, 2024

View reviewed changes

nhulston force-pushed the nicholas.hulston/s3-span-pointers branch 2 times, most recently from 2cee24b to f2dc82f Compare November 12, 2024 19:22

nhulston commented Nov 12, 2024

View reviewed changes

Add span pointer info on S3 putObject, copyObject, and `completeM…

bb321d2

…ultipartUpload` requests.

nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from f2dc82f to 2a7a3d5 Compare November 12, 2024 19:39

Unit tests

4ca5b30

nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 2a7a3d5 to 4ca5b30 Compare November 12, 2024 20:18

nhulston marked this pull request as ready for review November 12, 2024 20:58

nhulston requested review from a team as code owners November 12, 2024 20:58

purple4reina reviewed Nov 12, 2024

View reviewed changes

packages/datadog-plugin-aws-sdk/src/services/s3.js Show resolved Hide resolved

nhulston and others added 2 commits November 13, 2024 09:57

small improvement

b5ef4c7

Merge branch 'master' into nicholas.hulston/s3-span-pointers

a36db0f

apiarian-datadog reviewed Nov 13, 2024

View reviewed changes

khanayan123 self-requested a review November 13, 2024 18:56

nhulston added 2 commits November 13, 2024 16:52

Create addSpanPointer() so we don't have to export a context with 0…

550f616

…s for trace+span id; add debug logs

Add integration test for completeMultipartUpload; update unit test

833d1b7

nhulston requested a review from apiarian-datadog November 13, 2024 21:53

Rename to addSpanPointers()

f11b2e6

khanayan123 reviewed Nov 14, 2024

View reviewed changes

packages/datadog-plugin-aws-sdk/src/base.js Show resolved Hide resolved

Update comments and make getting eTag more reliable

20bfdc2

nhulston mentioned this pull request Nov 14, 2024

Add S3 Downstream Span Pointers DataDog/datadog-lambda-js#587

Draft

11 tasks

Validate parameters before calling generateS3PointerHash

44785f7

nhulston requested a review from khanayan123 November 14, 2024 17:56

Merge branch 'master' into nicholas.hulston/s3-span-pointers

cef75b2

apiarian-datadog previously approved these changes Nov 14, 2024

View reviewed changes

khanayan123 previously approved these changes Nov 15, 2024

View reviewed changes

khanayan123 self-requested a review November 15, 2024 02:23

nhulston dismissed apiarian-datadog’s stale review via 0a9a7f4 November 15, 2024 15:07

add unit tests

346ec99

nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 1536b1f to a7c44f3 Compare November 15, 2024 15:23

nhulston requested a review from a team as a code owner November 15, 2024 15:23

nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from a7c44f3 to 346ec99 Compare November 15, 2024 15:24

Merge branch 'master' into nicholas.hulston/s3-span-pointers

3cca413

nhulston requested a review from apiarian-datadog November 15, 2024 15:25

apiarian-datadog previously approved these changes Nov 15, 2024

View reviewed changes

Rename var to SPAN_LINK_POINTER_KIND; standardize the hashing funct…

be10fa8

…ion.

nhulston dismissed apiarian-datadog’s stale review via be10fa8 November 15, 2024 15:51

Set the span link kind in the addSpanPointer() functions so that do…

a0ce63e

…wnstream callers don't have to worry about passing it.

nhulston force-pushed the nicholas.hulston/s3-span-pointers branch from 4faec6f to a0ce63e Compare November 15, 2024 16:29

khanayan123 reviewed Nov 15, 2024

View reviewed changes

Move constants to constants.js; move generatePointerHash to util.js

6e7d38f

nhulston requested a review from khanayan123 November 15, 2024 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[serverless] Add S3 Span Pointers #4875

[serverless] Add S3 Span Pointers #4875

nhulston commented Nov 11, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024 •

edited

Loading

pr-commenter bot commented Nov 11, 2024 •

edited

Loading

nhulston Nov 11, 2024

codecov bot commented Nov 11, 2024 •

edited

Loading

nhulston Nov 11, 2024

nhulston Nov 12, 2024 •

edited

Loading

nhulston Nov 12, 2024

apiarian-datadog left a comment

apiarian-datadog left a comment

khanayan123 commented Nov 14, 2024

khanayan123 left a comment •

edited

Loading

nhulston commented Nov 15, 2024 •

edited

Loading

apiarian-datadog left a comment

apiarian-datadog Nov 15, 2024

nhulston Nov 15, 2024 •

edited

Loading

khanayan123 left a comment

khanayan123 Nov 15, 2024

khanayan123 Nov 15, 2024

nhulston Nov 15, 2024

[serverless] Add S3 Span Pointers #4875

Are you sure you want to change the base?

[serverless] Add S3 Span Pointers #4875

Conversation

nhulston commented Nov 11, 2024 • edited Loading

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

github-actions bot commented Nov 11, 2024 • edited Loading

Overall package size

pr-commenter bot commented Nov 11, 2024 • edited Loading

Benchmarks

nhulston Nov 11, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2024 • edited Loading

Codecov Report

nhulston Nov 11, 2024

Choose a reason for hiding this comment

nhulston Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

nhulston Nov 12, 2024

Choose a reason for hiding this comment

apiarian-datadog left a comment

Choose a reason for hiding this comment

apiarian-datadog left a comment

Choose a reason for hiding this comment

khanayan123 commented Nov 14, 2024

khanayan123 left a comment • edited Loading

Choose a reason for hiding this comment

nhulston commented Nov 15, 2024 • edited Loading

apiarian-datadog left a comment

Choose a reason for hiding this comment

apiarian-datadog Nov 15, 2024

Choose a reason for hiding this comment

nhulston Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

khanayan123 left a comment

Choose a reason for hiding this comment

khanayan123 Nov 15, 2024

Choose a reason for hiding this comment

khanayan123 Nov 15, 2024

Choose a reason for hiding this comment

nhulston Nov 15, 2024

Choose a reason for hiding this comment

nhulston commented Nov 11, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024 •

edited

Loading

pr-commenter bot commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 11, 2024 •

edited

Loading

nhulston Nov 12, 2024 •

edited

Loading

khanayan123 left a comment •

edited

Loading

nhulston commented Nov 15, 2024 •

edited

Loading

nhulston Nov 15, 2024 •

edited

Loading