Use errorMonitor symbol instead of listening to the error event #5682
Conversation
Overall package sizeSelf size: 9.37 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/pprof | 5.8.0 | 12.55 MB | 12.92 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 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 | | mutexify | 1.4.0 | 5.71 kB | 8.74 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5682 +/- ##
=======================================
Coverage 79.04% 79.05%
=======================================
Files 515 515
Lines 23589 23590 +1
=======================================
+ Hits 18647 18648 +1
Misses 4942 4942 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Datadog ReportBranch report: ✅ 0 Failed, 986 Passed, 0 Skipped, 15m 24.78s Total Time |
BenchmarksBenchmark execution time: 2025-05-15 10:31:48 Comparing candidate commit 0d52cd1 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 1268 metrics, 54 unstable metrics. scenario:appsec-iast-no-vulnerability-iast-enabled-default-config-18
|
This is important to guarantee user code behaves as it did without the instrumentation. So user crashes should happen as before.
BridgeAR
force-pushed
the
BridgeAR/safer-on-error
branch
from
dd6a6fb to
d6d080c
Compare
| if (stream) { | ||
| newQuery.on('end', () => { | ||
| finish(null, []) | ||
| }) | ||
| newQuery.on(errorMonitor, (err) => { | ||
| finish(err) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
This was actually adding a second listener. Now, it's only a single one and only the monitor one.
There was a problem hiding this comment.
Well, it wasn't adding a second listener in case newQuery.callback is truthy. In that case it's the only one. However, one would assume that the callback override above should serve the same purpose, so I agree with you that removing of this if-statement seems like the right move. Have you verified that this code is covered by tests?
There was a problem hiding this comment.
The time this was added, tests were added that would fail if the above listener is removed. All callback cases are event emitters but it's fine, since we wait for that to finish. It was only important if there was no callback.
I did however do a closer check and added a test for failing cases as well.
Interestingly, the .then part is never going to be reached in any of our cases, even the promise related tests. The pg internals just all work with callbacks. I wonder if we want to remove that condition.
| if (stream) { | ||
| newQuery.on('end', () => { | ||
| finish(null, []) | ||
| }) | ||
| newQuery.on(errorMonitor, (err) => { | ||
| finish(err) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Well, it wasn't adding a second listener in case newQuery.callback is truthy. In that case it's the only one. However, one would assume that the callback override above should serve the same purpose, so I agree with you that removing of this if-statement seems like the right move. Have you verified that this code is covered by tests?
This is important to guarantee user code behaves as it did without
the instrumentation. So user crashes should happen as before.
I did not verify if we handle this else-wise and I mainly wanted to open this to look into this closer to make sure this is indeed what we want or not.