Skip to content
This repository was archived by the owner on Aug 4, 2023. It is now read-only.

Conversation

@trentm
Copy link
Member

@trentm trentm commented Mar 7, 2022

Use the 'agentkeepalive' alternative to Node.js core http.Agent
for keep-alive handling to get the freeSocketTimeout option that
can timeout kept-alive (aka "free") sockets before getting in the
range of the downstream APM Server (or Lambda extension) HTTP
Keep-Alive timeout. This avoids occasional ECONNRESET errors.

Refs: elastic/apm-agent-nodejs#2594

Checklist

Use the 'agentkeepalive' alternative to Node.js core `http.Agent`
for keep-alive handling to get the `freeSocketTimeout` option that
can timeout kept-alive (aka "free") sockets before getting in the
range of the downstream APM Server (or Lambda extension) HTTP
Keep-Alive timeout. This avoids occasional ECONNRESET errors.

Fixes: #2594
@trentm trentm requested a review from astorm March 7, 2022 22:04
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Mar 7, 2022
@trentm trentm self-assigned this Mar 7, 2022
@trentm trentm removed the request for review from astorm March 7, 2022 22:20
…req socket. It doesn't hurt to reset it for later node versions
@ghost
Copy link

ghost commented Mar 7, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-03-09T20:19:48.481+0000

  • Duration: 8 min 4 sec

Test stats 🧪

Test Results
Failed 0
Passed 287
Skipped 0
Total 287

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@trentm trentm requested a review from astorm March 8, 2022 00:10
Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

Swapping in a new http.Agent seems like a pretty big change, but if we are going to do it agentkeepalive looks like a reasonable choice. It's a long standing project with a maintainer that's still semi-engaged. And if it fixes some lambda problems then I guess we're in "trust our tests and yolo be agile" territory.

The specific code changes themselves appear to do what they intend to.

I see that we're ensuring keepAliveMsecs, maxSockets, maxFreeSockets, and freeSocketTimeout are set on our configuration object.

I see we're conditionally choosing which keep alive constructor-function/class to use (the HTTP or HTTPS version) based on the protocol portion of the server URL.

I see we're then instantiating an object with that constructor function/class and including two new configuration fields -- freeSocketTimeout and serverTimeout.

Approving.

const end = Date.now()
const delta = end - start
t.ok(delta > 1000 && delta < 2000, 'timeout should occur between 1-2 seconds')
t.ok(delta > 1000 && delta < 2000, `timeout should occur between 1-2 seconds: delta=${delta}ms`)
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for some better test logging.

@trentm trentm merged commit 0073c43 into main Mar 9, 2022
@trentm trentm deleted the trentm/issue2594-keep-alive-ECONNRESET-bug branch March 9, 2022 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

agent-nodejs Make available for APM Agents project planning.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants