Fix: router and http2

[shahmirn]

What does this PR do?

We recently experimented with switching our express server to http2, and noticed that we weren't seeing a breakdown of the express requests under APM > Services anymore.

Dug into the code, and it looks like it's simply because the router plugin wasn't listening for the http2 server's requests finishing?

This PR should hopefully fix that issue?

Motivation

Plugin Checklist

• Unit tests.
• TypeScript definitions.
• TypeScript tests.
• API documentation.
• CircleCI jobs/workflows.
• Plugin is exported.

Additional Notes

[Qard]

Good catch. Do you think you could add a test to confirm this solves the issue?

[shahmirn]

Good catch. Do you think you could add a test to confirm this solves the issue?

Done. Updated PR to add tests.

Took quite bit of time to figure out that the http2 plugin wasn't getting instrumented, and then another incorrect rabbit hole of creating an http2 agent before realizing that wasn't needed.

It should all be working now though.

[rochdev]

Can both tests be merged with just a parameter of which HTTP module to use? Otherwise this is a lot of duplication.

[Qard]

Yeah, should be able to do something like:

const tests = [
{ name: 'http', createServer () { /* ... / } },
{ name: 'http2', createServer () { / ... */ } },
]

for (const { name, createServer } of tests) {
describe(name, () => {
// use createServer wherever needed, and add the tests...
})
}

[shahmirn]

I don't think this is a good idea.

The actual implementation of the tests are slightly different, so there'd end up being if statements in 'em anyways.

For http2, fetch-h2 is being used as the client, as axios doesn't support http2. And the URLs being passed to fetch-h2 start with http2://, so that it knows we're dealing with a http2 clearText request

so even though the test names are the same, and the expectations are similar, how the data is fetched is different.

[rochdev]

Why not just use an HTTP client that supports both? Then this becomes a non-issue and both tests can be merged.

[tlhunter]

@shahmirn is this PR something you're still interested in working on? If not I can close it for now.

[shahmirn]

@tlhunter Nope; closing it out.