feat(instrumentation-http)!: remove deprecated ignore options (#5085)

experimental/CHANGELOG.md

@@ -21,6 +21,10 @@ All notable changes to experimental packages in this project will be documented
     * `appendResourcePathToUrlIfNeeded`
     * `configureExporterTimeout`
     * `invalidTimeout`
+* feat(instrumentation-http)!: remove long deprecated options [#5085](https://github.com/open-telemetry/opentelemetry-js/pull/5085) @pichlermarc
+  * `ignoreIncomingPaths` has been removed, use the more versatile `ignoreIncomingRequestHook` instead.
+  * `ignoreOutgoingUrls` has been removed, use the more versatile `ignoreOutgoingRequestHook` instead.
+  * `isIgnored` utility function was intended for internal use and has been removed without replacement.
 
 ### :rocket: (Enhancement)
 

experimental/packages/opentelemetry-instrumentation-http/README.md

@@ -68,12 +68,6 @@ Http instrumentation has few options available to choose from. You can set the f
 | [`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. |
 | [`headersToSpanAttributes`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L107) | `object` | List of case insensitive HTTP headers to convert to span attributes. Client (outgoing requests, incoming responses) and server (incoming requests, outgoing responses) headers will be converted to span attributes in the form of `http.{request\|response}.header.header_name`, e.g. `http.response.header.content_length` |
 
-The following options are deprecated:
-
-| Options | Type | Description |
-| ------- | ---- | ----------- |
-| `ignoreIncomingPaths` | `IgnoreMatcher[]` | Http instrumentation will not trace all incoming requests that match paths |
-
 ## Semantic Conventions
 
 Prior to version `0.54`, this instrumentation created spans targeting an experimental semantic convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md).

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

@@ -86,7 +86,6 @@ import {
   getOutgoingRequestMetricAttributesOnResponse,
   getRequestInfo,
   headerCapture,
-  isIgnored,
   isValidOptionsType,
   parseResponseStatus,
   setSpanWithError,
@@ -603,22 +602,13 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
 
       const request = args[0] as http.IncomingMessage;
       const response = args[1] as http.ServerResponse & { socket: Socket };
-      const pathname = request.url
-        ? url.parse(request.url).pathname || '/'
-        : '/';
       const method = request.method || 'GET';
 
       instrumentation._diag.debug(
         `${component} instrumentation incomingRequest`
       );
 
       if (
-        isIgnored(
-          pathname,
-          instrumentation.getConfig().ignoreIncomingPaths,
-          (e: unknown) =>
-            instrumentation._diag.error('caught ignoreIncomingPaths error: ', e)
-        ) ||
         safeExecuteInTheMiddle(
           () =>
             instrumentation.getConfig().ignoreIncomingRequestHook?.(request),
@@ -767,10 +757,7 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
         (typeof options === 'string' || options instanceof url.URL)
           ? (args.shift() as http.RequestOptions)
           : undefined;
-      const { origin, pathname, method, optionsParsed } = getRequestInfo(
-        options,
-        extraOptions
-      );
+      const { method, optionsParsed } = getRequestInfo(options, extraOptions);
       /**
        * Node 8's https module directly call the http one so to avoid creating
        * 2 span for the same request we need to check that the protocol is correct
@@ -785,12 +772,6 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
       }
 
       if (
-        isIgnored(
-          origin + pathname,
-          instrumentation.getConfig().ignoreOutgoingUrls,
-          (e: unknown) =>
-            instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e)
-        ) ||
         safeExecuteInTheMiddle(
           () =>
             instrumentation

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

@@ -51,7 +51,6 @@ export {
   getRequestInfo,
   headerCapture,
   isCompressed,
-  isIgnored,
   isValidOptionsType,
   parseResponseStatus,
   satisfiesPattern,

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

@@ -84,18 +84,8 @@ export interface StartOutgoingSpanCustomAttributeFunction {
  * Options available for the HTTP instrumentation (see [documentation](https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation-http#http-instrumentation-options))
  */
 export interface HttpInstrumentationConfig extends InstrumentationConfig {
-  /**
-   * Not trace all incoming requests that match paths
-   * @deprecated use `ignoreIncomingRequestHook` instead
-   */
-  ignoreIncomingPaths?: IgnoreMatcher[];
   /** Not trace all incoming requests that matched with custom function */
   ignoreIncomingRequestHook?: IgnoreIncomingRequestFunction;
-  /**
-   * Not trace all outgoing requests that match urls
-   * @deprecated use `ignoreOutgoingRequestHook` instead
-   */
-  ignoreOutgoingUrls?: IgnoreMatcher[];
   /** Not trace all outgoing requests that matched with custom function */
   ignoreOutgoingRequestHook?: IgnoreOutgoingRequestFunction;
   /** If set to true, incoming requests will not be instrumented at all. */

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

@@ -146,39 +146,6 @@ export const satisfiesPattern = (
   }
 };
 
-/**
- * Check whether the given request is ignored by configuration
- * It will not re-throw exceptions from `list` provided by the client
- * @param constant e.g URL of request
- * @param [list] List of ignore patterns
- * @param [onException] callback for doing something when an exception has
- *     occurred
- */
-export const isIgnored = (
-  constant: string,
-  list?: IgnoreMatcher[],
-  onException?: (error: unknown) => void
-): boolean => {
-  if (!list) {
-    // No ignored urls - trace everything
-    return false;
-  }
-  // Try/catch outside the loop for failing fast
-  try {
-    for (const pattern of list) {
-      if (satisfiesPattern(constant, pattern)) {
-        return true;
-      }
-    }
-  } catch (e) {
-    if (onException) {
-      onException(e);
-    }
-  }
-
-  return false;
-};
-
 /**
  * Sets the span with the error passed in params
  * @param {Span} span the span that need to be set

experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts

@@ -183,19 +183,9 @@ describe('HttpInstrumentation', () => {
 
       before(async () => {
         const config: HttpInstrumentationConfig = {
-          ignoreIncomingPaths: [
-            (url: string) => {
-              throw new Error('bad ignoreIncomingPaths function');
-            },
-          ],
           ignoreIncomingRequestHook: _request => {
             throw new Error('bad ignoreIncomingRequestHook function');
           },
-          ignoreOutgoingUrls: [
-            (url: string) => {
-              throw new Error('bad ignoreOutgoingUrls function');
-            },
-          ],
           ignoreOutgoingRequestHook: _request => {
             throw new Error('bad ignoreOutgoingRequestHook function');
           },
@@ -308,21 +298,11 @@ describe('HttpInstrumentation', () => {
 
       before(async () => {
         instrumentation.setConfig({
-          ignoreIncomingPaths: [
-            '/ignored/string',
-            /\/ignored\/regexp$/i,
-            (url: string) => url.endsWith('/ignored/function'),
-          ],
           ignoreIncomingRequestHook: request => {
             return (
               request.headers['user-agent']?.match('ignored-string') != null
             );
           },
-          ignoreOutgoingUrls: [
-            `${protocol}://${hostname}:${serverPort}/ignored/string`,
-            /\/ignored\/regexp$/i,
-            (url: string) => url.endsWith('/ignored/function'),
-          ],
           ignoreOutgoingRequestHook: request => {
             if (request.headers?.['user-agent'] != null) {
               return (
@@ -592,19 +572,7 @@ describe('HttpInstrumentation', () => {
         });
       });
 
-      for (const ignored of ['string', 'function', 'regexp']) {
-        it(`should not trace ignored requests with paths (client and server side) with type ${ignored}`, async () => {
-          const testPath = `/ignored/${ignored}`;
-
-          await httpRequest.get(
-            `${protocol}://${hostname}:${serverPort}${testPath}`
-          );
-          const spans = memoryExporter.getFinishedSpans();
-          assert.strictEqual(spans.length, 0);
-        });
-      }
-
-      it('should not trace ignored requests with headers (client and server side)', async () => {
+      it('should not trace ignored requests when ignore hook returns true', async () => {
         const testValue = 'ignored-string';
 
         await Promise.all([
@@ -618,7 +586,7 @@ describe('HttpInstrumentation', () => {
         assert.strictEqual(spans.length, 0);
       });
 
-      it('should trace not ignored requests with headers (client and server side)', async () => {
+      it('should trace requests when ignore hook returns false', async () => {
         await httpRequest.get(`${protocol}://${hostname}:${serverPort}`, {
           headers: {
             'user-agent': 'test-bot',

experimental/packages/opentelemetry-instrumentation-http/test/functionals/https-enable.test.ts

@@ -111,19 +111,9 @@ describe('HttpsInstrumentation', () => {
 
       before(() => {
         instrumentation.setConfig({
-          ignoreIncomingPaths: [
-            (url: string) => {
-              throw new Error('bad ignoreIncomingPaths function');
-            },
-          ],
           ignoreIncomingRequestHook: _request => {
             throw new Error('bad ignoreIncomingRequestHook function');
           },
-          ignoreOutgoingUrls: [
-            (url: string) => {
-              throw new Error('bad ignoreOutgoingUrls function');
-            },
-          ],
           ignoreOutgoingRequestHook: _request => {
             throw new Error('bad ignoreOutgoingRequestHook function');
           },
@@ -192,21 +182,11 @@ describe('HttpsInstrumentation', () => {
 
       before(() => {
         instrumentation.setConfig({
-          ignoreIncomingPaths: [
-            '/ignored/string',
-            /\/ignored\/regexp$/i,
-            (url: string) => url.endsWith('/ignored/function'),
-          ],
           ignoreIncomingRequestHook: request => {
             return (
               request.headers['user-agent']?.match('ignored-string') != null
             );
           },
-          ignoreOutgoingUrls: [
-            `${protocol}://${hostname}:${serverPort}/ignored/string`,
-            /\/ignored\/regexp$/i,
-            (url: string) => url.endsWith('/ignored/function'),
-          ],
           ignoreOutgoingRequestHook: request => {
             if (request.headers?.['user-agent'] != null) {
               return (
@@ -441,19 +421,7 @@ describe('HttpsInstrumentation', () => {
         });
       });
 
-      for (const ignored of ['string', 'function', 'regexp']) {
-        it(`should not trace ignored requests with paths (client and server side) with type ${ignored}`, async () => {
-          const testPath = `/ignored/${ignored}`;
-
-          await httpsRequest.get(
-            `${protocol}://${hostname}:${serverPort}${testPath}`
-          );
-          const spans = memoryExporter.getFinishedSpans();
-          assert.strictEqual(spans.length, 0);
-        });
-      }
-
-      it('should not trace ignored requests with headers (client and server side)', async () => {
+      it('should trace requests when ignore hook returns false', async () => {
         const testValue = 'ignored-string';
 
         await Promise.all([

experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts

@@ -149,85 +149,6 @@ describe('Utility', () => {
     });
   });
 
-  describe('isIgnored()', () => {
-    beforeEach(() => {
-      sinon.spy(utils, 'satisfiesPattern');
-    });
-
-    afterEach(() => {
-      sinon.restore();
-    });
-
-    it('should call isSatisfyPattern, n match', () => {
-      const answer1 = utils.isIgnored('/test/1', ['/test/11']);
-      assert.strictEqual(answer1, false);
-      assert.strictEqual(
-        (utils.satisfiesPattern as sinon.SinonSpy).callCount,
-        1
-      );
-    });
-
-    it('should call isSatisfyPattern, match for function', () => {
-      const answer1 = utils.isIgnored('/test/1', [
-        url => url.endsWith('/test/1'),
-      ]);
-      assert.strictEqual(answer1, true);
-    });
-
-    it('should not re-throw when function throws an exception', () => {
-      const onException = (e: unknown) => {
-        // Do nothing
-      };
-      for (const callback of [undefined, onException]) {
-        assert.doesNotThrow(() =>
-          utils.isIgnored(
-            '/test/1',
-            [
-              () => {
-                throw new Error('test');
-              },
-            ],
-            callback
-          )
-        );
-      }
-    });
-
-    it('should call onException when function throws an exception', () => {
-      const onException = sinon.spy();
-      assert.doesNotThrow(() =>
-        utils.isIgnored(
-          '/test/1',
-          [
-            () => {
-              throw new Error('test');
-            },
-          ],
-          onException
-        )
-      );
-      assert.strictEqual((onException as sinon.SinonSpy).callCount, 1);
-    });
-
-    it('should not call isSatisfyPattern', () => {
-      utils.isIgnored('/test/1', []);
-      assert.strictEqual(
-        (utils.satisfiesPattern as sinon.SinonSpy).callCount,
-        0
-      );
-    });
-
-    it('should return false on empty list', () => {
-      const answer1 = utils.isIgnored('/test/1', []);
-      assert.strictEqual(answer1, false);
-    });
-
-    it('should not throw and return false when list is undefined', () => {
-      const answer2 = utils.isIgnored('/test/1', undefined);
-      assert.strictEqual(answer2, false);
-    });
-  });
-
   describe('getAbsoluteUrl()', () => {
     it('should return absolute url with localhost', () => {
       const path = '/test/1';

experimental/packages/opentelemetry-instrumentation-http/test/integrations/http-enable.test.ts

@@ -45,7 +45,6 @@ import { Socket } from 'net';
 import { sendRequestTwice } from '../utils/rawRequest';
 
 const protocol = 'http';
-const serverPort = 32345;
 const hostname = 'localhost';
 const memoryExporter = new InMemorySpanExporter();
 
@@ -138,14 +137,7 @@ describe('HttpInstrumentation Integration tests', () => {
     });
 
     before(() => {
-      const ignoreConfig = [
-        `${protocol}://${hostname}:${serverPort}/ignored/string`,
-        /\/ignored\/regexp$/i,
-        (url: string) => url.endsWith('/ignored/function'),
-      ];
       instrumentation.setConfig({
-        ignoreIncomingPaths: ignoreConfig,
-        ignoreOutgoingUrls: ignoreConfig,
         applyCustomAttributesOnSpan: customAttributeFunction,
       });
       instrumentation.enable();