dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

Topher c/apmsvls 52 fixing payload tagging

Open TophrC-dd opened this issue 3 months ago • 6 comments

What does this PR do?

This fixes a couple of issues with payload tagging logic. Currently if using an invalid json path can break tracing.

After investigating the reason why the trace breaks is due to trying to write to property that doesn't exist. This cannot be evaluated at configuration time as we do not have the user's JSON schema. So I have just implemented a check to see if the the parent and parentProperty exist before trying to set this.

This is based on the stacktrace generated:

2025-09-04T19:16:13.181Z	1d5edca0-30cc-4b4f-9153-98b480162b26	ERROR	[dd.trace_id=5438775504825836037 dd.span_id=5438775504825836037] Uncaught Exception 	
{"errorType":"TypeError","errorMessage":"Cannot set properties of null (setting 'null')","stack":["TypeError: Cannot set properties of null (setting 'null')","    at /var/task/node_modules/dd-trace/packages/dd-trace/src/payload-tagging/index.js:56:42"," 
  at JSONPath._handleCallback (/var/task/node_modules/jsonpath-plus/dist/index-node-cjs.cjs:1660:5)","    at JSONPath._trace (/var/task/node_modules/jsonpath-plus/dist/index-node-cjs.cjs:1688:10)","    at JSONPath._trace 
(/var/task/node_modules/jsonpath-plus/dist/index-node-cjs.cjs:1758:17)","    at JSONPath.evaluate 
(/var/task/node_modules/jsonpath-plus/dist/index-node-cjs.cjs:1611:23)","    at new JSONPath 
(/var/task/node_modules/jsonpath-plus/dist/index-node-cjs.cjs:1554:22)","    at JSONPath 
(/var/task/node_modules/jsonpath-plus/dist/index-node-cjs.cjs:1514:14)","    at redact (/var/task/node_modules/dd-
trace/packages/dd-trace/src/payload-tagging/index.js:55:7)","    at computeTags (/var/task/node_modules/dd-
trace/packages/dd-trace/src/payload-tagging/index.js:84:3)","    at tagsFromRequest (/var/task/node_modules/dd-
trace/packages/dd-trace/src/payload-tagging/index.js:89:10)"]}

Motivation

I want to make the use of this feature to be more intuitive along with catching when the user makes a mistake so that they are not questioning why their payloads are redacting or in this case their whole trace disappears.

Plugin Checklist

payload-tagging

Additional Notes

TophrC-dd avatar Aug 28 '25 20:08 TophrC-dd

Overall package size

Self size: 4.39 MB Deduped: 5.21 MB No deduping: 5.21 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.0 | 68.46 kB | 797.03 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |

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

github-actions[bot] avatar Aug 28 '25 20:08 github-actions[bot]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 84.49%. Comparing base (b00ab50) to head (2803e27). :warning: Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6358   +/-   ##
=======================================
  Coverage   84.49%   84.49%           
=======================================
  Files         523      523           
  Lines       22447    22449    +2     
=======================================
+ Hits        18966    18968    +2     
  Misses       3481     3481           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Aug 28 '25 20:08 codecov[bot]

Benchmarks

Benchmark execution time: 2025-12-22 17:29:38

Comparing candidate commit 2803e27034bb6acd874f0c7e81f897c39ddbcee7 in PR branch TopherC/APMSVLS-52-Fixing-payload-tagging with baseline commit b00ab5001877f2142c405f9ed5812e07f6e1772a in branch master.

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

pr-commenter[bot] avatar Sep 04 '25 20:09 pr-commenter[bot]

The code is LGTM, we would just also need some tests. Do you know what happens in case a user passes through faulty rules?

@BridgeAR -- this will just return. Because the filter is incorrect there is nothing to redact or capture. Today we're trying to assign using a null property as a key.

TophrC-dd avatar Sep 18 '25 15:09 TophrC-dd

Hey @BridgeAR -- I see there is payload-tagging tests residing in here. Is this sufficient or should I add more testing?

Thanks, ~Topher

TophrC-dd avatar Oct 14 '25 19:10 TophrC-dd

⚠️ Tests

Fix all issues with Cursor

⚠️ Warnings

🧪 1 Test failed

❄️ Known flaky: tests.appsec.waf.test_telemetry.Test_TelemetryMetrics.test_waf_requests_match_traced_requests[uds-express4] from system_tests_suite (Datadog) (Fix with Cursor)
AssertionError: Number of requests in traces do not match waf.requests metric total
assert 5 == 4

self = <tests.appsec.waf.test_telemetry.Test_TelemetryMetrics object at 0x7f7d48dcc0b0>

    @bug(context.library < "[email protected]", reason="APPSEC-51509")
    def test_waf_requests_match_traced_requests(self):
        """Total waf.requests metric should match the number of requests in traces."""
        spans = [s for _, s in interfaces.library.get_root_spans()]
        spans = [
...

ℹ️ Info

❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 2803e27 | Docs | Datadog PR Page | Was this helpful? Give us feedback!

I change the commit title to something a little cleaner. It's not clear what payload tagging is though so the title could probably use more love. At any rate when squashing please ensure the commit title is well formed.

Oh, this is the AWS payload tagging project I worked on a while ago. I'll update the title to include the text "AWS" as that seems relevant.

tlhunter avatar Dec 22 '25 17:12 tlhunter