Skip to content

feat(instrumentation-express): Add support for Express v5 #2437

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.

Sign up for GitHub

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

Closed
wants to merge 14 commits into from
Closed

feat(instrumentation-express): Add support for Express v5 #2437

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7bf9b3a
Merge remote-tracking branch 'upstream/main'
timfish Sep 16, 2024
35fd061
Move v4 tests
timfish Sep 17, 2024
e0bee03
Get v5 tests passing
timfish Sep 18, 2024
f9c4b72
Fix tests
timfish Sep 18, 2024
b84ad64
Split tests and merge coverage
timfish Sep 18, 2024
39c4dcc
Merge remote-tracking branch 'upstream/main' into feat/support-expres…
timfish Sep 18, 2024
28a9fd0
Add new merged tests to v5 and fix paths
timfish Sep 18, 2024
4488637
Skip express v5 tests for Node v14
timfish Sep 18, 2024
c209205
Skip node v14
timfish Sep 18, 2024
3aa1c9e
Fix regex tests
timfish Sep 18, 2024
3524745
Merge branch 'main' into feat/support-express-v5
david-luna Oct 3, 2024
94f564c
Use different `InstrumentationNodeModuleDefinition` for each version
timfish Oct 23, 2024
9587d3e
Merge branch 'feat/support-express-v5' of https://github.com/timfish/…
timfish Oct 23, 2024
4a71e9b
Merge remote-tracking branch 'upstream/main' into feat/support-expres…
timfish Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion plugins/node/opentelemetry-instrumentation-express/package.json
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

"@types/express": "4.17.21"

