Skip to content

Commit ce5f48d

Browse files
fix(instr-express): keep hidden properties in layer handlers (open-telemetry#2137)
* fix(instr-express): keep hidden properties in router layer handle function * chore(instr-express): re-enable ESM test * chore(instr-express): fix lint issues * chore(instr-express): fix lint issues * chore(instr-expres): update internal types * chore(instr-express): fix lint issues --------- Co-authored-by: Marc Pichler <[email protected]>
1 parent 93776fa commit ce5f48d

File tree

3 files changed

+52
-8
lines changed

3 files changed

+52
-8
lines changed

plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,7 @@ export class ExpressInstrumentation extends InstrumentationBase {
152152
) {
153153
const route = original.apply(this, args);
154154
const layer = this._router.stack[this._router.stack.length - 1];
155-
instrumentation._applyPatch.call(
156-
instrumentation,
157-
layer,
158-
getLayerPath(args)
159-
);
155+
instrumentation._applyPatch(layer, getLayerPath(args));
160156
return route;
161157
};
162158
};
@@ -173,10 +169,11 @@ export class ExpressInstrumentation extends InstrumentationBase {
173169
if (layer[kLayerPatched] === true) return;
174170
layer[kLayerPatched] = true;
175171

176-
this._wrap(layer, 'handle', (original: Function) => {
172+
this._wrap(layer, 'handle', original => {
177173
// TODO: instrument error handlers
178174
if (original.length === 4) return original;
179-
return function (
175+
176+
const patched = function (
180177
this: ExpressLayer,
181178
req: PatchedRequest,
182179
res: express.Response
@@ -313,6 +310,23 @@ export class ExpressInstrumentation extends InstrumentationBase {
313310
}
314311
}
315312
};
313+
314+
// `handle` isn't just a regular function in some cases. It also contains
315+
// some properties holding metadata and state so we need to proxy them
316+
// through through patched function
317+
// ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1950
318+
Object.keys(original).forEach(key => {
319+
Object.defineProperty(patched, key, {
320+
get() {
321+
return original[key];
322+
},
323+
set(value) {
324+
original[key] = value;
325+
},
326+
});
327+
});
328+
329+
return patched;
316330
});
317331
}
318332

plugins/node/opentelemetry-instrumentation-express/src/internal-types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ export type ExpressRouter = {
5858

5959
// https://github.com/expressjs/express/blob/main/lib/router/layer.js#L33
6060
export type ExpressLayer = {
61-
handle: Function;
61+
handle: Function & Record<string, any>;
6262
[kLayerPatched]?: boolean;
6363
name: string;
6464
params: { [key: string]: string };

plugins/node/opentelemetry-instrumentation-express/test/express.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,35 @@ describe('ExpressInstrumentation', () => {
488488
}
489489
);
490490
});
491+
492+
it('should keep stack in the router layer handle', async () => {
493+
const rootSpan = tracer.startSpan('rootSpan');
494+
let routerLayer: { name: string; handle: { stack: any[] } };
495+
const httpServer = await serverWithMiddleware(tracer, rootSpan, app => {
496+
app.use(express.json());
497+
app.get('/bare_route', (req, res) => {
498+
const stack = req.app._router.stack as any[];
499+
routerLayer = stack.find(layer => layer.name === 'router');
500+
return res.status(200).end('test');
501+
});
502+
});
503+
server = httpServer.server;
504+
port = httpServer.port;
505+
await context.with(
506+
trace.setSpan(context.active(), rootSpan),
507+
async () => {
508+
const response = await httpRequest.get(
509+
`http://localhost:${port}/bare_route`
510+
);
511+
assert.strictEqual(response, 'test');
512+
rootSpan.end();
513+
assert.ok(
514+
routerLayer?.handle?.stack?.length === 1,
515+
'router layer stack is accessible'
516+
);
517+
}
518+
);
519+
});
491520
});
492521

493522
describe('Disabling plugin', () => {
@@ -527,6 +556,7 @@ describe('ExpressInstrumentation', () => {
527556
);
528557
});
529558
});
559+
530560
it('should work with ESM usage', async () => {
531561
await testUtils.runTestFixture({
532562
cwd: __dirname,

0 commit comments

Comments
 (0)