feat(express): Skip update HTTP's span name and update RpcMetadata's route instead (#1557)

* feat(express): Skip update HTTP's span name and update RpcMetadata's route instead

* feat(express): simplify condition to when written to rpcMetadata.route

* feat(express): remove comment related to layerType being undefined

* chore: force build

* chore: force cache key

* Revert "chore: force cache key"

This reverts commit 483c7f1.

---------

Co-authored-by: Gerhard Stöbich <[email protected]>

Author: chigia001

plugins/node/opentelemetry-instrumentation-express/README.md

@@ -76,7 +76,7 @@ Express instrumentation has few options available to choose from. You can set th
 
 `spanNameHook` is invoked with 2 arguments:
 
-- `info: ExpressRequestInfo` containing the incoming Express.js request, the current route handler creating a span and `ExpressLayerType` - the type of the handling layer or undefined when renaming the root HTTP instrumentation span.
+- `info: ExpressRequestInfo` containing the incoming Express.js request, the current route handler creating a span and `ExpressLayerType` - the type of the handling layer.
 - `defaultName: string` - original name proposed by the instrumentation.
 
 #### Ignore a whole Express route

plugins/node/opentelemetry-instrumentation-express/src/instrumentation.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { setRPCMetadata, getRPCMetadata, RPCType } from '@opentelemetry/core';
+import { getRPCMetadata, RPCType } from '@opentelemetry/core';
 import { trace, context, diag, SpanAttributes } from '@opentelemetry/api';
 import type * as express from 'express';
 import { ExpressInstrumentationConfig, ExpressRequestInfo } from './types';
@@ -198,18 +198,10 @@ export class ExpressInstrumentation extends InstrumentationBase<
         // once we reach the request handler
         const rpcMetadata = getRPCMetadata(context.active());
         if (
-          metadata.attributes[AttributeNames.EXPRESS_TYPE] ===
-            ExpressLayerType.REQUEST_HANDLER &&
+          type === ExpressLayerType.REQUEST_HANDLER &&
           rpcMetadata?.type === RPCType.HTTP
         ) {
-          const name = instrumentation._getSpanName(
-            {
-              request: req,
-              route,
-            },
-            `${req.method} ${route.length > 0 ? route : '/'}`
-          );
-          rpcMetadata.span.updateName(name);
+          rpcMetadata.route = route || '/';
         }
 
         // verify against the config if the layer should be ignored
@@ -270,13 +262,6 @@ 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) {
@@ -288,7 +273,7 @@ export class ExpressInstrumentation extends InstrumentationBase<
               (req[_LAYERS_STORE_PROPERTY] as string[]).pop();
             }
             const callback = args[callbackIdx] as Function;
-            return context.bind(newContext, callback).apply(this, arguments);
+            return callback.apply(this, arguments);
           };
         }
         const result = original.apply(this, arguments);

plugins/node/opentelemetry-instrumentation-express/src/types.ts

@@ -24,10 +24,7 @@ export type IgnoreMatcher = string | RegExp | ((name: string) => boolean);
 export type ExpressRequestInfo = {
   request: Request;
   route: string;
-  /**
-   * If layerType is undefined, SpanNameHook is being invoked to rename the original root HTTP span.
-   */
-  layerType?: ExpressLayerType;
+  layerType: ExpressLayerType;
 };
 
 export type SpanNameHook = (

plugins/node/opentelemetry-instrumentation-express/test/custom-config.test.ts

@@ -23,7 +23,7 @@ import {
 } from '@opentelemetry/sdk-trace-base';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
 import * as assert from 'assert';
-import { RPCType, setRPCMetadata } from '@opentelemetry/core';
+import { RPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core';
 import { ExpressLayerType } from '../src/enums/ExpressLayerType';
 import { AttributeNames } from '../src/enums/AttributeNames';
 import { ExpressInstrumentation, ExpressInstrumentationConfig } from '../src';
@@ -110,8 +110,9 @@ describe('ExpressInstrumentation', () => {
     });
 
     it('should not repeat middleware paths in the span name', async () => {
+      let rpcMetadata: RPCMetadata;
       app.use((req, res, next) => {
-        const rpcMetadata = { type: RPCType.HTTP, span: rootSpan };
+        rpcMetadata = { type: RPCType.HTTP, span: rootSpan };
         return context.with(
           setRPCMetadata(
             trace.setSpan(context.active(), rootSpan),
@@ -139,8 +140,6 @@ describe('ExpressInstrumentation', () => {
           assert.strictEqual(response, 'ok');
           rootSpan.end();
 
-          const spans = memoryExporter.getFinishedSpans();
-
           const requestHandlerSpan = memoryExporter
             .getFinishedSpans()
             .find(span => span.name.includes('request handler'));
@@ -154,8 +153,7 @@ describe('ExpressInstrumentation', () => {
             requestHandlerSpan?.attributes[AttributeNames.EXPRESS_TYPE],
             'request_handler'
           );
-          const exportedRootSpan = spans.find(span => span.name === 'GET /mw');
-          assert.notStrictEqual(exportedRootSpan, undefined);
+          assert.strictEqual(rpcMetadata.route, '/mw');
         }
       );
     });
@@ -167,8 +165,9 @@ describe('ExpressInstrumentation', () => {
           ExpressLayerType.REQUEST_HANDLER,
         ],
       } as ExpressInstrumentationConfig);
+      let rpcMetadata: RPCMetadata;
       app.use((req, res, next) => {
-        const rpcMetadata = { type: RPCType.HTTP, span: rootSpan };
+        rpcMetadata = { type: RPCType.HTTP, span: rootSpan };
         return context.with(
           setRPCMetadata(
             trace.setSpan(context.active(), rootSpan),
@@ -192,8 +191,6 @@ describe('ExpressInstrumentation', () => {
           assert.strictEqual(response, 'ok');
           rootSpan.end();
 
-          const spans = memoryExporter.getFinishedSpans();
-
           const requestHandlerSpan = memoryExporter
             .getFinishedSpans()
             .find(span => span.name.includes('request handler'));
@@ -207,8 +204,7 @@ describe('ExpressInstrumentation', () => {
             requestHandlerSpan?.attributes[AttributeNames.EXPRESS_TYPE],
             'request_handler'
           );
-          const exportedRootSpan = spans.find(span => span.name === 'GET /');
-          assert.notStrictEqual(exportedRootSpan, undefined);
+          assert.strictEqual(rpcMetadata?.route, '/');
         }
       );
     });