fix(koa): ignore generator-based Koa middleware

[unflxw]

Which problem is this PR solving?

• Before this change, using a generator-based middleware alongside the OpenTelemetry instrumentation would break the request, as the instrumentation does not expect a generator-based middleware.
• Furthermore, the replacement of the middleware with its async-based patch would silence the deprecation warning about the generator-based middleware.

Short description of the changes

• After this change, generator-based middleware are not instrumented, the request is not broken, and the deprecation warning is shown. Users who wish for their generator-based middleware to be instrumented can manually use koa-convert.

Checklist

• Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

[codecov]

Codecov Report

Merging #1119 (7f66e81) into main (b406b1d) will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1119      +/-   ##
==========================================
+ Coverage   96.07%   96.38%   +0.30%     
==========================================
  Files          14       19       +5     
  Lines         892     1051     +159     
  Branches      191      223      +32     
==========================================
+ Hits          857     1013     +156     
- Misses         35       38       +3     

Impacted Files	Coverage Δ
...lemetry-instrumentation-koa/src/instrumentation.ts	97.67% <100.00%> (ø)
...ry-instrumentation-koa/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts	98.00% <0.00%> (ø)
...ode/opentelemetry-instrumentation-koa/src/utils.ts	100.00% <0.00%> (ø)
...ode/opentelemetry-instrumentation-koa/src/types.ts	100.00% <0.00%> (ø)

[blumamir]

Can you please add a unit-test for this?

[dyladan]

The generator detection is not perfect but it is actually not possible to have perfect detection based on this https://stackoverflow.com/a/19660350/1272149

Might be worth a note in the README but otherwise LGTM

[unflxw]

@dyladan While the detection logic is indeed not perfect, it's worth noting that it's the same detection logic used by the koa-convert library, which is in turn used by the Koa 2.x internals to automatically convert legacy generator-based middleware to async middleware.

Meaning that in Koa 2.x, which is the Koa version we support, the only generator middleware who would ever work (with or without the OpenTelemetry instrumentation) are those that are GeneratorFunction or AsyncGeneratorFunction, as those are the only ones that will be converted.

It is true that, in order to decide whether to call koa-convert and show the deprecation warning, Koa itself uses is-generator-function, which has some more convoluted logic for figuring out whether something is a generator function, and does not check for AsyncGeneratorFunctions. Meaning that an AsyncGeneratorFunction will not show the warning, will not be converted, and will not work, with or without the instrumentation. If you think using is-generator-function here (and pulling the extra dependency) as well is preferable, let me know.

[dyladan]

Thanks for the thorough explanation. I think the logic is fine the way it is in this case

[unflxw]

EDIT: I have pushed a commit that assumes this behaviour is not a bug and fixes the tests accordingly:

Due to the logic in the Koa instrumentation to avoid re-patching middlewares, middlewares are only instrumented the first time they are used. This commit fixes false positives in the test suite due to middleware reuse between tests, and adds a test for the behavior described.

It does still seem to me like a bug, though? 🤷

@blumamir When adding a unit test that re-uses the existing test middleware, I noticed what I believe is a bug in the logic to avoid re-patching a middleware. I'm not sure, because I'm not sure what the desired behaviour of kLayerPatched is. This is not a blocker for this PR, as I will rewrite the new unit test to avoid this issue, I just wanted to make this issue known.

When a middleware is marked as patched, we mark it with kLayerPatched, to avoid patching it again if it is used more than once. However, when the same middleware is encountered again, since we already marked it as patched, we return the un-patched middleware, not the same patched middleware. What this means is that a middleware is only instrumented the first time it's given to app.use, and future uses are not re-patched, but they're not instrumented at all either. Consider the following (slightly contrived) example:

const commonMiddleware = (_ctx, next) => next()

const firstApp = new koa()
firstApp.use(commonMiddleware)
firstApp.use(commonMiddleware)
// ☝️ this won't be re-patched, but it also won't be instrumented at all!

const secondApp = new koa()
secondApp.use(commonMiddleware)
// ☝️ this won't be re-patched or instrumented either!

The second app will not have its middleware instrumented, because the middleware was already marked as patched the first time, and marking it as patched does not mean we return the previously patched version, but the middleware as-is.

So, contrived example aside, where does this happen in real life? Well, it happens in our test suite! Although the functionality it tests does actually work, the "should not create span if there is no parent span" test is currently a false positive of sorts, in the sense that the test passes for the wrong reasons: because it comes after other tests, and it reuses middleware that have already been used by the previous tests, the middleware that are passed to it are marked as patched, so the test uses the un-patched middleware, meaning it's not actually testing the instrumentation at all. Unless it's executed in isolation, the test does not fail when adding a parent span.

From this explanation on, choose your own adventure (I'm happy to implement whatever the desired behaviour is):

• This is not a bug, and a middleware that is used several times should only be instrumented on its first usage: in that case, our tests should not reuse middleware. EDIT: 👈 implemented this as part of adding the unit test, because at least it puts the current existing behaviour under test
• This is a bug, and a middleware that is used several times should be instrumented for each use...
  • ... and we don't mind creating several patches for the same middleware: remove all kLayerPatched related logic.
  • ... but we still would like to avoid creating several patches for the same middleware: instead of setting kLayerPatched to true, set it to the patched function. When skipping re-patching a middleware, instead of returning the unpatched middleware, return the patched function stored in kLayerPatched (which might be better renamed as kLayerPatch).

Either way, there should be a test that checks whatever the expected behaviour is when reusing middleware.

[unflxw]

Rebased out README conflict.