PR feedback

open-telemetry · Jul 17, 2024 · 321f07c · 321f07c
1 parent 3d6f8ec
commit 321f07c
Show file tree

Hide file tree

Showing 6 changed files with 31 additions and 52 deletions.
diff --git a/experimental/packages/opentelemetry-instrumentation-http/README.md b/experimental/packages/opentelemetry-instrumentation-http/README.md
@@ -61,8 +61,8 @@ Http instrumentation has few options available to choose from. You can set the f
 | [`startOutgoingSpanHook`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L99) | `StartOutgoingSpanCustomAttributeFunction` | Function for adding custom attributes before a span is started in outgoingRequest |
 | `ignoreIncomingRequestHook` | `IgnoreIncomingRequestFunction` | Http instrumentation will not trace all incoming requests that matched with custom function |
 | `ignoreOutgoingRequestHook` | `IgnoreOutgoingRequestFunction` | Http instrumentation will not trace all outgoing requests that matched with custom function |
-| `instrumentOutgoingRequests` | `boolean` | Set to false to avoid instrumenting outgoing requests at all. |
-| `instrumentIncomingRequests` | `boolean` | Set to false to avoid instrumenting incoming requests at all. |
+| `disableOutgoingRequestInstrumentation` | `boolean` | Set to true to avoid instrumenting outgoing requests at all. This can be helpful when another instrumentation handles outgoing requests. |
+| `disableIncomingRequestInstrumentation` | `boolean` | Set to true to avoid instrumenting incoming requests at all. This can be helpful when another instrumentation handles incoming requests. |
 | [`serverName`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L101) | `string` | The primary server name of the matched virtual host. |
 | [`requireParentforOutgoingSpans`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L103) | Boolean | Require that is a parent span to create new span for outgoing requests. |
 | [`requireParentforIncomingSpans`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L105) | Boolean | Require that is a parent span to create new span for incoming requests. |

diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
@@ -109,32 +109,20 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
     return new InstrumentationNodeModuleDefinition(
       'http',
       ['*'],
-      moduleExports => {
-        this._diag.debug(`Applying patch for http@${version}`);
-        if (this._getConfig().instrumentOutgoingRequests !== false) {
-          if (isWrapped(moduleExports.request)) {
-            this._unwrap(moduleExports, 'request');
-          }
-
+      (moduleExports: Http): Http => {
+        if (!this.getConfig().disableOutgoingRequestInstrumentation) {
           this._wrap(
             moduleExports,
             'request',
             this._getPatchOutgoingRequestFunction('http')
           );
-
-          if (isWrapped(moduleExports.get)) {
-            this._unwrap(moduleExports, 'get');
-          }
           this._wrap(
             moduleExports,
             'get',
             this._getPatchOutgoingGetFunction(moduleExports.request)
           );
-          if (isWrapped(moduleExports.Server.prototype.emit)) {
-            this._unwrap(moduleExports.Server.prototype, 'emit');
-          }
         }
-        if (this._getConfig().instrumentIncomingRequests !== false) {
+        if (!this.getConfig().disableIncomingRequestInstrumentation) {
           this._wrap(
             moduleExports.Server.prototype,
             'emit',
@@ -146,11 +134,11 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
       (moduleExports: Http) => {
         if (moduleExports === undefined) return;
 
-        if (this._getConfig().instrumentOutgoingRequests !== false) {
+        if (!this.getConfig().disableOutgoingRequestInstrumentation) {
           this._unwrap(moduleExports, 'request');
           this._unwrap(moduleExports, 'get');
         }
-        if (this._getConfig().instrumentIncomingRequests !== false) {
+        if (!this.getConfig().disableIncomingRequestInstrumentation) {
           this._unwrap(moduleExports.Server.prototype, 'emit');
         }
       }
@@ -161,30 +149,21 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
     return new InstrumentationNodeModuleDefinition(
       'https',
       ['*'],
-      moduleExports => {
-        this._diag.debug(`Applying patch for https@${version}`);
-        if (this._getConfig().instrumentOutgoingRequests !== false) {
-          if (isWrapped(moduleExports.request)) {
-            this._unwrap(moduleExports, 'request');
-          }
+      (moduleExports: Https): Https => {
+        if (!this.getConfig().disableOutgoingRequestInstrumentation) {
           this._wrap(
             moduleExports,
             'request',
             this._getPatchHttpsOutgoingRequestFunction('https')
           );
-          if (isWrapped(moduleExports.get)) {
-            this._unwrap(moduleExports, 'get');
-          }
+
           this._wrap(
             moduleExports,
             'get',
             this._getPatchHttpsOutgoingGetFunction(moduleExports.request)
           );
         }
-        if (this._getConfig().instrumentIncomingRequests !== false) {
-          if (isWrapped(moduleExports.Server.prototype.emit)) {
-            this._unwrap(moduleExports.Server.prototype, 'emit');
-          }
+        if (!this.getConfig().disableIncomingRequestInstrumentation) {
           this._wrap(
             moduleExports.Server.prototype,
             'emit',
@@ -196,11 +175,11 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
       (moduleExports: Https) => {
         if (moduleExports === undefined) return;
 
-        if (this._getConfig().instrumentOutgoingRequests !== false) {
+        if (!this.getConfig().disableOutgoingRequestInstrumentation) {
           this._unwrap(moduleExports, 'request');
           this._unwrap(moduleExports, 'get');
         }
-        if (this._getConfig().instrumentIncomingRequests !== false) {
+        if (!this.getConfig().disableIncomingRequestInstrumentation) {
           this._unwrap(moduleExports.Server.prototype, 'emit');
         }
       }

diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/types.ts b/experimental/packages/opentelemetry-instrumentation-http/src/types.ts
@@ -98,10 +98,10 @@ export interface HttpInstrumentationConfig extends InstrumentationConfig {
   ignoreOutgoingUrls?: IgnoreMatcher[];
   /** Not trace all outgoing requests that matched with custom function */
   ignoreOutgoingRequestHook?: IgnoreOutgoingRequestFunction;
-  /** If set to false, incoming requests will not be instrumented at all. */
-  instrumentIncomingRequests?: boolean;
-  /** If set to false, outgoing requests will not be instrumented at all. */
-  instrumentOutgoingRequests?: boolean;
+  /** If set to true, incoming requests will not be instrumented at all. */
+  disableIncomingRequestInstrumentation?: boolean;
+  /** If set to true, outgoing requests will not be instrumented at all. */
+  disableOutgoingRequestInstrumentation?: boolean;
   /** Function for adding custom attributes after response is handled */
   applyCustomAttributesOnSpan?: HttpCustomAttributeFunction;
   /** Function for adding custom attributes before request is handled */

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
@@ -242,7 +242,7 @@ describe('HttpInstrumentation', () => {
         instrumentation.disable();
 
         instrumentation.setConfig({
-          instrumentOutgoingRequests: false,
+          disableOutgoingRequestInstrumentation: true,
         });
         instrumentation.enable();
         server = http.createServer((_request, response) => {
@@ -261,7 +261,7 @@ describe('HttpInstrumentation', () => {
         instrumentation.disable();
 
         instrumentation.setConfig({
-          instrumentIncomingRequests: false,
+          disableIncomingRequestInstrumentation: true,
         });
         instrumentation.enable();
         server = http.createServer((_request, response) => {

diff --git a/...imental/packages/opentelemetry-instrumentation-http/test/functionals/https-enable.test.ts b/...imental/packages/opentelemetry-instrumentation-http/test/functionals/https-enable.test.ts
@@ -718,7 +718,7 @@ describe('HttpsInstrumentation', () => {
 
       it('allows to disable outgoing request instrumentation', () => {
         instrumentation.setConfig({
-          instrumentOutgoingRequests: false,
+          disableOutgoingRequestInstrumentation: true,
         });
         instrumentation.enable();
         server = https.createServer(
@@ -740,7 +740,7 @@ describe('HttpsInstrumentation', () => {
 
       it('allows to disable incoming request instrumentation', () => {
         instrumentation.setConfig({
-          instrumentIncomingRequests: false,
+          disableIncomingRequestInstrumentation: true,
         });
         instrumentation.enable();
         server = https.createServer(

diff --git a/package-lock.json b/package-lock.json