Skip to content

wrap all callbacks in instrumentations with wrapFunction#4634

Merged
bengl merged 2 commits intomasterfrom
bengl/wrap-function-all-callbacks
Aug 28, 2024
Merged

wrap all callbacks in instrumentations with wrapFunction#4634
bengl merged 2 commits intomasterfrom
bengl/wrap-function-all-callbacks

Conversation

@bengl
Copy link
Copy Markdown
Collaborator

@bengl bengl commented Aug 27, 2024

These were the remaining areas with non-trivial code that might not be caught by try/catches when our code throws an exception. Adding wrapFunction adds that try/catch appropriately, but only when SSI is enabled. Otherwise these are all passthroughs. In the future, we may enable those try/catches at all times.

I considered the following to be out of scope:

  • Many (but not all) completely trivial wrappers.
  • Exceptions that might originate in async_hooks.

I may have missed some. It's an extremely difficult task to verify that all opportunities for throwing an exception are covered.

@bengl bengl requested review from a team as code owners August 27, 2024 19:16
@bengl bengl requested a review from liashenko August 27, 2024 19:16
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 27, 2024

Overall package size

Self size: 6.98 MB
Deduped: 58.21 MB
No deduping: 58.49 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.0.1 | 15.59 MB | 15.6 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.4.1 | 2.14 MB | 2.23 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.8.1 | 71.67 kB | 785.15 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 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 | | lru-cache | 7.14.0 | 74.95 kB | 74.95 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 | | 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.7 | 6.78 kB | 6.78 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

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Aug 27, 2024

Benchmarks

Benchmark execution time: 2024-08-28 14:23:11

Comparing candidate commit 043923f in PR branch bengl/wrap-function-all-callbacks with baseline commit 1f2914f in branch master.

Found 0 performance improvements and 8 performance regressions! Performance is the same for 253 metrics, 5 unstable metrics.

scenario:appsec-iast-no-vulnerability-control-18

  • 🟥 cpu_user_time [+29.854ms; +52.298ms] or [+5.267%; +9.228%]
  • 🟥 execution_time [+38.684ms; +82.015ms] or [+5.307%; +11.252%]
  • 🟥 instructions [+79.1M instructions; +82.2M instructions] or [+5.863%; +6.089%]

scenario:plugin-dns-with-tracer-18

  • 🟥 cpu_user_time [+78.473ms; +103.518ms] or [+11.112%; +14.658%]
  • 🟥 instructions [+113.3M instructions; +124.1M instructions] or [+10.232%; +11.215%]
  • 🟥 max_rss_usage [+5.070MB; +5.404MB] or [+7.272%; +7.751%]

scenario:plugin-http-client-with-tracer-18

  • 🟥 cpu_user_time [+64.726ms; +84.503ms] or [+6.300%; +8.225%]
  • 🟥 instructions [+95.5M instructions; +96.7M instructions] or [+6.309%; +6.388%]

@bengl bengl force-pushed the bengl/wrap-function-all-callbacks branch from 65c2f12 to 6807a88 Compare August 27, 2024 19:38
Qard
Qard previously approved these changes Aug 27, 2024
These were the remaining areas with non-trivial code that might not be
caught by try/catches when our code throws an exception. Adding
`wrapFunction` adds that try/catch appropriately, but only when SSI is
enabled. Otherwise these are all passthroughs. In the future, we may
enable those try/catches at all times.
@bengl bengl force-pushed the bengl/wrap-function-all-callbacks branch from 6807a88 to fa0b8f3 Compare August 27, 2024 20:06
Comment thread packages/datadog-instrumentations/src/grpc/client.js Outdated
Comment thread packages/datadog-instrumentations/src/winston.js Outdated
Comment thread packages/datadog-instrumentations/src/mongoose.js
@bengl
Copy link
Copy Markdown
Collaborator Author

bengl commented Aug 28, 2024

The two failing are all-green and next.js, so I'll just go ahead and merge.

@bengl bengl merged commit 474a735 into master Aug 28, 2024
@bengl bengl deleted the bengl/wrap-function-all-callbacks branch August 28, 2024 14:51
bengl added a commit that referenced this pull request Aug 29, 2024
These were the remaining areas with non-trivial code that might not be
caught by try/catches when our code throws an exception. Adding
`wrapFunction` adds that try/catch appropriately, but only when SSI is
enabled. Otherwise these are all passthroughs. In the future, we may
enable those try/catches at all times.
@bengl bengl mentioned this pull request Aug 29, 2024
bengl added a commit that referenced this pull request Aug 29, 2024
These were the remaining areas with non-trivial code that might not be
caught by try/catches when our code throws an exception. Adding
`wrapFunction` adds that try/catch appropriately, but only when SSI is
enabled. Otherwise these are all passthroughs. In the future, we may
enable those try/catches at all times.
@bengl bengl mentioned this pull request Aug 29, 2024
bengl added a commit that referenced this pull request Aug 30, 2024
These were the remaining areas with non-trivial code that might not be
caught by try/catches when our code throws an exception. Adding
`wrapFunction` adds that try/catch appropriately, but only when SSI is
enabled. Otherwise these are all passthroughs. In the future, we may
enable those try/catches at all times.
bengl added a commit that referenced this pull request Aug 30, 2024
These were the remaining areas with non-trivial code that might not be
caught by try/catches when our code throws an exception. Adding
`wrapFunction` adds that try/catch appropriately, but only when SSI is
enabled. Otherwise these are all passthroughs. In the future, we may
enable those try/catches at all times.
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