Skip to content

test: destroy active connections when stopping fake agent#5853

Merged
watson merged 1 commit intomasterfrom
watson/fake-agent-force-stop
Jun 7, 2025
Merged

test: destroy active connections when stopping fake agent#5853
watson merged 1 commit intomasterfrom
watson/fake-agent-force-stop

Conversation

@watson
Copy link
Copy Markdown
Collaborator

@watson watson commented Jun 7, 2025

The fake agent is used in tests and is stopped after the test is done.

When calling its stop function, it will wait for active connections (sockets) to close before considering itself stopped. This can add an unnecessary waiting period after each test, as the timeout on the socket by default is two seconds in our code-base:

const timeout = options.timeout || 2000

For integration tests making heavy use of fake agents, this can add several minutes of overhead that can be avoided by destroying the sockets when calling the stop function on the fake agent.

This mainly affects tests running on Node.js 18, as sockets there for some reason takes a little longer to clear out. I haven't spend time figuring out why, but here's a recent CI run of Debugger on master:

image

Compare that, to the same run in this PR:

image

The fake agent is used in tests and is stopped after the test is done.

When calling its `stop` function, it will wait for active connections
(sockets) to close before considering itself stopped. This adds an
unnessecary waiting period after each test, as the timeout on the socket
by default is two seconds in our code-base:

    https://github.com/DataDog/dd-trace-js/blob/90fe64b9845d04d6273ec2b7a36ba6a7844b8253/packages/dd-trace/src/exporters/common/request.js#L56

For integration tests making heavy use of fake agents, this adds several
minutes of overhead that can be avoided by destroying the sockets when
calling the `stop` function on the fake agent.
@watson watson self-assigned this Jun 7, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2025

Overall package size

Self size: 9.61 MB
Deduped: 104.55 MB
No deduping: 105.07 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.6.0 | 30.47 MB | 30.47 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.5.3 | 2.95 MB | 5.6 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.14.0 | 120.58 kB | 841.68 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.2 | 34.37 kB | 34.37 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.9 | 25.11 kB | 25.11 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.4 | 3.96 kB | 3.96 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.71%. Comparing base (9c58aef) to head (d1289e1).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5853      +/-   ##
==========================================
+ Coverage   80.62%   80.71%   +0.08%     
==========================================
  Files         458      464       +6     
  Lines       19757    19875     +118     
==========================================
+ Hits        15930    16043     +113     
- Misses       3827     3832       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@datadog-datadog-prod-us1
Copy link
Copy Markdown

Datadog Report

Branch report: watson/fake-agent-force-stop
Commit report: 78682a9
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 1249 Passed, 0 Skipped, 17m 34.16s Total Time

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Jun 7, 2025

Benchmarks

Benchmark execution time: 2025-06-07 07:33:39

Comparing candidate commit d1289e1 in PR branch watson/fake-agent-force-stop with baseline commit 9c58aef in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1275 metrics, 48 unstable metrics.

@watson watson marked this pull request as ready for review June 7, 2025 07:41
@watson watson requested a review from a team as a code owner June 7, 2025 07:41
Copy link
Copy Markdown
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Interesting finding!
LGTM, just left a suggestion to improve readability of touched code (that existed before)

Comment on lines 44 to 55
stop () {
if (!this.server) return Promise.resolve()

return new Promise((resolve) => {
if (!this.server?.listening) return resolve()
for (const socket of this._sockets) {
socket.destroy()
}
this._sockets.clear()
this.server.on('close', resolve)
this.server.close()
})
Copy link
Copy Markdown
Member

@BridgeAR BridgeAR Jun 7, 2025

Choose a reason for hiding this comment

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

We could make it very readable using once from the built-in events module.

Suggested change
stop () {
if (!this.server) return Promise.resolve()
return new Promise((resolve) => {
if (!this.server?.listening) return resolve()
for (const socket of this._sockets) {
socket.destroy()
}
this._sockets.clear()
this.server.on('close', resolve)
this.server.close()
})
async stop () {
if (!this.server?.listening) return
for (const socket of this._sockets) {
socket.destroy()
}
this._sockets.clear()
this.server.close()
return once(this.server, 'close')

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in follow up PR: #5854

@watson watson merged commit b7e4f8e into master Jun 7, 2025
512 checks passed
@watson watson deleted the watson/fake-agent-force-stop branch June 7, 2025 13:23
ghost pushed a commit that referenced this pull request Jun 8, 2025
The fake agent is used in tests and is stopped after the test is done.

When calling its `stop` function, it will wait for active connections (sockets)
to close before considering itself stopped. This can add an unnecessary waiting
period after each test, as the timeout on the socket by default is two seconds
in our code-base:

    https://github.com/DataDog/dd-trace-js/blob/90fe64b9845d04d6273ec2b7a36ba6a7844b8253/packages/dd-trace/src/exporters/common/request.js#L56

For integration tests making heavy use of fake agents, this can add several
minutes of overhead that can be avoided by destroying the sockets when calling
the `stop` function on the fake agent.

This mainly affects tests running on Node.js 18, as sockets there for some
reason takes a little longer to clear out.
@ghost ghost mentioned this pull request Jun 8, 2025
ghost pushed a commit that referenced this pull request Jun 12, 2025
The fake agent is used in tests and is stopped after the test is done.

When calling its `stop` function, it will wait for active connections (sockets)
to close before considering itself stopped. This can add an unnecessary waiting
period after each test, as the timeout on the socket by default is two seconds
in our code-base:

    https://github.com/DataDog/dd-trace-js/blob/90fe64b9845d04d6273ec2b7a36ba6a7844b8253/packages/dd-trace/src/exporters/common/request.js#L56

For integration tests making heavy use of fake agents, this can add several
minutes of overhead that can be avoided by destroying the sockets when calling
the `stop` function on the fake agent.

This mainly affects tests running on Node.js 18, as sockets there for some
reason takes a little longer to clear out.
@ghost ghost mentioned this pull request Jun 12, 2025
szegedi pushed a commit that referenced this pull request Jun 12, 2025
The fake agent is used in tests and is stopped after the test is done.

When calling its `stop` function, it will wait for active connections (sockets)
to close before considering itself stopped. This can add an unnecessary waiting
period after each test, as the timeout on the socket by default is two seconds
in our code-base:

    https://github.com/DataDog/dd-trace-js/blob/90fe64b9845d04d6273ec2b7a36ba6a7844b8253/packages/dd-trace/src/exporters/common/request.js#L56

For integration tests making heavy use of fake agents, this can add several
minutes of overhead that can be avoided by destroying the sockets when calling
the `stop` function on the fake agent.

This mainly affects tests running on Node.js 18, as sockets there for some
reason takes a little longer to clear out.
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.

2 participants