dd-trace-js

dd-trace-js copied to clipboard

What does this PR do?

Uses undici instead of the regular node http API.

Before:

$ yarn bench:e2e -- --duration=300
avg latency overhead % 52.27722772277228
avg requests overhead % 47.5775574317691
avg throughput overhead % 47.564089591711856

After:

$ yarn bench:e2e -- --duration=300
avg latency overhead % 39.0928725701944
avg requests overhead % 35.334456624993926
avg throughput overhead % 35.315102625776994

Results may vary.

Motivation

Better performance.

Can you provide more details as to why undici is so much faster? Is there something we could do internally that would be even faster without adding a library? This is especially true since it doesn't support Node 8 which we currently still support.

Can you provide more details as to why undici is so much faster?

It skips a lot of the work involved in implementing node's http API. It reduces the usage of streams on the JavaScript side, unless you need them (we don't). It reduces object allocations. It also performs some lower-level tricks like corking and uncorking at various times to optimize packet sizes.

Is there something we could do internally that would be even faster without adding a library? This is especially true since it doesn't support Node 8 which we currently still support.

Maybe? A lot of the work is already done and tested by undici though, and since HTTP can get pretty complex, I'd rather note re-implement large parts of it, test those parts, and then hope we've covered all the weird edge cases. I don't think re-implementing its wins is a good use of time.

Instead, if we can't drop Node 8 support yet, undici could be an optionalDependency, and if it's not available we can fall back to http/https. How does that sound?

EDIT: To clarify the optionalDependency option, if it's in that section, it won't fail any installs. In our request code, we can check Node.js version numbers to see what's supported, and then act accordingly.

This PR is not up to date with the branch. I had accidentally committed a bunch of garbage files, so I removed them, and while the branch is up to date, the PR is not, so I'd hold off until this is resolved by GitHub. I will contact GitHub support now.

Should this be hidden behind an option at first since it's a major change?

Updated it to only use undici if the node versions supports it and experimental: { undici: true }

Also, I see that the library seems to be using promises in a few places which worries be given all the issues we have with them. Is any promise code path hit by the tracer?

It only seems to use promises in 2 places:

1. In dispatch if you invoke it without callbacks. We're giving it callbacks.
2. In Pool to handle connection lifetimes.

@bengl dearest OP, it looks like some time has passed since this PR was kicked off. Is it something you'd like to drive on home or would it be safer to just close it for now?

I'll close this PR for now but we can consider reopening it in the future.