-
Notifications
You must be signed in to change notification settings - Fork 335
Improve log safety #5631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve log safety #5631
Conversation
Overall package sizeSelf size: 9.3 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 | | 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 #5631 +/- ##
==========================================
+ Coverage 79.04% 79.10% +0.05%
==========================================
Files 512 513 +1
Lines 23403 23458 +55
==========================================
+ Hits 18500 18556 +56
+ Misses 4903 4902 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Datadog ReportBranch report: ✅ 0 Failed, 929 Passed, 0 Skipped, 15m 50.41s Total Time |
BenchmarksBenchmark execution time: 2025-04-30 18:08:37 Comparing candidate commit 6e0b0fc in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 1268 metrics, 53 unstable metrics. scenario:plugin-bluebird-control-22
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you wanted to get rid of the Proxy
entirely? Does this change fully address your original concerns?
That would be ideal. When looking into why this was done, it did seem like the best short term trade-off to keep the proxy but to not access the receiver anymore. That addresses at least the issues that we recently faced. |
The logger instrumentation is using a proxy. Calling Reflect.get with a receiver might actually cause issues at times. Instead, just access the target directly. That is easier and safer in this situation. On top, remove the special handling for Symbol.toStringTag. That was added for the testing library and should not have been there in the first place. As a drive-by fix, it also improves the performance.
42194da
to
6e0b0fc
Compare
The logger instrumentation is using a proxy. Calling Reflect.get with a receiver might actually cause issues at times. Instead, just access the target directly. That is easier and safer in this situation. On top, remove the special handling for Symbol.toStringTag. That was added for the testing library and should not have been there in the first place. As a drive-by fix, it also improves the performance a very tiny bit.
The logger instrumentation is using a proxy. Calling Reflect.get with a receiver might actually cause issues at times. Instead, just access the target directly. That is easier and safer in this situation. On top, remove the special handling for Symbol.toStringTag. That was added for the testing library and should not have been there in the first place. As a drive-by fix, it also improves the performance a very tiny bit.
The logger instrumentation is using a proxy. Calling Reflect.get with a receiver might actually cause issues at times. Instead, just access the target directly. That is easier and safer in this situation.
On top, remove the special handling for Symbol.toStringTag. That was added for the testing library and should not have been there in the first place.
As a drive-by fix, it also improves the performance / cleans up the code.