diff --git a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts index e2b7823465..aea093d3f0 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts @@ -14,7 +14,12 @@ * limitations under the License. */ -import { hrTime } from '@opentelemetry/core'; +import { + hrTime, + setRPCMetadata, + getRPCMetadata, + RPCType, +} from '@opentelemetry/core'; import { trace, context, diag, SpanAttributes } from '@opentelemetry/api'; import * as express from 'express'; import { @@ -23,7 +28,6 @@ import { PatchedRequest, _LAYERS_STORE_PROPERTY, ExpressInstrumentationConfig, - ExpressInstrumentationSpan, } from './types'; import { ExpressLayerType } from './enums/ExpressLayerType'; import { AttributeNames } from './enums/AttributeNames'; @@ -183,7 +187,7 @@ export class ExpressInstrumentation extends InstrumentationBase< .filter(path => path !== '/' && path !== '/*') .join(''); const attributes: SpanAttributes = { - [SemanticAttributes.HTTP_ROUTE]: route.length > 0 ? route : undefined, + [SemanticAttributes.HTTP_ROUTE]: route.length > 0 ? route : '/', }; const metadata = getLayerMetadata(layer, layerPath); const type = metadata.attributes[ @@ -192,19 +196,15 @@ export class ExpressInstrumentation extends InstrumentationBase< // 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 ( metadata.attributes[AttributeNames.EXPRESS_TYPE] === - ExpressLayerType.REQUEST_HANDLER + ExpressLayerType.REQUEST_HANDLER && + rpcMetadata?.type === RPCType.HTTP ) { - const parent = trace.getSpan( - context.active() - ) as ExpressInstrumentationSpan; - if (parent?.name) { - const parentRoute = parent.name.split(' ')[1]; - if (!route.includes(parentRoute)) { - parent.updateName(`${req.method} ${route}`); - } - } + rpcMetadata.span.updateName( + `${req.method} ${route.length > 0 ? route : '/'}` + ); } // verify against the config if the layer should be ignored @@ -242,6 +242,13 @@ export class ExpressInstrumentation extends InstrumentationBase< // verify we have a callback const args = Array.from(arguments); const callbackIdx = args.findIndex(arg => typeof arg === 'function'); + const newContext = + rpcMetadata?.type === RPCType.HTTP + ? setRPCMetadata( + context.active(), + Object.assign(rpcMetadata, { route: route }) + ) + : context.active(); if (callbackIdx >= 0) { arguments[callbackIdx] = function () { if (spanHasEnded === false) { @@ -253,7 +260,7 @@ export class ExpressInstrumentation extends InstrumentationBase< (req[_LAYERS_STORE_PROPERTY] as string[]).pop(); } const callback = args[callbackIdx] as Function; - return context.bind(callback).apply(this, arguments); + return context.bind(callback, newContext).apply(this, arguments); }; } const result = original.apply(this, arguments); diff --git a/plugins/node/opentelemetry-instrumentation-express/src/types.ts b/plugins/node/opentelemetry-instrumentation-express/src/types.ts index f110a562b7..30aa38eddd 100644 --- a/plugins/node/opentelemetry-instrumentation-express/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-express/src/types.ts @@ -16,7 +16,7 @@ import { kLayerPatched } from './'; import { Request } from 'express'; -import { SpanAttributes, Span } from '@opentelemetry/api'; +import { SpanAttributes } from '@opentelemetry/api'; import { InstrumentationConfig } from '@opentelemetry/instrumentation'; import { ExpressLayerType } from './enums/ExpressLayerType'; @@ -79,10 +79,3 @@ export interface ExpressInstrumentationConfig extends InstrumentationConfig { /** Ignore specific layers based on their type */ ignoreLayersType?: ExpressLayerType[]; } - -/** - * extends opentelemetry/api Span object to instrument the root span name of http instrumentation - */ -export interface ExpressInstrumentationSpan extends Span { - name?: string; -} diff --git a/plugins/node/opentelemetry-instrumentation-express/test/custom-config.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/custom-config.test.ts index b43d5db1a1..6d39082969 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/custom-config.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/custom-config.test.ts @@ -23,10 +23,10 @@ import { } from '@opentelemetry/tracing'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import * as assert from 'assert'; -import { ExpressInstrumentationSpan } from '../src/types'; +import { RPCType, setRPCMetadata } from '@opentelemetry/core'; import { ExpressLayerType } from '../src/enums/ExpressLayerType'; import { AttributeNames } from '../src/enums/AttributeNames'; -import { ExpressInstrumentation } from '../src'; +import { ExpressInstrumentation, ExpressInstrumentationConfig } from '../src'; const instrumentation = new ExpressInstrumentation({ ignoreLayersType: [ExpressLayerType.MIDDLEWARE], @@ -129,9 +129,16 @@ describe('ExpressInstrumentation', () => { }); it('should not repeat middleware paths in the span name', async () => { - app.use((req, res, next) => - context.with(trace.setSpan(context.active(), rootSpan), next) - ); + app.use((req, res, next) => { + const rpcMetadata = { type: RPCType.HTTP, span: rootSpan }; + return context.with( + setRPCMetadata( + trace.setSpan(context.active(), rootSpan), + rpcMetadata + ), + next + ); + }); app.use('/mw', (req, res, next) => { next(); @@ -141,9 +148,7 @@ describe('ExpressInstrumentation', () => { res.send('ok'); }); - const rootSpan = tracer.startSpan( - 'rootSpan' - ) as ExpressInstrumentationSpan; + const rootSpan = tracer.startSpan('rootSpan'); assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); await context.with( @@ -153,8 +158,6 @@ describe('ExpressInstrumentation', () => { assert.strictEqual(response, 'ok'); rootSpan.end(); - assert.strictEqual(rootSpan.name, 'GET /mw'); - const spans = memoryExporter.getFinishedSpans(); const requestHandlerSpan = memoryExporter @@ -175,5 +178,58 @@ describe('ExpressInstrumentation', () => { } ); }); + + it('should correctly name http root path when its /', async () => { + instrumentation.setConfig({ + ignoreLayerTypes: [ + ExpressLayerType.MIDDLEWARE, + ExpressLayerType.REQUEST_HANDLER, + ], + } as ExpressInstrumentationConfig); + app.use((req, res, next) => { + const rpcMetadata = { type: RPCType.HTTP, span: rootSpan }; + return context.with( + setRPCMetadata( + trace.setSpan(context.active(), rootSpan), + rpcMetadata + ), + next + ); + }); + + app.get('/', (req, res) => { + res.send('ok'); + }); + + const rootSpan = tracer.startSpan('rootSpan'); + assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); + + await context.with( + trace.setSpan(context.active(), rootSpan), + async () => { + const response = await httpRequest.get(`http://localhost:${port}/`); + assert.strictEqual(response, 'ok'); + rootSpan.end(); + + const spans = memoryExporter.getFinishedSpans(); + + const requestHandlerSpan = memoryExporter + .getFinishedSpans() + .find(span => span.name.includes('request handler')); + assert.notStrictEqual(requestHandlerSpan, undefined); + assert.strictEqual( + requestHandlerSpan?.attributes[SemanticAttributes.HTTP_ROUTE], + '/' + ); + + assert.strictEqual( + requestHandlerSpan?.attributes[AttributeNames.EXPRESS_TYPE], + 'request_handler' + ); + const exportedRootSpan = spans.find(span => span.name === 'GET /'); + assert.notStrictEqual(exportedRootSpan, undefined); + } + ); + }); }); }); diff --git a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts index dbdbfc97c2..4097289978 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/express.test.ts @@ -22,7 +22,7 @@ import { SimpleSpanProcessor, } from '@opentelemetry/tracing'; import * as assert from 'assert'; -import { ExpressInstrumentationSpan } from '../src/types'; +import { setRPCMetadata, RPCType } from '@opentelemetry/core'; import { AttributeNames } from '../src/enums/AttributeNames'; import { ExpressInstrumentation } from '../src'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; @@ -61,9 +61,13 @@ const serverWithMiddleware = async ( ): Promise => { const app = express(); if (tracer) { - app.use((req, res, next) => - context.with(trace.setSpan(context.active(), rootSpan), next) - ); + app.use((req, res, next) => { + const rpcMetadata = { type: RPCType.HTTP, span: rootSpan }; + return context.with( + setRPCMetadata(trace.setSpan(context.active(), rootSpan), rpcMetadata), + next + ); + }); } app.use(express.json()); @@ -109,9 +113,7 @@ describe('ExpressInstrumentation', () => { describe('Instrumenting normal get operations', () => { it('should create a child span for middlewares', async () => { - const rootSpan = tracer.startSpan( - 'rootSpan' - ) as ExpressInstrumentationSpan; + const rootSpan = tracer.startSpan('rootSpan'); const app = express(); app.use((req, res, next) => context.with(trace.setSpan(context.active(), rootSpan), next) @@ -148,7 +150,6 @@ describe('ExpressInstrumentation', () => { ); assert.strictEqual(response, 'tata'); rootSpan.end(); - assert.strictEqual(rootSpan.name, 'GET /toto/:id'); assert.strictEqual(finishListenerCount, 2); assert.notStrictEqual( memoryExporter diff --git a/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts b/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts index 2a52cfbd45..69bbd42c54 100644 --- a/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts +++ b/plugins/node/opentelemetry-instrumentation-express/test/ignore-all.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { context, trace } from '@opentelemetry/api'; +import { context, trace, Span } from '@opentelemetry/api'; import { NodeTracerProvider } from '@opentelemetry/node'; import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; import { @@ -22,7 +22,7 @@ import { SimpleSpanProcessor, } from '@opentelemetry/tracing'; import * as assert from 'assert'; -import { ExpressInstrumentationSpan } from '../src/types'; +import { RPCType, setRPCMetadata } from '@opentelemetry/core'; import { AttributeNames } from '../src/enums/AttributeNames'; import { ExpressInstrumentation, ExpressLayerType } from '../src'; @@ -81,14 +81,22 @@ describe('ExpressInstrumentation', () => { describe('when route exists', () => { let server: http.Server; - let rootSpan: ExpressInstrumentationSpan; + let rootSpan: Span; beforeEach(async () => { - rootSpan = tracer.startSpan('rootSpan') as ExpressInstrumentationSpan; + rootSpan = tracer.startSpan('rootSpan'); const app = express(); - app.use((req, res, next) => - context.with(trace.setSpan(context.active(), rootSpan), next) - ); + + app.use((req, res, next) => { + const rpcMetadata = { type: RPCType.HTTP, span: rootSpan }; + return context.with( + setRPCMetadata( + trace.setSpan(context.active(), rootSpan), + rpcMetadata + ), + next + ); + }); app.use(express.json()); app.use((req, res, next) => { for (let i = 0; i < 1000; i++) {} @@ -144,7 +152,6 @@ describe('ExpressInstrumentation', () => { async () => { await httpRequest.get(`http://localhost:${port}/toto/tata`); rootSpan.end(); - assert.strictEqual(rootSpan.name, 'GET /toto/:id'); const exportedRootSpan = memoryExporter .getFinishedSpans() .find(span => span.name === 'GET /toto/:id'); diff --git a/plugins/node/opentelemetry-instrumentation-koa/package.json b/plugins/node/opentelemetry-instrumentation-koa/package.json index 67716ba7b8..146f411dda 100644 --- a/plugins/node/opentelemetry-instrumentation-koa/package.json +++ b/plugins/node/opentelemetry-instrumentation-koa/package.json @@ -60,6 +60,7 @@ }, "dependencies": { "@opentelemetry/api": "^0.20.0", + "@opentelemetry/core": "^0.20.0", "@opentelemetry/instrumentation": "^0.20.0", "@opentelemetry/semantic-conventions": "^0.20.0", "@types/koa": "2.13.1", diff --git a/plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts index 64498dc856..f4cc15beda 100644 --- a/plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts @@ -33,6 +33,7 @@ import { import { AttributeNames } from './enums/AttributeNames'; import { VERSION } from './version'; import { getMiddlewareMetadata } from './utils'; +import { getRPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core'; /** Koa instrumentation for OpenTelemetry */ export class KoaInstrumentation extends InstrumentationBase { @@ -143,41 +144,34 @@ export class KoaInstrumentation extends InstrumentationBase { attributes: metadata.attributes, }); - if (!context.request.ctx.parentSpan) { - context.request.ctx.parentSpan = parent; - } + const rpcMetadata = getRPCMetadata(api.context.active()); if ( - metadata.attributes[AttributeNames.KOA_TYPE] === KoaLayerType.ROUTER + metadata.attributes[AttributeNames.KOA_TYPE] === KoaLayerType.ROUTER && + rpcMetadata?.type === RPCType.HTTP ) { - if (context.request.ctx.parentSpan.name) { - const parentRoute = context.request.ctx.parentSpan.name.split(' ')[1]; - if ( - context._matchedRoute && - !context._matchedRoute.toString().includes(parentRoute) - ) { - context.request.ctx.parentSpan.updateName( - `${context.method} ${context._matchedRoute}` - ); - - delete context.request.ctx.parentSpan; - } - } + rpcMetadata.span.updateName( + `${context.method} ${context._matchedRoute}` + ); } - return api.context.with( - api.trace.setSpan(api.context.active(), span), - async () => { - try { - return await middlewareLayer(context, next); - } catch (err) { - span.recordException(err); - throw err; - } finally { - span.end(); - } + let newContext = api.trace.setSpan(api.context.active(), span); + if (rpcMetadata?.type === RPCType.HTTP) { + newContext = setRPCMetadata( + newContext, + Object.assign(rpcMetadata, { route: context._matchedRoute }) + ); + } + return api.context.with(newContext, async () => { + try { + return await middlewareLayer(context, next); + } catch (err) { + span.recordException(err); + throw err; + } finally { + span.end(); } - ); + }); }; } } diff --git a/plugins/node/opentelemetry-instrumentation-koa/test/koa.test.ts b/plugins/node/opentelemetry-instrumentation-koa/test/koa.test.ts index 4bd95569a2..ee23c42741 100644 --- a/plugins/node/opentelemetry-instrumentation-koa/test/koa.test.ts +++ b/plugins/node/opentelemetry-instrumentation-koa/test/koa.test.ts @@ -33,6 +33,7 @@ import * as http from 'http'; import { AddressInfo } from 'net'; import { KoaLayerType } from '../src/types'; import { AttributeNames } from '../src/enums/AttributeNames'; +import { RPCType, setRPCMetadata } from '@opentelemetry/core'; const httpRequest = { get: (options: http.ClientRequestArgs | string) => { @@ -122,8 +123,15 @@ describe('Koa Instrumentation', () => { describe('Instrumenting @koa/router calls', () => { it('should create a child span for middlewares', async () => { const rootSpan = tracer.startSpan('rootSpan'); + const rpcMetadata = { type: RPCType.HTTP, span: rootSpan }; app.use((ctx, next) => - context.with(trace.setSpan(context.active(), rootSpan), next) + context.with( + setRPCMetadata( + trace.setSpan(context.active(), rootSpan), + rpcMetadata + ), + next + ) ); const router = new KoaRouter(); @@ -165,8 +173,15 @@ describe('Koa Instrumentation', () => { it('should correctly instrument nested routers', async () => { const rootSpan = tracer.startSpan('rootSpan'); + const rpcMetadata = { type: RPCType.HTTP, span: rootSpan }; app.use((ctx, next) => - context.with(trace.setSpan(context.active(), rootSpan), next) + context.with( + setRPCMetadata( + trace.setSpan(context.active(), rootSpan), + rpcMetadata + ), + next + ) ); const router = new KoaRouter(); @@ -210,8 +225,15 @@ describe('Koa Instrumentation', () => { it('should correctly instrument prefixed routers', async () => { const rootSpan = tracer.startSpan('rootSpan'); + const rpcMetadata = { type: RPCType.HTTP, span: rootSpan }; app.use((ctx, next) => - context.with(trace.setSpan(context.active(), rootSpan), next) + context.with( + setRPCMetadata( + trace.setSpan(context.active(), rootSpan), + rpcMetadata + ), + next + ) ); const router = new KoaRouter(); diff --git a/plugins/node/opentelemetry-instrumentation-restify/package.json b/plugins/node/opentelemetry-instrumentation-restify/package.json index 0be54aa46d..0f00f42088 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/package.json +++ b/plugins/node/opentelemetry-instrumentation-restify/package.json @@ -57,6 +57,7 @@ }, "dependencies": { "@opentelemetry/api": "^0.20.0", + "@opentelemetry/core": "^0.20.0", "@opentelemetry/instrumentation": "^0.20.0", "@opentelemetry/semantic-conventions": "^0.20.0", "@types/restify": "4.3.7" diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/constants.ts b/plugins/node/opentelemetry-instrumentation-restify/src/constants.ts index 2ca6d52182..24a21e6251 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/constants.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/constants.ts @@ -25,4 +25,3 @@ export const RESTIFY_METHODS = [ ]; export const MODULE_NAME = 'restify'; export const SUPPORTED_VERSIONS = ['>=4.0.0']; -export const REQ_SPAN = Symbol('REQ_SPAN'); diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts index e72283be8f..1804573684 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts @@ -29,6 +29,7 @@ import { } from '@opentelemetry/instrumentation'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import { isPromise, isAsyncFunction } from './utils'; +import { getRPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core'; const { diag } = api; @@ -159,23 +160,9 @@ export class RestifyInstrumentation extends InstrumentationBase< : req.route?.path; // replace HTTP instrumentations name with one that contains a route - // in first handlers, we might not now the route yet, in which case the HTTP - // span has to be stored and fixed in later handler. - // https://github.com/open-telemetry/opentelemetry-specification/blob/a44d863edcdef63b0adce7b47df001933b7a158a/specification/trace/semantic_conventions/http.md#name - if (req[constants.REQ_SPAN] === undefined) { - req[constants.REQ_SPAN] = api.trace.getSpan( - api.context.active() - ) as types.InstrumentationSpan; - } - if ( - route && - req[constants.REQ_SPAN] && - req[constants.REQ_SPAN]?.name?.startsWith('HTTP ') - ) { - (req[constants.REQ_SPAN] as types.InstrumentationSpan).updateName( - `${req.method} ${route}` - ); - req[constants.REQ_SPAN] = false; + const httpMetadata = getRPCMetadata(api.context.active()); + if (httpMetadata?.type === RPCType.HTTP) { + httpMetadata.span.updateName(`${req.method} ${route}`); } const fnName = handler.name || undefined; @@ -216,8 +203,15 @@ export class RestifyInstrumentation extends InstrumentationBase< }); }; + let newContext = api.trace.setSpan(api.context.active(), span); + if (httpMetadata) { + newContext = setRPCMetadata( + newContext, + Object.assign(httpMetadata, { route }) + ); + } return api.context.with( - api.trace.setSpan(api.context.active(), span), + newContext, (req: types.Request, res: restify.Response, next: restify.Next) => { if (isAsyncFunction(handler)) { return wrapPromise(handler(req, res, next)); diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts index 85bd3afe04..89da9fd49c 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts @@ -15,7 +15,6 @@ */ import { Span } from '@opentelemetry/api'; import * as restify from 'restify'; -import { REQ_SPAN } from './constants'; export enum LayerType { MIDDLEWARE = 'middleware', @@ -23,8 +22,6 @@ export enum LayerType { } declare interface RequestWithRoute extends restify.Request { - // undefined /* uninitialized */ | false /* renamed */ | InstrumentationSpan /* not yet renamed */ - [REQ_SPAN]?: any; route: { path: string }; getRoute: () => { path: string }; } diff --git a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts index d325a9b31f..9de84c27c3 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts @@ -16,6 +16,7 @@ import * as restify from 'restify'; import { context, trace } from '@opentelemetry/api'; +import { RPCType, setRPCMetadata } from '@opentelemetry/core'; import { NodeTracerProvider } from '@opentelemetry/node'; import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; import { @@ -259,9 +260,19 @@ describe('Restify Instrumentation', () => { const httpSpan: types.InstrumentationSpan = tracer.startSpan('HTTP GET'); const testLocalServer = await createServer((server: restify.Server) => { + const rpcMetadata = { + type: RPCType.HTTP, + span: httpSpan, + }; server.pre((req, res, next) => { // to simulate HTTP instrumentation - context.with(trace.setSpan(context.active(), httpSpan), next); + context.with( + setRPCMetadata( + trace.setSpan(context.active(), httpSpan), + rpcMetadata + ), + next + ); }); server.get('/route/:param', getHandler); });