From ff046a4ee3188f6e01ebe220f2463855cbc0ab1d Mon Sep 17 00:00:00 2001 From: Noemi Date: Tue, 16 Aug 2022 19:15:45 +0200 Subject: [PATCH] fix(koa): ignore generator-based Koa middleware In Koa 2.x, which is the Koa version currently targeted by the instrumentation, generator-based middleware from Koa 0.x and 1.x are deprecated, and a deprecation warning is shown when these middleware are used. However, most of these middleware still work as intended on Koa version 2.x, due to the use of the `koa-convert` compatibility layer, which is applied automatically when a generator-based middleware is used. Because of this, low-quality tutorial websites continue to recommend the usage of generator-based middleware for error handling. 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. After this change, generator-based middleware are not instrumented, and the deprecation warning is shown. Users who wish for their generator-based middleware to be instrumented can manually use `koa-convert`. In Koa 3.x, generator-based middleware will not work, with or without instrumentation. 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. --- .../README.md | 4 +- .../src/instrumentation.ts | 10 ++ .../test/koa.test.ts | 142 +++++++++++++----- 3 files changed, 119 insertions(+), 37 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-koa/README.md b/plugins/node/opentelemetry-instrumentation-koa/README.md index a5c1ad9155..b55339f24f 100644 --- a/plugins/node/opentelemetry-instrumentation-koa/README.md +++ b/plugins/node/opentelemetry-instrumentation-koa/README.md @@ -38,7 +38,9 @@ registerInstrumentations({ }); ``` -See [`examples/koa`](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/examples/koa) for a short example using both Koa and @koa/router +See [`examples/koa`](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/examples/koa) for a short example using both Koa and @koa/router. + +Note that generator-based middleware are deprecated and won't be instrumented. ### Koa Instrumentation Options diff --git a/plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts index ac2028f6c1..c2c2aef969 100644 --- a/plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts @@ -133,7 +133,17 @@ export class KoaInstrumentation extends InstrumentationBase { isLayerIgnored(layerType, this._config) ) return middlewareLayer; + + if ( + middlewareLayer.constructor.name === 'GeneratorFunction' || + middlewareLayer.constructor.name === 'AsyncGeneratorFunction' + ) { + api.diag.debug('ignoring generator-based Koa middleware layer'); + return middlewareLayer; + } + middlewareLayer[kLayerPatched] = true; + api.diag.debug('patching Koa middleware layer'); return async (context: KoaContext, next: koa.Next) => { const parent = api.trace.getSpan(api.context.active()); diff --git a/plugins/node/opentelemetry-instrumentation-koa/test/koa.test.ts b/plugins/node/opentelemetry-instrumentation-koa/test/koa.test.ts index 063985343a..46295f8869 100644 --- a/plugins/node/opentelemetry-instrumentation-koa/test/koa.test.ts +++ b/plugins/node/opentelemetry-instrumentation-koa/test/koa.test.ts @@ -91,34 +91,44 @@ describe('Koa Instrumentation', () => { server.close(); }); - const simpleResponse: koa.Middleware = async (ctx, next) => { - ctx.body = 'test'; - await next(); - }; - - const customMiddleware: koa.Middleware = async (ctx, next) => { - for (let i = 0; i < 1000000; i++) { - continue; - } - await next(); - }; - - const spanCreateMiddleware: koa.Middleware = async (ctx, next) => { - const span = tracer.startSpan('foo'); - span.end(); - await next(); - }; - - const asyncMiddleware: koa.Middleware = async (ctx, next) => { - const start = Date.now(); - await next(); - const ms = Date.now() - start; - ctx.body = `${ctx.method} ${ctx.url} - ${ms}ms`; - }; - - const failingMiddleware: koa.Middleware = async (_ctx, _next) => { - throw new Error('I failed!'); - }; + const simpleResponse: () => koa.Middleware = () => + async function simpleResponse(ctx, next) { + ctx.body = 'test'; + await next(); + }; + + const customMiddleware: () => koa.Middleware = () => + async function customMiddleware(ctx, next) { + for (let i = 0; i < 1000000; i++) { + continue; + } + await next(); + }; + + const spanCreateMiddleware: () => koa.Middleware = () => + async function spanCreateMiddleware(ctx, next) { + const span = tracer.startSpan('foo'); + span.end(); + await next(); + }; + + const asyncMiddleware: () => koa.Middleware = () => + async function asyncMiddleware(ctx, next) { + const start = Date.now(); + await next(); + const ms = Date.now() - start; + ctx.body = `${ctx.method} ${ctx.url} - ${ms}ms`; + }; + + const failingMiddleware: () => koa.Middleware = () => + async function failingMiddleware(_ctx, _next) { + throw new Error('I failed!'); + }; + + const generatorMiddleware: () => koa.Middleware = () => + function* generatorMiddleware(next) { + yield next; + }; describe('Instrumenting @koa/router calls', () => { it('should create a child span for middlewares', async () => { @@ -330,9 +340,9 @@ describe('Koa Instrumentation', () => { app.use((ctx, next) => context.with(trace.setSpan(context.active(), rootSpan), next) ); - app.use(customMiddleware); - app.use(simpleResponse); - app.use(spanCreateMiddleware); + app.use(customMiddleware()); + app.use(simpleResponse()); + app.use(spanCreateMiddleware()); await context.with( trace.setSpan(context.active(), rootSpan), @@ -383,8 +393,8 @@ describe('Koa Instrumentation', () => { }); it('should not create span if there is no parent span', async () => { - app.use(customMiddleware); - app.use(simpleResponse); + app.use(customMiddleware()); + app.use(simpleResponse()); const res = await httpRequest.get(`http://localhost:${port}`); assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); @@ -396,7 +406,7 @@ describe('Koa Instrumentation', () => { app.use((ctx, next) => context.with(trace.setSpan(context.active(), rootSpan), next) ); - app.use(asyncMiddleware); + app.use(asyncMiddleware()); await context.with( trace.setSpan(context.active(), rootSpan), @@ -422,12 +432,72 @@ describe('Koa Instrumentation', () => { ); }); + it('should not instrument generator middleware functions', async () => { + const rootSpan = tracer.startSpan('rootSpan'); + app.use((_ctx, next) => + context.with(trace.setSpan(context.active(), rootSpan), next) + ); + + app.use(generatorMiddleware()); + app.use(simpleResponse()); + + await context.with( + trace.setSpan(context.active(), rootSpan), + async () => { + await httpRequest.get(`http://localhost:${port}`); + rootSpan.end(); + assert.deepStrictEqual(memoryExporter.getFinishedSpans().length, 2); + + const simpleResponseSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name.includes('simpleResponse')); + assert.notStrictEqual(simpleResponseSpan, undefined); + + const exportedRootSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name === 'rootSpan'); + assert.notStrictEqual(exportedRootSpan, undefined); + } + ); + }); + + it('should not instrument the same middleware more than once', async () => { + const sameAsyncMiddleware = asyncMiddleware(); + + const rootSpan = tracer.startSpan('rootSpan'); + app.use((_ctx, next) => + context.with(trace.setSpan(context.active(), rootSpan), next) + ); + + app.use(sameAsyncMiddleware); + app.use(sameAsyncMiddleware); + + await context.with( + trace.setSpan(context.active(), rootSpan), + async () => { + await httpRequest.get(`http://localhost:${port}`); + rootSpan.end(); + assert.deepStrictEqual(memoryExporter.getFinishedSpans().length, 2); + + const asyncMiddlewareSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name.includes('asyncMiddleware')); + assert.notStrictEqual(asyncMiddlewareSpan, undefined); + + const exportedRootSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name === 'rootSpan'); + assert.notStrictEqual(exportedRootSpan, undefined); + } + ); + }); + it('should propagate exceptions in the middleware while marking the span with an exception', async () => { const rootSpan = tracer.startSpan('rootSpan'); app.use((_ctx, next) => context.with(trace.setSpan(context.active(), rootSpan), next) ); - app.use(failingMiddleware); + app.use(failingMiddleware()); const res = await httpRequest.get(`http://localhost:${port}`); assert.deepStrictEqual(res, 'Internal Server Error'); @@ -463,7 +533,7 @@ describe('Koa Instrumentation', () => { it('should not create new spans', async () => { plugin.disable(); const rootSpan = tracer.startSpan('rootSpan'); - app.use(customMiddleware); + app.use(customMiddleware()); await context.with( trace.setSpan(context.active(), rootSpan),