fix(instrumentation-http): close server span when response finishes

open-telemetry · Nov 14, 2022 · bca9b9a · bca9b9a
1 parent c032493
commit bca9b9a
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 53 deletions.
diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
@@ -12,6 +12,8 @@ All notable changes to experimental packages in this project will be documented
 
 ### :bug: (Bug Fix)
 
+* feat(instrumentation-http): close server span when response finishes [#3407](https://github.com/open-telemetry/opentelemetry-js/pull/3407) @legendecas
+
 ### :books: (Refine Doc)
 
 ### :house: (Internal)

diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
@@ -43,7 +43,6 @@ import {
   HttpInstrumentationConfig,
   HttpRequestArgs,
   Https,
-  ResponseEndArgs,
 } from './types';
 import * as utils from './utils';
 import { VERSION } from './version';
@@ -479,54 +478,13 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
 
           instrumentation._headerCapture.server.captureRequestHeaders(span, header => request.headers[header]);
 
-          // Wraps end (inspired by:
-          // https://github.com/GoogleCloudPlatform/cloud-trace-nodejs/blob/master/src/instrumentations/instrumentation-connect.ts#L75)
-          const originalEnd = response.end;
-          response.end = function (
-            this: http.ServerResponse,
-            ..._args: ResponseEndArgs
-          ) {
-            response.end = originalEnd;
-            // Cannot pass args of type ResponseEndArgs,
-            const returned = safeExecuteInTheMiddle(
-              () => response.end.apply(this, arguments as never),
-              error => {
-                if (error) {
-                  utils.setSpanWithError(span, error);
-                  instrumentation._closeHttpSpan(span, SpanKind.SERVER, startTime, metricAttributes);
-                  throw error;
-                }
-              }
-            );
-
-            const attributes = utils.getIncomingRequestAttributesOnResponse(
-              request,
-              response
-            );
-            metricAttributes = Object.assign(metricAttributes, utils.getIncomingRequestMetricAttributesOnResponse(attributes));
-
-            instrumentation._headerCapture.server.captureResponseHeaders(span, header => response.getHeader(header));
-
-            span
-              .setAttributes(attributes)
-              .setStatus({ code: utils.parseResponseStatus(SpanKind.SERVER, response.statusCode) });
-
-            if (instrumentation._getConfig().applyCustomAttributesOnSpan) {
-              safeExecuteInTheMiddle(
-                () =>
-                  instrumentation._getConfig().applyCustomAttributesOnSpan!(
-                    span,
-                    request,
-                    response
-                  ),
-                () => { },
-                true
-              );
-            }
-
-            instrumentation._closeHttpSpan(span, SpanKind.SERVER, startTime, metricAttributes);
-            return returned;
-          };
+          // After 'error', no further events other than 'close' should be emitted.
+          response.on('finish', () => {
+            instrumentation._onServerResponseFinish(request, response, span, metricAttributes, startTime);
+          });
+          response.on(errorMonitor, (err: Err) => {
+            instrumentation._onServerResponseError(span, metricAttributes, startTime, err);
+          });
 
           return safeExecuteInTheMiddle(
             () => original.apply(this, [event, ...args]),
@@ -661,6 +619,40 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
     };
   }
 
+  private _onServerResponseFinish(request: http.IncomingMessage, response: http.ServerResponse, span: Span, metricAttributes: MetricAttributes, startTime: HrTime) {
+    const attributes = utils.getIncomingRequestAttributesOnResponse(
+      request,
+      response
+    );
+    metricAttributes = Object.assign(metricAttributes, utils.getIncomingRequestMetricAttributesOnResponse(attributes));
+
+    this._headerCapture.server.captureResponseHeaders(span, header => response.getHeader(header));
+
+    span
+      .setAttributes(attributes)
+      .setStatus({ code: utils.parseResponseStatus(SpanKind.SERVER, response.statusCode) });
+
+    if (this._getConfig().applyCustomAttributesOnSpan) {
+      safeExecuteInTheMiddle(
+        () =>
+          this._getConfig().applyCustomAttributesOnSpan!(
+            span,
+            request,
+            response
+          ),
+        () => { },
+        true
+      );
+    }
+
+    this._closeHttpSpan(span, SpanKind.SERVER, startTime, metricAttributes);
+  }
+
+  private _onServerResponseError(span: Span, metricAttributes: MetricAttributes, startTime: HrTime, error: Err) {
+    utils.setSpanWithError(span, error);
+    this._closeHttpSpan(span, SpanKind.SERVER, startTime, metricAttributes);
+  }
+
   private _startHttpSpan(
     name: string,
     options: SpanOptions,

diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/types.ts b/experimental/packages/opentelemetry-instrumentation-http/src/types.ts
@@ -50,10 +50,6 @@ export type Http = typeof http;
 export type Https = typeof https;
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 export type Func<T> = (...args: any[]) => T;
-export type ResponseEndArgs =
-  | [((() => void) | undefined)?]
-  | [unknown, ((() => void) | undefined)?]
-  | [unknown, string, ((() => void) | undefined)?];
 
 export interface HttpCustomAttributeFunction {
   (

diff --git a/...rimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts b/...rimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts
@@ -95,6 +95,14 @@ export const responseHookFunction = (
   response: IncomingMessage | ServerResponse
 ): void => {
   span.setAttribute('custom response hook attribute', 'response');
+  // IncomingMessage (Readable) 'end'.
+  response.on('end', () => {
+    span.setAttribute('custom incoming message attribute', 'end');
+  });
+  // ServerResponse (writable) 'finish'.
+  response.on('finish', () => {
+    span.setAttribute('custom server response attribute', 'finish');
+  });
 };
 
 export const startIncomingSpanHookFunction = (
@@ -772,6 +780,7 @@ describe('HttpInstrumentation', () => {
         const spans = memoryExporter.getFinishedSpans();
         const [incomingSpan, outgoingSpan] = spans;
 
+        // server request
         assert.strictEqual(
           incomingSpan.attributes['custom request hook attribute'],
           'request'
@@ -780,6 +789,10 @@ describe('HttpInstrumentation', () => {
           incomingSpan.attributes['custom response hook attribute'],
           'response'
         );
+        assert.strictEqual(
+          incomingSpan.attributes['custom server response attribute'],
+          'finish'
+        );
         assert.strictEqual(
           incomingSpan.attributes['guid'],
           'user_guid'
@@ -789,6 +802,7 @@ describe('HttpInstrumentation', () => {
           SpanKind.CLIENT
         );
 
+        // client request
         assert.strictEqual(
           outgoingSpan.attributes['custom request hook attribute'],
           'request'
@@ -797,6 +811,10 @@ describe('HttpInstrumentation', () => {
           outgoingSpan.attributes['custom response hook attribute'],
           'response'
         );
+        assert.strictEqual(
+          outgoingSpan.attributes['custom incoming message attribute'],
+          'end'
+        );
         assert.strictEqual(
           outgoingSpan.attributes['guid'],
           'user_guid'