There is now a v5 for the typing too
https://www.npmjs.com/package/@types/express

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@
"repository": "open-telemetry/opentelemetry-js-contrib",
"scripts": {
"test-all-versions": "tav",
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'",
"pretest": "cd test/v5 && npm install",
"test": "npm run test:common && npm run test:v4 && npm run test:v5",
"test:common": "nyc --silent ts-mocha -p tsconfig.json 'test/*.test.ts'",
"test:v4": "nyc --silent --no-clean ts-mocha -p tsconfig.json 'test/v4/*.test.ts'",
"test:v5": "nyc --silent --no-clean ts-mocha -p tsconfig.json 'test/v5/*.test.ts'",
"posttest": "nyc report --reporter=lcov --reporter=text",
"tdd": "yarn test -- --watch-extensions ts --watch",
"clean": "rimraf build/*",
"lint": "eslint . --ext .ts",
Expand Down
28 changes: 20 additions & 8 deletions plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,12 @@ export class ExpressInstrumentation extends InstrumentationBase<ExpressInstrumen
return [
new InstrumentationNodeModuleDefinition(
'express',
['>=4.0.0 <5'],
['>=4.0.0 <6'],
moduleExports => {
const routerProto = moduleExports.Router as unknown as express.Router;
const isExpressV5 = 'Route' in moduleExports.Router;
Copy link

Choose a reason for hiding this comment

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

I am not sure I like this kind of check. I have been thinking about this problem as well because another package I maintain had compatibility for the v5 router but we broke that in the final push to release. I don't particularly like the idea of importing the package.json at startup in express, but we dont limit you from doing it. I am not familiar with your testing setup so I don't know how feasible that is for you to do. My main concern with this is we could decide to add a Route constructor back (not that I know of any plans to do so) which would break 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.

Yeah agreed. I suspect it might be a better idea to target the two different versions separately 🤔

const routerProto = isExpressV5
? moduleExports.Router.prototype
: moduleExports.Router;
// patch express.Router.route
if (isWrapped(routerProto.route)) {
this._unwrap(routerProto, 'route');
Expand All @@ -81,13 +84,16 @@ export class ExpressInstrumentation extends InstrumentationBase<ExpressInstrumen
moduleExports.application,
'use',
// eslint-disable-next-line @typescript-eslint/no-explicit-any
this._getAppUsePatch() as any
this._getAppUsePatch(isExpressV5) as any
);
return moduleExports;
},
moduleExports => {
if (moduleExports === undefined) return;
const routerProto = moduleExports.Router as unknown as express.Router;
const isExpressV5 = 'Route' in moduleExports.Router;
const routerProto = isExpressV5
? moduleExports.Router.prototype
: moduleExports.Router;
this._unwrap(routerProto, 'route');
this._unwrap(routerProto, 'use');
this._unwrap(moduleExports.application, 'use');
Expand Down Expand Up @@ -135,16 +141,22 @@ export class ExpressInstrumentation extends InstrumentationBase<ExpressInstrumen
/**
* Get the patch for Application.use function
*/
private _getAppUsePatch() {
private _getAppUsePatch(isExpressV5: boolean) {
const instrumentation = this;
return function (original: express.Application['use']) {
return function use(
this: { _router: ExpressRouter },
// In express 5.x the router is stored in `router` whereas in 4.x it's stored in `_router`
this: { _router?: ExpressRouter; router?: ExpressRouter },
...args: Parameters<typeof original>
) {
// if we access app.router in express 4.x we trigger an assertion error
// This property existed in v3, was removed in v4 and then re-added in v5
const router = isExpressV5 ? this.router : this._router;
const route = original.apply(this, args);
const layer = this._router.stack[this._router.stack.length - 1];
instrumentation._applyPatch(layer, getLayerPath(args));
if (router) {
const layer = router.stack[router.stack.length - 1];
instrumentation._applyPatch(layer, getLayerPath(args));
}
return route;
};
};
Expand Down
13 changes: 0 additions & 13 deletions plugins/node/opentelemetry-instrumentation-express/src/internal-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

import type { Request } from 'express';
import { Attributes } from '@opentelemetry/api';

/**
* This symbol is used to mark express layer as being already instrumented
Expand Down Expand Up @@ -48,11 +47,6 @@ export type PathParams = string | RegExp | Array<string | RegExp>;

// https://github.com/expressjs/express/blob/main/lib/router/index.js#L53
export type ExpressRouter = {
params: { [key: string]: string };
_params: string[];
caseSensitive: boolean;
mergeParams: boolean;
strict: boolean;
stack: ExpressLayer[];
};

Expand All @@ -61,13 +55,6 @@ export type ExpressLayer = {
handle: Function & Record<string, any>;
[kLayerPatched]?: boolean;
name: string;
params: { [key: string]: string };
path: string;
regexp: RegExp;
route?: ExpressLayer;
};

export type LayerMetadata = {
attributes: Attributes;
name: string;
};
2 changes: 1 addition & 1 deletion plugins/node/opentelemetry-instrumentation-express/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export const getLayerMetadata = (
},
name: `router - ${extractedRouterPath}`,
};
} else if (layer.name === 'bound dispatch') {
} else if (layer.name === 'bound dispatch' || layer.name === 'handle') {
return {
attributes: {
[AttributeNames.EXPRESS_NAME]:
Expand Down
9 changes: 6 additions & 3 deletions ...tation-express/test/custom-config.test.ts → ...ion-express/test/v4/custom-config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@ import {
import { SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions';
import * as assert from 'assert';
import { RPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core';
import { ExpressLayerType } from '../src/enums/ExpressLayerType';
import { AttributeNames } from '../src/enums/AttributeNames';
import { ExpressInstrumentation, ExpressInstrumentationConfig } from '../src';
import { ExpressLayerType } from '../../src/enums/ExpressLayerType';
import { AttributeNames } from '../../src/enums/AttributeNames';
import {
ExpressInstrumentation,
ExpressInstrumentationConfig,
} from '../../src';
import { createServer, httpRequest } from './utils';

const instrumentation = new ExpressInstrumentation({
Expand Down
4 changes: 2 additions & 2 deletions ...trumentation-express/test/express.test.ts → ...mentation-express/test/v4/express.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ import {
SimpleSpanProcessor,
} from '@opentelemetry/sdk-trace-base';
import * as assert from 'assert';
import { AttributeNames } from '../src/enums/AttributeNames';
import { ExpressInstrumentation } from '../src';
import { AttributeNames } from '../../src/enums/AttributeNames';
import { ExpressInstrumentation } from '../../src';
import { createServer, httpRequest, serverWithMiddleware } from './utils';
import { SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions';
import * as testUtils from '@opentelemetry/contrib-test-utils';
Expand Down
2 changes: 1 addition & 1 deletion ...st/fixtures/use-express-nested-router.mjs → ...v4/fixtures/use-express-nested-router.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { promisify } from 'util';
import { createTestNodeSdk } from '@opentelemetry/contrib-test-utils';

import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { ExpressInstrumentation } from '../../build/src/index.js';
import { ExpressInstrumentation } from '../../../build/src/index.js';

const sdk = createTestNodeSdk({
serviceName: 'use-express-nested',
Expand Down
2 changes: 1 addition & 1 deletion ...press/test/fixtures/use-express-regex.mjs → ...ss/test/v4/fixtures/use-express-regex.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { promisify } from 'util';
import { createTestNodeSdk } from '@opentelemetry/contrib-test-utils';

import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { ExpressInstrumentation } from '../../build/src/index.js';
import { ExpressInstrumentation } from '../../../build/src/index.js';

const sdk = createTestNodeSdk({
serviceName: 'use-express-regex',
Expand Down
2 changes: 1 addition & 1 deletion ...ress/test/fixtures/use-express-router.mjs → ...s/test/v4/fixtures/use-express-router.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { promisify } from 'util';
import { createTestNodeSdk } from '@opentelemetry/contrib-test-utils';

import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { ExpressInstrumentation } from '../../build/src/index.js';
import { ExpressInstrumentation } from '../../../build/src/index.js';

const sdk = createTestNodeSdk({
serviceName: 'use-express-nested',
Expand Down
9 changes: 3 additions & 6 deletions ...ion-express/test/fixtures/use-express.mjs → ...-express/test/v4/fixtures/use-express.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,12 @@ import { promisify } from 'util';
import { createTestNodeSdk } from '@opentelemetry/contrib-test-utils';

import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { ExpressInstrumentation } from '../../build/src/index.js';
import { ExpressInstrumentation } from '../../../build/src/index.js';

const sdk = createTestNodeSdk({
serviceName: 'use-express',
instrumentations: [
new HttpInstrumentation(),
new ExpressInstrumentation()
]
})
instrumentations: [new HttpInstrumentation(), new ExpressInstrumentation()],
});
sdk.start();

import express from 'express';
Expand Down
6 changes: 3 additions & 3 deletions ...nstrumentation-express/test/hooks.test.ts → ...rumentation-express/test/v4/hooks.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ import {
import * as assert from 'assert';
import type * as http from 'http';
import * as sinon from 'sinon';
import { ExpressInstrumentation } from '../src';
import { ExpressRequestInfo, SpanNameHook } from '../src/types';
import { ExpressLayerType } from '../src/enums/ExpressLayerType';
import { ExpressInstrumentation } from '../../src';
import { ExpressRequestInfo, SpanNameHook } from '../../src/types';
import { ExpressLayerType } from '../../src/enums/ExpressLayerType';
import { SEMATTRS_HTTP_METHOD } from '@opentelemetry/semantic-conventions';

const instrumentation = new ExpressInstrumentation();
Expand Down
4 changes: 2 additions & 2 deletions ...mentation-express/test/ignore-all.test.ts → ...tation-express/test/v4/ignore-all.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import {
} from '@opentelemetry/sdk-trace-base';
import * as assert from 'assert';
import { RPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core';
import { AttributeNames } from '../src/enums/AttributeNames';
import { ExpressInstrumentation, ExpressLayerType } from '../src';
import { AttributeNames } from '../../src/enums/AttributeNames';
import { ExpressInstrumentation, ExpressLayerType } from '../../src';
import { createServer, httpRequest } from './utils';

const instrumentation = new ExpressInstrumentation({
Expand Down
0 ...try-instrumentation-express/test/utils.ts → ...-instrumentation-express/test/v4/utils.ts
View file
Open in desktop
File renamed without changes.
Loading