fix(connect): Skip update HTTP's span name and update RpcMetadata's r…

…oute instead Signed-off-by: Chi Ma <chigia001@gmail.com>
open-telemetry · Jun 14, 2023 · f5ee4f2 · f5ee4f2
1 parent efdfc72
commit f5ee4f2
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 14 deletions.
diff --git a/plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
@@ -15,7 +15,7 @@
  */
 
 import { context, diag, Span, SpanOptions } from '@opentelemetry/api';
-import { getRPCMetadata, RPCType } from '@opentelemetry/core';
+import { getRPCMetadata, setRPCMetadata, RPCType } from '@opentelemetry/core';
 import type { HandleFunction, NextFunction, Server } from 'connect';
 import type { IncomingMessage, ServerResponse } from 'http';
 import {
@@ -120,16 +120,16 @@ export class ConnectInstrumentation extends InstrumentationBase<Server> {
       if (!instrumentation.isEnabled()) {
         return (middleWare as any).apply(this, arguments);
       }
-      const [reqArgIdx, resArgIdx, nextArgIdx] = isErrorMiddleware
-        ? [1, 2, 3]
-        : [0, 1, 2];
-      const req = arguments[reqArgIdx] as IncomingMessage;
+      const [resArgIdx, nextArgIdx] = isErrorMiddleware ? [2, 3] : [1, 2];
       const res = arguments[resArgIdx] as ServerResponse;
       const next = arguments[nextArgIdx] as NextFunction;
 
       const rpcMetadata = getRPCMetadata(context.active());
       if (routeName && rpcMetadata?.type === RPCType.HTTP) {
-        rpcMetadata.span.updateName(`${req.method} ${routeName || '/'}`);
+        setRPCMetadata(
+          context.active(),
+          Object.assign(rpcMetadata, { route: routeName || '/' })
+        );
       }
       let spanName = '';
       if (routeName) {

diff --git a/plugins/node/opentelemetry-instrumentation-connect/test/instrumentation.test.ts b/plugins/node/opentelemetry-instrumentation-connect/test/instrumentation.test.ts
@@ -16,7 +16,12 @@
 import * as assert from 'assert';
 
 import { context, trace } from '@opentelemetry/api';
-import { RPCType, setRPCMetadata } from '@opentelemetry/core';
+import {
+  RPCType,
+  getRPCMetadata,
+  setRPCMetadata,
+  RPCMetadata,
+} from '@opentelemetry/core';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
 import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
 import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
@@ -186,7 +191,7 @@ describe('connect', () => {
       assert.strictEqual(span.name, 'request handler - /foo');
     });
 
-    it('should change name for parent http route', async () => {
+    it('should not change name for parent http route ', async () => {
       const rootSpan = tracer.startSpan('root span');
       app.use((req, res, next) => {
         const rpcMetadata = { type: RPCType.HTTP, span: rootSpan };
@@ -209,13 +214,34 @@ describe('connect', () => {
       const spans = memoryExporter.getFinishedSpans();
       assert.strictEqual(spans.length, 3);
       const changedRootSpan = spans[2];
-      const span = spans[0];
-      assert.strictEqual(changedRootSpan.name, 'GET /foo');
-      assert.strictEqual(span.name, 'request handler - /foo');
-      assert.strictEqual(
-        span.parentSpanId,
-        changedRootSpan.spanContext().spanId
+      assert.strictEqual(changedRootSpan.name, 'root span');
+    });
+  });
+
+  it('should change route value of RpcMetadata', async () => {
+    const rootSpan = tracer.startSpan('root span');
+    app.use((req, res, next) => {
+      const rpcMetadata = { type: RPCType.HTTP, span: rootSpan };
+      return context.with(
+        setRPCMetadata(trace.setSpan(context.active(), rootSpan), rpcMetadata),
+        next
       );
     });
+    let rpcMetadata: RPCMetadata | undefined;
+    app.use('/foo', (req, res, next) => {
+      rpcMetadata = getRPCMetadata(context.active());
+      next();
+    });
+
+    await httpRequest.get(`http://localhost:${PORT}/foo`);
+    rootSpan.end();
+
+    const spans = memoryExporter.getFinishedSpans();
+    assert.strictEqual(spans.length, 3);
+    const changedRootSpan = spans[2];
+    const span = spans[0];
+    assert.strictEqual(rpcMetadata?.route, '/foo');
+    assert.strictEqual(span.name, 'request handler - /foo');
+    assert.strictEqual(span.parentSpanId, changedRootSpan.spanContext().spanId);
   });
 });