Fix span pointers env var & tests #5546

nhulston · 2025-04-07T20:11:03Z

What does this PR do?

Fixes dynamodb span pointer tests which were not running correctly.

Also fixes the trace.dynamoDb.tablePrimaryKeys env var.

Also, here I remove a duplicate log

Motivation

It looks someone accidentally renamed the env var trace.dynamoDb.tablePrimaryKeys to aws.dynamoDb.tablePrimaryKeys. This was not caught because the tests were not failing when expected.
(Originally, it was aws.dynamoDb but I changed it to trace.dynamoDb. Looks like later, someone must've rebased incorrectly and reverted my change in #5266)

Plugin Checklist

Additional Notes

https://datadoghq.atlassian.net/browse/SVLS-6602

github-actions · 2025-04-07T20:11:55Z

Overall package size

Self size: 9.33 MB
Deduped: 102.57 MB
No deduping: 103.09 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.1 | 29.73 MB | 29.73 MB | | @datadog/native-appsec | 8.5.2 | 19.33 MB | 19.34 MB | | @datadog/native-iast-taint-tracking | 3.3.1 | 13.99 MB | 13.99 MB | | @datadog/pprof | 5.7.1 | 9.51 MB | 9.88 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.4.0 | 2.77 MB | 5.42 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.13.1 | 117.64 kB | 839.26 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.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.8 | 25.08 kB | 25.08 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 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 · 2025-04-07T20:12:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.33%. Comparing base (3ed32fb) to head (fe9434a).
Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5546      +/-   ##
==========================================
+ Coverage   79.27%   79.33%   +0.06%     
==========================================
  Files         513      514       +1     
  Lines       23342    23416      +74     
==========================================
+ Hits        18504    18577      +73     
- Misses       4838     4839       +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

rochdev

This should be tested to avoid the same situation happening again.

pr-commenter · 2025-04-07T20:17:43Z

Benchmarks

Benchmark execution time: 2025-04-16 17:14:50

Comparing candidate commit fe9434a in PR branch nicholas.hulston/fix-dynamodb-span-ptr-env-var with baseline commit 3ed32fb in branch master.

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

scenario:startup-with-tracer-22

🟩 cpu_user_time [-21.250ms; -14.395ms] or [-10.957%; -7.423%]
🟩 execution_time [-19.531ms; -17.999ms] or [-8.349%; -7.694%]

datadog-datadog-prod-us1 · 2025-04-07T20:20:21Z

Datadog Report

Branch report: nicholas.hulston/fix-dynamodb-span-ptr-env-var
Commit report: f59c0d8
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 926 Passed, 0 Skipped, 9m 33.79s Total Time

nhulston · 2025-04-07T20:28:31Z

This should be tested to avoid the same situation happening again.

@rochdev
I have unit tests here, I'm not sure how they passed when the env var was initially changed. WDYT? https://github.com/DataDog/dd-trace-js/blob/nicholas.hulston/fix-dynamodb-span-ptr-env-var/packages/datadog-plugin-aws-sdk/test/dynamodb.spec.js#L325-L340

Looks like the config requires a default value to be set (see https://github.com/DataDog/dd-trace-js/blob/nicholas.hulston/fix-dynamodb-span-ptr-env-var/packages/dd-trace/src/config.js#L444-L445), but it seems this is not tested in unit tests.

This seems like a bigger issue with testing config.js.

I'm currently working on functional tests for Serverless; that's how I discovered this issue:
https://github.com/DataDog/serverless-e2e-tests/pull/28
This will notify the Serverless team of any future regression before we release, and the APM-serverless team has an OKR item to run these tests on every tracer repo

BridgeAR

The faulty change is about the default value. It was originally just undefined. Changing it to the correct name should still result in the same behavior. It is still a falsy value and that did not change by renaming the default.

AFAIC the issue is somewhere else.

nhulston · 2025-04-08T02:42:57Z

The faulty change is about the default value. It was originally just undefined. Changing it to the correct name should still result in the same behavior. It is still a falsy value and that did not change by renaming the default.

AFAIC the issue is somewhere else.

@BridgeAR
It definitely looks that way and I see where you're coming from, but the default value is actually required, see https://github.com/DataDog/dd-trace-js/blob/nicholas.hulston/fix-dynamodb-span-ptr-env-var/packages/dd-trace/src/config.js#L444-L445

I've tested manually and this one-liner fixes our regression

BridgeAR

I checked again and we seem to iterate over the default keys in the config to merge just those values. That is something we should probably fix. It is weird that we need a default value for any value to be defined later.
This will therefore indeed fix the issue.

After looking into the tests, it seems that those do not get executed properly. If I read the code correct, the test calls testSpanPointers() which returns a method that is never called and the test passes. Please always make sure that the test fails initially before letting it pass. That way we guarantee that our test actually tests the changed code. A good coding pattern for this is Test Driven Development.

I am going to approve this due to indeed fixing the issue but please fix the tests right afterwards. I will have a look at the config sometime during the next weeks.

rochdev

I don't see having tests elsewhere that don't fail on PRs and that might manually be checked before releasing a dependant library after releasing a broken dd-trace a satisfactory testing strategy. If there is a missing test for this feature, let's add it in this repo.

nhulston · 2025-04-11T16:56:08Z

I don't see having tests elsewhere that don't fail on PRs and that might manually be checked before releasing a dependant library after releasing a broken dd-trace a satisfactory testing strategy. If there is a missing test for this feature, let's add it in this repo.

The issue was that the dynamodb span pointer tests were not failing when they should have failed. I've fixed that issue in this PR. Thanks :)

…-var

BridgeAR

Thanks for looking into the tests! Now that they run, they seem to cause a lot of tests to fail 😅

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

Co-authored-by: Ruben Bridgewater <[email protected]>

… one test Co-authored-by: Ruben Bridgewater <[email protected]>

…-var

BridgeAR · 2025-04-14T15:37:19Z

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

-                operation: (callback) => {
-                  dynamo.deleteItem({
+                operation () {
+                  return util.promisify(dynamo.deleteItem).bind(dynamo)({


Could you please use a helper method that checks if the method already supports promises and to use that instead in those cases? That way we prevent the deprecation warning from popping up and having to fix this later.

That applies to all usages of util.promisify()

BridgeAR

LGTM with the deprecation warnings gone :)

rochdev · 2025-04-15T14:04:19Z

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

-            delete process.env.DD_TRACE_DYNAMODB_TABLE_PRIMARY_KEYS
-          })
+          async function testSpanPointers ({ env, expectedHashes, operation }) {
+            await agent.close({ ritmReset: false, wipe: true })


This should be done in after or afterEach, never in tests directly to avoid dependencies between tests.

It should actually be a beforeEach to match the current behavior (that is also reflected in the test failures).

Thanks. I moved the agent.close call into beforeEach. However, we still have to load the agent inside this method because we have to update the env var before loading the agent.

It should actually be a beforeEach to match the current behavior (that is also reflected in the test failures).

Why would it be needed in beforeEach if afterEach cleans up properly?

However, we still have to load the agent inside this method because we have to update the env var before loading the agent.

That's fine, although I think it could be refactored to happen in a before.

If we try to close the agent but it's already been closed, the tests fail. So the first test in this describe block will fail if we put it in afterEach.

Seems like an issue with the mock agent, but that's outside the scope of this PR.

I'm not a member of the APM team; just trying to fix a feature for Serverless

…-var

rochdev · 2025-04-17T15:57:08Z

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

-
-        before(async () => {
-          await agent.load('aws-sdk')
+          await agent.load()


Why the empty agent.load()?

Not sure, I didn't write these with configuration tests. Looks like we need to load the agent with a specific tracer config, and closing the agent when it's not already loaded causes failures. It fails without this line

I am just trying to fix a regression; these tests are unrelated

BridgeAR

Still LGTM. This is definitely a big improvement. It fixes the reported issue and the tests are now run. So no matter that there are a few more theoretical improvements about how these tests could be implemented, it's a big improvement.

nhulston added 2 commits April 7, 2025 16:05

fix env var

7991619

remove duplicate log

3b319c2

nhulston requested review from a team as code owners April 7, 2025 20:11

nhulston added semver-minor semver-patch and removed semver-minor labels Apr 7, 2025

rochdev requested changes Apr 7, 2025

View reviewed changes

nhulston requested a review from rochdev April 7, 2025 20:29

BridgeAR requested changes Apr 8, 2025

View reviewed changes

nhulston requested a review from BridgeAR April 8, 2025 02:43

BridgeAR previously approved these changes Apr 11, 2025

View reviewed changes

rochdev requested changes Apr 11, 2025

View reviewed changes

nhulston dismissed BridgeAR’s stale review via 0007c7b April 11, 2025 13:45

nhulston marked this pull request as draft April 11, 2025 14:40

nhulston force-pushed the nicholas.hulston/fix-dynamodb-span-ptr-env-var branch from 0007c7b to e2fa243 Compare April 11, 2025 16:40

nhulston added 4 commits April 11, 2025 12:51

fix dynamodb tests not running

09628f2

update config from env var before each test

e22245e

fix transactWriteItem test

0b5a71a

lint

4fe0deb

nhulston force-pushed the nicholas.hulston/fix-dynamodb-span-ptr-env-var branch from e2fa243 to 4fe0deb Compare April 11, 2025 16:53

Merge branch 'master' into nicholas.hulston/fix-dynamodb-span-ptr-env…

dd57a32

…-var

BridgeAR reviewed Apr 11, 2025

View reviewed changes

packages/datadog-plugin-aws-sdk/test/dynamodb.spec.js Outdated Show resolved Hide resolved

nhulston force-pushed the nicholas.hulston/fix-dynamodb-span-ptr-env-var branch from 412eeaf to 8af1ab3 Compare April 11, 2025 20:57

nhulston and others added 3 commits April 14, 2025 10:24

refactor and simplify dynamodb integration tests

08d7e7a

Co-authored-by: Ruben Bridgewater <[email protected]>

allow mock agent to be closed idempotently

8d7b243

Co-authored-by: Ruben Bridgewater <[email protected]>

fix race condition when mock agent is opened/closed multiple times in…

976b3f1

… one test Co-authored-by: Ruben Bridgewater <[email protected]>

nhulston force-pushed the nicholas.hulston/fix-dynamodb-span-ptr-env-var branch from ed6f96f to 976b3f1 Compare April 14, 2025 14:26

Merge branch 'master' into nicholas.hulston/fix-dynamodb-span-ptr-env…

73bc54b

…-var

nhulston marked this pull request as ready for review April 14, 2025 15:04

nhulston requested a review from rochdev April 14, 2025 15:04

BridgeAR reviewed Apr 14, 2025

View reviewed changes

nhulston requested a review from BridgeAR April 14, 2025 16:36

nhulston changed the title ~~Fix span pointers env var~~ Fix span pointers env var & tests Apr 14, 2025

fix deprecation warnings on v3 of aws-sdk

23f8f93

BridgeAR previously approved these changes Apr 15, 2025

View reviewed changes

rochdev reviewed Apr 15, 2025

View reviewed changes

close agent after each span pointer test

2fdb299

nhulston dismissed BridgeAR’s stale review via 2fdb299 April 16, 2025 14:56

nhulston added 3 commits April 16, 2025 10:56

Merge branch 'master' into nicholas.hulston/fix-dynamodb-span-ptr-env…

58c0685

…-var

refactor payload tagging tests

682c819

fix: reset agent before each span pointer test

fe9434a

nhulston requested a review from rochdev April 16, 2025 17:23

rochdev reviewed Apr 17, 2025

View reviewed changes

nhulston requested a review from rochdev April 17, 2025 18:27

BridgeAR approved these changes Apr 23, 2025

View reviewed changes

rochdev approved these changes Apr 23, 2025

View reviewed changes

BridgeAR merged commit 80ec575 into master Apr 23, 2025
437 checks passed

BridgeAR deleted the nicholas.hulston/fix-dynamodb-span-ptr-env-var branch April 23, 2025 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix span pointers env var & tests #5546

Fix span pointers env var & tests #5546

nhulston commented Apr 7, 2025 •

edited

Loading

github-actions bot commented Apr 7, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading

rochdev left a comment

pr-commenter bot commented Apr 7, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Apr 7, 2025 •

edited

Loading

nhulston commented Apr 7, 2025 •

edited

Loading

BridgeAR left a comment

nhulston commented Apr 8, 2025

BridgeAR left a comment

rochdev left a comment

nhulston commented Apr 11, 2025

BridgeAR left a comment

BridgeAR Apr 14, 2025 •

edited

Loading

BridgeAR left a comment

rochdev Apr 15, 2025

BridgeAR Apr 16, 2025

nhulston Apr 16, 2025

rochdev Apr 17, 2025 •

edited

Loading

nhulston Apr 17, 2025

rochdev Apr 17, 2025

nhulston Apr 17, 2025 •

edited

Loading

nhulston Apr 17, 2025

BridgeAR left a comment

Fix span pointers env var & tests #5546

Fix span pointers env var & tests #5546

Conversation

nhulston commented Apr 7, 2025 • edited Loading

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

github-actions bot commented Apr 7, 2025 • edited Loading

Overall package size

codecov bot commented Apr 7, 2025 • edited Loading

Codecov Report

rochdev left a comment

Choose a reason for hiding this comment

pr-commenter bot commented Apr 7, 2025 • edited Loading

Benchmarks

scenario:startup-with-tracer-22

datadog-datadog-prod-us1 bot commented Apr 7, 2025 • edited Loading

Datadog Report

nhulston commented Apr 7, 2025 • edited Loading

BridgeAR left a comment

Choose a reason for hiding this comment

nhulston commented Apr 8, 2025

BridgeAR left a comment

Choose a reason for hiding this comment

rochdev left a comment

Choose a reason for hiding this comment

nhulston commented Apr 11, 2025

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

rochdev Apr 15, 2025

Choose a reason for hiding this comment

BridgeAR Apr 16, 2025

Choose a reason for hiding this comment

nhulston Apr 16, 2025

Choose a reason for hiding this comment

rochdev Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

nhulston Apr 17, 2025

Choose a reason for hiding this comment

rochdev Apr 17, 2025

Choose a reason for hiding this comment

nhulston Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

nhulston Apr 17, 2025

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

nhulston commented Apr 7, 2025 •

edited

Loading

github-actions bot commented Apr 7, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading

pr-commenter bot commented Apr 7, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Apr 7, 2025 •

edited

Loading

nhulston commented Apr 7, 2025 •

edited

Loading

BridgeAR Apr 14, 2025 •

edited

Loading

rochdev Apr 17, 2025 •

edited

Loading

nhulston Apr 17, 2025 •

edited

Loading