Skip to content
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

[ci-visibility] Remove usage of .asyncResource in mocha plugin #4348

Merged
merged 1 commit into from
May 27, 2024

Conversation

juan-fernandez
Copy link
Collaborator

@juan-fernandez juan-fernandez commented May 24, 2024

What does this PR do?

Remove the usage of .asyncResource to check if a function has been wrapped already. Now we'll use another WeakSet wrappedFunctions to check whether the function is wrapped.

I've additionally fixed the plugin tests for mocha, which were not running since the merge of #4314, as seen by this GHA: https://github.com/DataDog/dd-trace-js/actions/runs/9218422124/job/25361943902

Screenshot 2024-05-24 at 12 59 37

It was not running because the instrumentation was split in three files and our test setup didn't allow for that.

After this change, they're running: https://github.com/DataDog/dd-trace-js/actions/runs/9222873007/job/25374958036?pr=4348

Motivation

It's deprecated: nodejs/node#46432

Plugin Checklist

No extra unit tests needed: just check that the ones we have are running and passing.

Copy link

Overall package size

Self size: 6.51 MB
Deduped: 60.56 MB
No deduping: 60.84 MB

Dependency sizes

name version self size total size
@datadog/native-iast-taint-tracking 2.1.0 14.91 MB 14.92 MB
@datadog/native-appsec 7.1.1 14.39 MB 14.4 MB
@datadog/pprof 5.3.0 9.85 MB 10.22 MB
protobufjs 7.2.5 2.77 MB 6.56 MB
@datadog/native-iast-rewriter 2.3.1 2.15 MB 2.24 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.7.4 70.19 kB 739.86 kB
msgpack-lite 0.1.26 201.16 kB 281.59 kB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.5.4 93.4 kB 123.8 kB
pprof-format 2.1.0 111.69 kB 111.69 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 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.2.4 51.22 kB 51.22 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

@juan-fernandez juan-fernandez marked this pull request as ready for review May 24, 2024 11:02
@juan-fernandez juan-fernandez requested review from a team as code owners May 24, 2024 11:02
@pr-commenter
Copy link

pr-commenter bot commented May 24, 2024

Benchmarks

Benchmark execution time: 2024-05-24 11:13:42

Comparing candidate commit 6a48c03 in PR branch juan-fernandez/fix-async-resource-mocha with baseline commit 95b5a41 in branch master.

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

scenario:plugin-graphql-with-depth-and-collapse-on-18

  • 🟩 max_rss_usage [-146.164MB; -134.320MB] or [-15.019%; -13.802%]

scenario:plugin-graphql-with-depth-on-max-18

  • 🟩 max_rss_usage [-145.059MB; -128.833MB] or [-14.905%; -13.238%]

@@ -469,7 +469,7 @@ addHook({
// Used to start and finish test session and test module
addHook({
name: 'mocha',
versions: ['>=8.0.0'],
versions: ['>=5.2.0'],
Copy link
Member

Choose a reason for hiding this comment

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

just a confirmation, you're making the version support lower than it was before (it's usually not a common thing so i'm just pointing it out)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah, I should've mentioned this. This file is not there for <8 so this should be a noop. I changed it because the testing matrix became way more complicated. We should probably fix our testing setup but that's for another PR 😄

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