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

feat: enable root span route instrumentation without any express layer spans #273

Merged
merged 11 commits into from
Jan 7, 2021
Merged
1 change: 1 addition & 0 deletions plugins/node/opentelemetry-plugin-express/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
"@types/node": "14.0.27",
"@types/shimmer": "1.0.1",
"codecov": "3.7.2",
"eslint-plugin-header": "^3.1.0",
"express": "4.17.1",
"gts": "2.0.2",
"mocha": "7.2.0",
Expand Down
32 changes: 21 additions & 11 deletions plugins/node/opentelemetry-plugin-express/src/express.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ export class ExpressPlugin extends BasePlugin<typeof express> {
) {
storeLayerPath(req, layerPath);
const route = (req[_LAYERS_STORE_PROPERTY] as string[])
.filter(path => path !== '/')
.filter(path => path !== '/' && path !== '/*')
.join('');
const attributes: Attributes = {
[AttributeNames.COMPONENT]: ExpressPlugin.component,
Expand All @@ -180,23 +180,33 @@ export class ExpressPlugin extends BasePlugin<typeof express> {
const type = metadata.attributes[
AttributeNames.EXPRESS_TYPE
] as ExpressLayerType;
// verify against the config if the layer should be ignored
if (isLayerIgnored(metadata.name, type, plugin._config)) {
return original.apply(this, arguments);
}
if (plugin._tracer.getCurrentSpan() === undefined) {
return original.apply(this, arguments);
}
// Rename the root http span once we reach the request handler

// Rename the root http span in case we haven't done it already
// once we reach the request handler
if (
metadata.attributes[AttributeNames.EXPRESS_TYPE] ===
ExpressLayerType.REQUEST_HANDLER
) {
const parent = plugin._tracer.getCurrentSpan();
if (parent) {
parent.updateName(`${req.method} ${route}`);
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore
shyimo marked this conversation as resolved.
Show resolved Hide resolved
const parentSpanName = parent && parent.name;
if (parent && parentSpanName) {
shyimo marked this conversation as resolved.
Show resolved Hide resolved
const parentRoute = parentSpanName.split(' ')[1];
if (!route.includes(parentRoute)) {
parent.updateName(`${req.method} ${route}`);
}
}
}

// verify against the config if the layer should be ignored
if (isLayerIgnored(metadata.name, type, plugin._config)) {
return original.apply(this, arguments);
}
if (plugin._tracer.getCurrentSpan() === undefined) {
return original.apply(this, arguments);
}

const span = plugin._tracer.startSpan(metadata.name, {
attributes: Object.assign(attributes, metadata.attributes),
});
Expand Down
61 changes: 61 additions & 0 deletions plugins/node/opentelemetry-plugin-express/test/express.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ describe('Express Plugin', () => {
await tracer.withSpan(rootSpan, async () => {
await httpRequest.get(`http://localhost:${port}/toto/tata`);
rootSpan.end();
// @ts-ignore
shyimo marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(rootSpan.name, 'GET /toto/:id');
assert.notStrictEqual(
memoryExporter
.getFinishedSpans()
Expand Down Expand Up @@ -204,6 +206,65 @@ describe('Express Plugin', () => {
});
server.close();
});
it('should ignore all ExpressLayerType based on config. root span name should be modified when route exists', async () => {
shyimo marked this conversation as resolved.
Show resolved Hide resolved
plugin.disable();
const config: ExpressPluginConfig = {
ignoreLayersType: [
ExpressLayerType.MIDDLEWARE,
ExpressLayerType.ROUTER,
ExpressLayerType.REQUEST_HANDLER,
],
};
plugin.enable(express, provider, logger, config);
const rootSpan = tracer.startSpan('rootSpan');
const app = express();
app.use((req, res, next) => tracer.withSpan(rootSpan, next));
app.use(express.json());
app.use((req, res, next) => {
for (let i = 0; i < 1000; i++) {
continue;
}
return next();
});
const router = express.Router();
app.use('/toto', router);
router.get('/:id', (req, res) => {
setImmediate(() => {
res.status(200).end();
});
});
const server = http.createServer(app);
await new Promise(resolve => server.listen(0, resolve));

const port = (server.address() as AddressInfo).port;
assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
await tracer.withSpan(rootSpan, async () => {
await httpRequest.get(`http://localhost:${port}/toto/tata`);
rootSpan.end();
// @ts-ignore
shyimo marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(rootSpan.name, 'GET /toto/:id');

assert.deepStrictEqual(
memoryExporter
.getFinishedSpans()
.filter(
span =>
span.attributes[AttributeNames.EXPRESS_TYPE] ===
ExpressLayerType.MIDDLEWARE ||
span.attributes[AttributeNames.EXPRESS_TYPE] ===
ExpressLayerType.ROUTER ||
span.attributes[AttributeNames.EXPRESS_TYPE] ===
ExpressLayerType.REQUEST_HANDLER
).length,
0
);
const exportedRootSpan = memoryExporter
.getFinishedSpans()
.find(span => span.name === 'GET /toto/:id');
assert.notStrictEqual(exportedRootSpan, undefined);
});
server.close();
});
});

describe('Disabling plugin', () => {
Expand Down