chore: avoid unneeded context.with in http instrumentation (#2043)

Avoid the overhead of context.with() by directly passing the extracted
context to tracer.startSpan().
This ensures also that the extracted context is not accidently propagated
out of the HTTP instrumentation.

Co-authored-by: Valentin Marchaud <[email protected]>
Co-authored-by: Naseem <[email protected]>

Author: 3 people

packages/opentelemetry-instrumentation-http/src/http.ts

@@ -405,72 +405,35 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
         }),
       };
 
-      return context.with(propagation.extract(ROOT_CONTEXT, headers), () => {
-        const span = instrumentation._startHttpSpan(
-          `${component.toLocaleUpperCase()} ${method}`,
-          spanOptions
-        );
-
-        return context.with(setSpan(context.active(), span), () => {
-          context.bind(request);
-          context.bind(response);
-
-          if (instrumentation._getConfig().requestHook) {
-            instrumentation._callRequestHook(span, request);
-          }
-          if (instrumentation._getConfig().responseHook) {
-            instrumentation._callResponseHook(span, response);
-          }
-
-          // 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(
-              // eslint-disable-next-line @typescript-eslint/no-explicit-any
-              () => response.end.apply(this, arguments as any),
-              error => {
-                if (error) {
-                  utils.setSpanWithError(span, error);
-                  instrumentation._closeHttpSpan(span);
-                  throw error;
-                }
-              }
-            );
-
-            const attributes = utils.getIncomingRequestAttributesOnResponse(
-              request,
-              response
-            );
+      const ctx = propagation.extract(ROOT_CONTEXT, headers);
+      const span = instrumentation._startHttpSpan(
+        `${component.toLocaleUpperCase()} ${method}`,
+        spanOptions,
+        ctx
+      );
 
-            span
-              .setAttributes(attributes)
-              .setStatus(utils.parseResponseStatus(response.statusCode));
-
-            if (instrumentation._getConfig().applyCustomAttributesOnSpan) {
-              safeExecuteInTheMiddle(
-                () =>
-                  instrumentation._getConfig().applyCustomAttributesOnSpan!(
-                    span,
-                    request,
-                    response
-                  ),
-                () => {},
-                true
-              );
-            }
+      return context.with(setSpan(ctx, span), () => {
+        context.bind(request);
+        context.bind(response);
 
-            instrumentation._closeHttpSpan(span);
-            return returned;
-          };
+        if (instrumentation._getConfig().requestHook) {
+          instrumentation._callRequestHook(span, request);
+        }
+        if (instrumentation._getConfig().responseHook) {
+          instrumentation._callResponseHook(span, response);
+        }
 
-          return safeExecuteInTheMiddle(
-            () => original.apply(this, [event, ...args]),
+        // 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);
@@ -479,7 +442,43 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
               }
             }
           );
-        });
+
+          const attributes = utils.getIncomingRequestAttributesOnResponse(
+            request,
+            response
+          );
+
+          span
+            .setAttributes(attributes)
+            .setStatus(utils.parseResponseStatus(response.statusCode));
+
+          if (instrumentation._getConfig().applyCustomAttributesOnSpan) {
+            safeExecuteInTheMiddle(
+              () =>
+                instrumentation._getConfig().applyCustomAttributesOnSpan!(
+                  span,
+                  request,
+                  response
+                ),
+              () => {},
+              true
+            );
+          }
+
+          instrumentation._closeHttpSpan(span);
+          return returned;
+        };
+
+        return safeExecuteInTheMiddle(
+          () => original.apply(this, [event, ...args]),
+          error => {
+            if (error) {
+              utils.setSpanWithError(span, error);
+              instrumentation._closeHttpSpan(span);
+              throw error;
+            }
+          }
+        );
       });
     };
   }
@@ -576,7 +575,11 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
     };
   }
 
-  private _startHttpSpan(name: string, options: SpanOptions) {
+  private _startHttpSpan(
+    name: string,
+    options: SpanOptions,
+    ctx = context.active()
+  ) {
     /*
      * If a parent is required but not present, we use a `NoopSpan` to still
      * propagate context without recording it.
@@ -587,16 +590,16 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
         : this._getConfig().requireParentforIncomingSpans;
 
     let span: Span;
-    const currentSpan = getSpan(context.active());
+    const currentSpan = getSpan(ctx);
 
     if (requireParent === true && currentSpan === undefined) {
       // TODO: Refactor this when a solution is found in
       // https://github.com/open-telemetry/opentelemetry-specification/issues/530
-      span = NOOP_TRACER.startSpan(name, options);
+      span = NOOP_TRACER.startSpan(name, options, ctx);
     } else if (requireParent === true && currentSpan?.context().isRemote) {
       span = currentSpan;
     } else {
-      span = this.tracer.startSpan(name, options);
+      span = this.tracer.startSpan(name, options, ctx);
     }
     this._spanNotEnded.add(span);
     return span;