Skip to content

Filter out log stack frames from error log messages#5519

Merged
szegedi merged 2 commits intomasterfrom
szegedi/omit-log-error-frames
Apr 9, 2025
Merged

Filter out log stack frames from error log messages#5519
szegedi merged 2 commits intomasterfrom
szegedi/omit-log-error-frames

Conversation

@szegedi
Copy link
Copy Markdown
Contributor

@szegedi szegedi commented Apr 2, 2025

What does this PR do?

Ensures that when log writer writes a stack trace for an error message it will omit the logging code frames from it.

Motivation

Log writer will transform a message into an error with a stack trace before logging it. However then it always logs the frames for its own code as well. This causes the frames:

at /path/to/my/app/node_modules/dd-trace/packages/dd-trace/src/log/writer.js:69:46
at withNoop (/path/to/my/app/node_modules/dd-trace/packages/dd-trace/src/log/writer.js:21:3)
at onError (/path/to/my/app/node_modules/dd-trace/packages/dd-trace/src/log/writer.js:69:18)
at Channel.publish (node:diagnostics_channel:143:9)
at Object.error (/path/to/my/app/node_modules/dd-trace/packages/dd-trace/src/log/index.js:82:20)

to be present in all logged errors. This can lead to a confusion as people looking to diagnose problems will Google for these frames, find unrelated issues, and presume they're related. I'm submitting this because just yesterday I had a TEE ask if maybe #5111 is a related issue to something completely different based on the match on these frames.

Additional Notes

Jira: PROF-11569

@szegedi szegedi requested a review from a team as a code owner April 2, 2025 09:47
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 2, 2025

Overall package size

Self size: 9.24 MB
Deduped: 101.55 MB
No deduping: 102.06 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.0 | 29.83 MB | 29.83 MB | | @datadog/native-appsec | 8.5.1 | 19.26 MB | 19.27 MB | | @datadog/native-iast-taint-tracking | 3.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.6.0 | 9.79 MB | 10.16 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 | 3.1.0 | 2.37 MB | 2.52 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 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 | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | dc-polyfill | 0.1.6 | 24.56 kB | 24.56 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
Copy link
Copy Markdown

codecov Bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.27%. Comparing base (b94052c) to head (5e5fb2e).
Report is 29 commits behind head on master.

Files with missing lines Patch % Lines
packages/dd-trace/src/log/writer.js 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5519      +/-   ##
==========================================
+ Coverage   79.18%   79.27%   +0.08%     
==========================================
  Files         512      512              
  Lines       23158    23188      +30     
==========================================
+ Hits        18338    18382      +44     
+ Misses       4820     4806      -14     

☔ 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.

@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented Apr 2, 2025

Datadog Report

Branch report: szegedi/omit-log-error-frames
Commit report: 1c51d5e
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 926 Passed, 0 Skipped, 15m 24.87s Total Time

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Apr 2, 2025

Benchmarks

Benchmark execution time: 2025-04-02 11:56:11

Comparing candidate commit edd698d in PR branch szegedi/omit-log-error-frames with baseline commit b94052c in branch master.

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

@szegedi szegedi force-pushed the szegedi/omit-log-error-frames branch from 6262148 to edd698d Compare April 2, 2025 11:38
@szegedi szegedi enabled auto-merge (squash) April 2, 2025 12:41
Error.stackTraceLimit = 0
const e = new Error(formatted)
Error.stackTraceLimit = l
Error.captureStackTrace(e, stackTraceLimitFunction)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to fall back to onError in case there's no stackTraceLimitFunction?

Suggested change
Error.captureStackTrace(e, stackTraceLimitFunction)
Error.captureStackTrace(e, stackTraceLimitFunction ?? onError)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In our only usage we definitely configure the property so the default won't be used ever, but it definitely doesn't hurt. I made a change although I set onError as the initial value of stackTraceLimitFunction instead of adding a ?? operator here.

@szegedi szegedi requested a review from watson April 4, 2025 14:43
Copy link
Copy Markdown
Member

@tlhunter tlhunter left a comment

Choose a reason for hiding this comment

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

It's worth noting that this may blast us with "new" errors as far as the error logging product is concerned since the stack traces will change.

@szegedi szegedi merged commit a5095be into master Apr 9, 2025
427 checks passed
@szegedi szegedi deleted the szegedi/omit-log-error-frames branch April 9, 2025 16:21
This was referenced Apr 10, 2025
@github-actions github-actions Bot mentioned this pull request Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants