-
Notifications
You must be signed in to change notification settings - Fork 323
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
dd-trace-api: handle returned promises #5245
Conversation
While #5240 allowed for objects to be returned, it did this by comparing strict equality. With promises, we add an additional `.then()` call, removing strict equality. In this change, comparison of _resolved_ values is used.
Overall package sizeSelf size: 8.63 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.4.0 | 29.44 MB | 29.44 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.5.1 | 9.79 MB | 10.17 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.8.0 | 2.6 MB | 2.74 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 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.11.2 | 112.74 kB | 835.4 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.0 | 109.9 kB | 109.9 kB | | semver | 7.7.1 | 96.67 kB | 96.67 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 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 | | 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5245 +/- ##
==========================================
- Coverage 81.18% 81.09% -0.09%
==========================================
Files 482 480 -2
Lines 21531 21442 -89
==========================================
- Hits 17479 17389 -90
- Misses 4052 4053 +1 ☔ View full report in Codecov by Sentry. |
@@ -84,7 +84,17 @@ module.exports = class DdTraceApiPlugin extends Plugin { | |||
objectMap.set(proxyVal, ret.value) | |||
ret.value = proxyVal | |||
} else if (ret.value && typeof ret.value === 'object' && passthroughRetVal !== ret.value) { | |||
throw new TypeError(`Objects need proxies when returned via API (${name})`) | |||
if (typeof ret.value.then === 'function') { |
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.
Do we have benchmarks for this whole function? It seems to be pretty complicated at this point, and everything ends up going through it.
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.
This brings up a good point. This is effectively test code to ensure dd-trace-api is implemented correctly. I'll just delete all of this (in a new PR) and re-do the testing in dd-trace-api's test code.
Datadog ReportBranch report: ✅ 0 Failed, 626 Passed, 0 Skipped, 14m 18.71s Total Time |
BenchmarksBenchmark execution time: 2025-02-11 20:11:57 Comparing candidate commit 0da33e0 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 909 metrics, 23 unstable metrics. scenario:plugin-graphql-with-async-hooks-22
|
While #5240 allowed for objects to be returned, it did this by comparing strict equality. With promises, we add an additional
.then()
call, removing strict equality. In this change, comparison of resolved values is used.