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.

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

fix(express): make rpcMetadata.route capture the last layer even when if the last layer is not REQUEST_HANDLER #1620

Merged
merged 15 commits into from
Aug 13, 2023
Merged

fix(express): make rpcMetadata.route capture the last layer even when if the last layer is not REQUEST_HANDLER #1620

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f144c02
fix(express): make rpcMetadata.route capture the last layer even when…
chigia001 Aug 1, 2023
6d15f52
fix lint issue
chigia001 Aug 2, 2023
21a5302
Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…
chigia001 Aug 2, 2023
b4f26f2
remove test.only
chigia001 Aug 2, 2023
259d49c
revert code to change ignore order
chigia001 Aug 3, 2023
1ee99d1
update test
chigia001 Aug 3, 2023
171763d
remove comment related to update span name
chigia001 Aug 3, 2023
8fefd23
Merge branch 'main' into fix-1618-express-rpcMetadata
chigia001 Aug 3, 2023
f54ba4c
Move rpcRoute.metadata calculation logic up
chigia001 Aug 4, 2023
8033026
Merge branch 'main' into fix-1618-express-rpcMetadata
chigia001 Aug 4, 2023
80bc054
Merge branch 'main' into fix-1618-express-rpcMetadata
chigia001 Aug 12, 2023
aafce10
Add more test
chigia001 Aug 13, 2023
32ddc04
Merge branch 'fix-1618-express-rpcMetadata' of github.com:chigia001/o…
chigia001 Aug 13, 2023
02eb353
Fix lint
chigia001 Aug 13, 2023
a5aec4a
Merge branch 'main' into fix-1618-express-rpcMetadata
chigia001 Aug 13, 2023
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
18 changes: 8 additions & 10 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 @@ -186,6 +186,7 @@ export class ExpressInstrumentation extends InstrumentationBase<
const route = (req[_LAYERS_STORE_PROPERTY] as string[])
.filter(path => path !== '/' && path !== '/*')
.join('');

const attributes: SpanAttributes = {
[SemanticAttributes.HTTP_ROUTE]: route.length > 0 ? route : '/',
};
Expand All @@ -194,23 +195,14 @@ export class ExpressInstrumentation extends InstrumentationBase<
AttributeNames.EXPRESS_TYPE
] as ExpressLayerType;

// Rename the root http span in case we haven't done it already
// once we reach the request handler
const rpcMetadata = getRPCMetadata(context.active());
if (
type === ExpressLayerType.REQUEST_HANDLER &&
rpcMetadata?.type === RPCType.HTTP
) {
rpcMetadata.route = route || '/';
}

// verify against the config if the layer should be ignored
if (isLayerIgnored(metadata.name, type, instrumentation._config)) {
if (type === ExpressLayerType.MIDDLEWARE) {
(req[_LAYERS_STORE_PROPERTY] as string[]).pop();
}
return original.apply(this, arguments);
}

if (trace.getSpan(context.active()) === undefined) {
return original.apply(this, arguments);
}
Expand Down Expand Up @@ -259,6 +251,12 @@ export class ExpressInstrumentation extends InstrumentationBase<
span.end();
}
};

const rpcMetadata = getRPCMetadata(context.active());
if (rpcMetadata?.type === RPCType.HTTP) {
rpcMetadata.route = route || '/';
}

// verify we have a callback
const args = Array.from(arguments);
const callbackIdx = args.findIndex(arg => typeof arg === 'function');
Expand Down
29 changes: 29 additions & 0 deletions plugins/node/opentelemetry-instrumentation-express/test/express.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,35 @@ describe('ExpressInstrumentation', () => {
assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
assert.strictEqual(res, 'test');
});

it('should update rpcMetadata.route with the latest middleware layer', async () => {
const rootSpan = tracer.startSpan('rootSpan');
let rpcMetadata: RPCMetadata | undefined;
const httpServer = await serverWithMiddleware(tracer, rootSpan, app => {
app.use(express.json());
app.use((req, res, next) => {
rpcMetadata = getRPCMetadata(context.active());
next();
});

app.use('/bare_middleware', (req, res) => {
Copy link
Member

Choose a reason for hiding this comment

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

This test should assert that the route is still captured when

type === ExpressLayerType.REQUEST_HANDLER

This type will be set when layer.name === 'bound dispatch'

  } else if (layer.name === 'bound dispatch') {
    return {
      attributes: {
        [AttributeNames.EXPRESS_NAME]: layerPath ?? 'request handler',
        [AttributeNames.EXPRESS_TYPE]: ExpressLayerType.REQUEST_HANDLER,
      },
      name: `request handler${layer.path ? ` - ${layerPath}` : ''}`,
    };

Is using the app.use triggers a layer with name 'bound dispatch'?

Copy link
Contributor Author

@chigia001 chigia001 Aug 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  const router = express.Router();

  router.use("/router_use", () => {}); // #3
  router.get('/tada/, () => {}); // #2

  app.use('/toto', router); // #1
  app.use('/bare_use', () => {}); // #3

  app.get('/bare_get', () => {}); // #2

#1, the middleware that bound app.use/router.use with Router object is 'router' layer

#2 is bound dispatch, ie, route handler with HTTP method name. This is for both Express's App and Express's Router object. This is something our unit test cover most of the time. Express's route handle will match to the end of URL.

#3 is the last case, IE middleware layer, where app.use/router.use have generic handler. It can be with or without the route's parameter .use will only need to match the start of the url.

Most of our test with app.use/router.use is for common's middleware use case, that don't have layername at the begining.

This new test for specificly app.use with route layer.

Copy link
Contributor Author

@chigia001 chigia001 Aug 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I add additional test for these 2 case, as I don't see we have specific test for them

  router.use("/router_use", () => {}); // #3
  app.get('/bare_get', () => {}); // #2

internally I think they are the same with case that we already cover (app.use === router.use and app.get === router.get) but just to make sure.

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_middleware`
);
assert.strictEqual(response, 'test');
rootSpan.end();
assert.strictEqual(rpcMetadata?.route, '/bare_middleware');
}
);
});
});

describe('Disabling plugin', () => {
Expand Down
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,14 @@ describe('ExpressInstrumentation', () => {
);
});

it('rpcMetadata.route should be modified to /todo/:id', async () => {
it('rpcMetadata.route should be undefined', async () => {
assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
await context.with(
trace.setSpan(context.active(), rootSpan),
async () => {
await httpRequest.get(`http://localhost:${port}/toto/tata`);
rootSpan.end();
assert.strictEqual(rpcMetadata.route, '/toto/:id');
assert.strictEqual(rpcMetadata.route, undefined);
Flarna marked this conversation as resolved.
Show resolved Hide resolved
}
);
});
Expand Down