Dd-trace-js: Use request pooling in node

[stephenh]

This is minor, but I think currently the writer is creating a new http connection/DNS lookup/etc. on each flush. We're seeing a few docker.net.tcpx.active_opens in our Node process, which is usually indicative of new connections being opened. The overhead is small but we try to have everything use connection pooling as a general best practice.

In node, the default behavior for http.request is to close the connection if there is not a currently-pending request lined up ready to go. For a process like dd-trace-js that batches and so periodically flushes a single writer, my understanding is that this means each flush is essentially a new connection.

Using a kept-alive connection is pretty easy:

const http = require('http');
const keepAliveAgent = new http.Agent({ keepAlive: true });
options.agent = keepAliveAgent;
http.request(options, onResponseCallback); 

From https://nodejs.org/api/http.html.

[rochdev]

I wonder if this should be configurable. Is there any case where you wouldn't want to use connection pooling?

[stephenh]

Good question; I believe in this case it should always be a good thing; dd-trace-js is only talking to a single host, and a single connection at a time, so its not like the pool would get really huge/take a lot of resources, which I think is usually the concern with pools (keeping around unneeded resources). And dd-trace-js knows it will reuse the connection really soon now, e.g. it won't go ~hours without using it.

For our client libraries, the destination is also effectively static (the ddagent on the box); for others even if the destination host/DNS/IP changes (e.g. they're writing directly to the DD API or something), the connection should get closed/reopened transparently.

[rochdev]

The tracer will only have one persistent connection, but the agent will get one for every tracer. This could increase the memory usage of the agent if there are enough services sending traces.

I'd have to investigate a bit more to get answers to the following 2 questions before making this the default:

• Do we use keep-alive in our other tracers?
• What would the impact be on the agent?

In the meantime, if you really think it would be beneficial I could add an option on the tracer. I would be very interested in seeing numbers however as I'm skeptical of the actual benefit.

[stephenh]

Yeah, that's true the agent would have some kept open. You're skepticism is warranted, as agreed any benefit would be minimal since its all in-data-center, not-SSL, etc. It is mostly just a knee-jerk "persistent connections are a good thing". (It also came up because an engineer here recommended resolving the statsd -> IP once on process boot, when we pass the URL into the dd config libraries, so that the domain name doesn't have to be re-resolved up on each flush, and my was musing, well, if the the connection was kept alive, then it also won't have to be re-resolved. Again disclaimer that optimizing out DNS lookups and non-SSL connection startups is minor.)

[rochdev]

I did some digging on our end and we are already evaluating whether to use connection pooling for our tracers. I prefer to keep the current behavior as the default for now until we have a better understanding of the impact of moving towards this model.

I'll add an option in the meantime so that this can at least be configured.

[millerick]

Has this been implemented? Either a way to provide an agent to use, or built in connection pooling?

[rochdev]

@millerick It hasn't been implemented yet, but it's something still on our radar. What is your specific use case?

[millerick]

The same knee-jerk reaction that persistent connections are a good thing. It looks like we are using an IP to connect to the dd-agent, so we aren't paying the cost of a DNS lookup with each flush, but I don't think it would hurt to maintain connections.

[rochdev]

@millerick Fair enough. Do you think there is value to be able to pass your own HTTP agent, or would an option be enough?

[millerick]

An option is more than fine.

[tekmaven]

@rochdev I would like surface this request again and add another customer's voice to the request.

We see excessive new connections without keep-alive turned on, leading a lot of DNS requests for each new connection, which puts a lot of strain on DNS servers.

It would be a lot of value to passing a custom HTTP agent. Many other libraries have similar options. For example:

• AWS JS SDK - https://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/node-reusing-connections.html
• Stripe Node SDK - https://github.com/stripe/stripe-node#configuration

[rochdev]

@tekmaven Just to make sure there isn't also another issue at play, how many new connections do you consider excessive? In most cases, the agent is called only once every 2 second by the tracer, so just want to make sure there isn't some other issue causing this to increase.

[tekmaven]

Thanks for getting back to me Roch! At a larger scale, the numbers do get pretty large. We have over 1000 containers of a single app sending data to the agent. In the past hour that app has resolved the hostname of the agent over _200k times_ in the dns.lookup operation. We've seen up to a 98% reduction in DNS lookups and TCP connections with this simple change.

[rochdev]

We plan to address this soon, by providing options for DNS caching as well as HTTP keep-alive. In general I recommend to cache at the OS level, but I understand this is not always possible.