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

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,7 @@
) {
const route = original.apply(this, args);
const layer = this._router.stack[this._router.stack.length - 1];
instrumentation._applyPatch.call(
instrumentation,
layer,
getLayerPath(args)
);
instrumentation._applyPatch(layer, getLayerPath(args));
return route;
};
};
Expand All @@ -176,7 +172,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 +310,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, {
blumamir marked this conversation as resolved.
Show resolved Hide resolved
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JamieDanielson

Thanks for your review :)

...makes me wonder if there's a better way to do this.

For "better" you meant at the type level or you think the usage of Object.defineProperty is not a good solution? I'll put my thoughts on both.

Type error
The type error we get is:

Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'Function'.
  No index signature with a parameter of type 'string' was found on type 'Function'.ts(7053)

The internal typings define the handle property of the layer as a Function. I've also checked @types/express to see if we can get the type from there but the router stack (where the layers are stored) is defines as any[]. So at the time of writing it adding comment to highlight express is "leveraging" the Object nature of functions to hide metadata felt the right thing to do. Note: the type error also arises with @blumamir's proposal since its a type level error.

We may get rid of the error by doing some improvements in the internal types so the compiler will know the function being patched is also used as a record. Something like the type below should work.

type HandlerType = Function & Record <string,any>

Object.defineProperty vs copy

Doing the shallow copy proposed by @blumamir fixes this issue as well but those properties represents the state of the layer. If that state is modified for the handler to change it's behaviour the original one will not notice that change and keep behaving according the original state (before the copy). With defineProperty we keep patched and original in sync (sharing state) so the instrumentation will not interfere with any code accessing or changing the state.

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, that sounds like a good idea to use that new HandlerType. Would you be open to adding that into internal-types, and using that instead of ignoring the TypeScript error?

Also thank you for the detailed explanation, that helps me understand much better!

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, that sounds like a good idea to use that new HandlerType. Would you be open to adding that into internal-types, and using that instead of ignoring the TypeScript error?

Also thank you for the detailed explanation, that helps me understand much better!

I support doing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the types :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @JamieDanielson :)

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

Check warning on line 324 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#L324

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

Check warning on line 326 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#L326

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

return patched;
});
}

Expand Down
Original file line number Diff line number Diff line change
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