Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(instr-express): keep hidden properties in layer handlers #2137

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Expand Up @@ -152,8 +152,7 @@
) {
const route = original.apply(this, args);
const layer = this._router.stack[this._router.stack.length - 1];
instrumentation._applyPatch.call(
instrumentation,
instrumentation._applyPatch(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: this usage of Function.protptype.call comes from this commit 0723d06 as a new addition.

I did not dig further to get the original implementation but I think there is no need to use .call(...) since we have access to the container object and therefore calling _applyPatch with the right context.

layer,
getLayerPath(args)
);
Expand All @@ -176,7 +175,8 @@
this._wrap(layer, 'handle', (original: Function) => {
// TODO: instrument error handlers
if (original.length === 4) return original;
return function (

const patched = function (
this: ExpressLayer,
req: PatchedRequest,
res: express.Response
Expand Down Expand Up @@ -313,6 +313,25 @@
}
}
};

// `handle` isn't just a regular function in some cases. It also contains
// some properties holding metadata and state so we need to proxy them
// through through patched function
// ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1950
Object.keys(original).forEach(key => {
Object.defineProperty(patched, key, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why define a property with getter and setter.

Will this work as well?

Object.keys(original).forEach(key => patched[key] = original[key])

Or

Object.entries(original).forEach([key, value] => patched[key] = value)

get() {
// @ts-expect-error -- the original function has hidden props indexed by strings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the purpose of what we're trying to achieve, the error that comes up here feels very relevant and makes me wonder if there's a better way to do this.

return original[key];
},
set(value) {

Check warning on line 327 in plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L327 was not covered by tests
// @ts-expect-error -- the original function has hidden props indexed by strings
original[key] = value;

Check warning on line 329 in plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L329 was not covered by tests
},
});
});

return patched;
});
}

Expand Down
Expand Up @@ -488,6 +488,35 @@ describe('ExpressInstrumentation', () => {
}
);
});

it('should keep stack in the router layer handle', async () => {
const rootSpan = tracer.startSpan('rootSpan');
let routerLayer: { name: string; handle: { stack: any[] } };
const httpServer = await serverWithMiddleware(tracer, rootSpan, app => {
app.use(express.json());
app.get('/bare_route', (req, res) => {
const stack = req.app._router.stack as any[];
routerLayer = stack.find(layer => layer.name === 'router');
return res.status(200).end('test');
});
});
server = httpServer.server;
port = httpServer.port;
await context.with(
trace.setSpan(context.active(), rootSpan),
async () => {
const response = await httpRequest.get(
`http://localhost:${port}/bare_route`
);
assert.strictEqual(response, 'test');
rootSpan.end();
assert.ok(
routerLayer?.handle?.stack?.length === 1,
'router layer stack is accessible'
);
}
);
});
});

describe('Disabling plugin', () => {
Expand Down Expand Up @@ -527,6 +556,7 @@ describe('ExpressInstrumentation', () => {
);
});
});

it('should work with ESM usage', async () => {
await testUtils.runTestFixture({
cwd: __dirname,
Expand Down