[http] Add connection establishment tracing.

[alloy]

This is helpful in order to know how much time you're spending on creating connections and determine if you should use keep-alive.

I also slightly refactored the tests to always sort spans before yielding them to the tests. There didn’t appear to be any test that cared about the unordered spans, but an option to not sort should be easy enough to add in the future, if that’s ever needed.

[alloy]

Didn’t see a v0.7.0 branch yet, so targeting master for now.

[alloy]

@rochdev Seems like on Node v4 & v6 the agent.use(…) callback is invoked twice, once with 3 spans and once with the last 1.

I see that in the past you had added a way to tell the agent how many traces to expect before invoking the callback, but subsequently removed it again. (Although you forgot to remove the counts, e.g. here.) However, that wouldn’t have worked here either, because I’m really not expecting a specific number of traces to be flushed at once, so in 11c6491 I made it possible to tell the agent to wait a bit before considering work done. Do you have a better idea?

[alloy]

I should add that the current implementation on master seems to (theoretically) leak some async work between tests, because the promise returned by agent.use(…) can resolve before all traces have arrived.

[rochdev]

I see that in the past you had added a way to tell the agent how many traces to expect before invoking the callback, but subsequently removed it again.

The idea is that this was very prone to errors since you had to know exactly how many traces to expect, which in some cases didn't work well. It was also very heavy since we had to clean the entire cache and reset the agent after every single test. The idea by removing it is that it will keep trying if it receives an invalid trace, meaning that leaks are simply ignored. This is similar to how Nock works. At some point I'd like to be able to completely remove the need for cleaning the cache.

I should add that the current implementation on master seems to (theoretically) leak some async work between tests, because the promise returned by agent.use(…) can resolve before all traces have arrived.

As mentioned above, this is by design and any previously leaked trace should not impact the current test.

I made it possible to tell the agent to wait a bit before considering work done. Do you have a better idea?

I think supporting multiple calls to .use() would be the best approach. For example, if you expect the agent to be called 2 times, you would call .use() twice with the 2 expectations. It could also simply accept multiple arguments with the same effect. Then the agent should wait for these 2 expectations to pass before resolving.

What do you think?

[alloy]

I think supporting multiple calls to .use() would be the best approach. For example, if you expect the agent to be called 2 times, you would call .use() twice with the 2 expectations. It could also simply accept multiple arguments with the same effect. Then the agent should wait for these 2 expectations to pass before resolving.

So that’s the thing, I don’t really expect it to be called any number of times. That it is invoked twice on Node v4 and v6 seems to me (naively) mostly an implementation detail that I don’t really care about in this test. While I do know it will take 2 times, it would mean I’d have to add something like agent.use(…, process.version < "8" ? 2 : 1) which also doesn’t feel great.

Maybe if you feel strongly about specifying specific numbers, do you have any insights into why this is being broken up into 2 invocations on these older Node versions? That way I can maybe better reason about the solution.

[alloy]

I should add that the current implementation on master seems to (theoretically) leak some async work between tests, because the promise returned by agent.use(…) can resolve before all traces have arrived.

As mentioned above, this is by design and any previously leaked trace should not impact the current test.

Got it 👍

[rochdev]

If the socket never connects and is reused for other requests, won't this created a leak?

[rochdev]

Superseded by #406

[alloy]

Ace, thanks!