feat(instrumentation): generic config type in instrumentation base (o…

…pen-telemetry#4659) * feat!(instrumentation): generic config type and no default config value * fix: apply type in base Instrumentation interface * revert: enabled flag rename * fix: autoloader types * chore: lint fix * revert: default config in constructor to empty object * revert: make constructor config default empty object * docs: note that instrumentation config fields are optional * revert: deftaul type for generic * revert: default object in instrumentation abstract constructor * chore: lint fix * chore: changelog * fix: changelog in merge
Zirak · Sep 14, 2024 · bffd4d5 · bffd4d5
1 parent 6b02af0
commit bffd4d5
Show file tree

Hide file tree

Showing 11 changed files with 79 additions and 78 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/
 
 ### :rocket: (Enhancement)
 
+* feat(instrumentation): generic config type in instrumentation base [#4659](https://github.com/open-telemetry/opentelemetry-js/pull/4659) @blumamir
 * feat: support node 22 [#4666](https://github.com/open-telemetry/opentelemetry-js/pull/4666) @dyladan
 
 ### :bug: (Bug Fix)

diff --git a/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts b/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
@@ -72,7 +72,7 @@ export interface FetchInstrumentationConfig extends InstrumentationConfig {
 /**
  * This class represents a fetch plugin for auto instrumentation
  */
-export class FetchInstrumentation extends InstrumentationBase {
+export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentationConfig> {
   readonly component: string = 'fetch';
   readonly version: string = VERSION;
   moduleName = this.component;
@@ -85,10 +85,6 @@ export class FetchInstrumentation extends InstrumentationBase {
 
   init(): void {}
 
-  private _getConfig(): FetchInstrumentationConfig {
-    return this._config;
-  }
-
   /**
    * Add cors pre flight child span
    * @param span
@@ -105,7 +101,7 @@ export class FetchInstrumentation extends InstrumentationBase {
       },
       api.trace.setSpan(api.context.active(), span)
     );
-    if (!this._getConfig().ignoreNetworkEvents) {
+    if (!this.getConfig().ignoreNetworkEvents) {
       web.addSpanNetworkEvents(childSpan, corsPreFlightRequest);
     }
     childSpan.end(
@@ -149,7 +145,7 @@ export class FetchInstrumentation extends InstrumentationBase {
     if (
       !web.shouldPropagateTraceHeaders(
         spanUrl,
-        this._getConfig().propagateTraceHeaderCorsUrls
+        this.getConfig().propagateTraceHeaderCorsUrls
       )
     ) {
       const headers: Partial<Record<string, unknown>> = {};
@@ -186,7 +182,7 @@ export class FetchInstrumentation extends InstrumentationBase {
    * @private
    */
   private _clearResources() {
-    if (this._tasksCount === 0 && this._getConfig().clearTimingResources) {
+    if (this._tasksCount === 0 && this.getConfig().clearTimingResources) {
       performance.clearResourceTimings();
       this._usedResources = new WeakSet<PerformanceResourceTiming>();
     }
@@ -201,7 +197,7 @@ export class FetchInstrumentation extends InstrumentationBase {
     url: string,
     options: Partial<Request | RequestInit> = {}
   ): api.Span | undefined {
-    if (core.isUrlIgnored(url, this._getConfig().ignoreUrls)) {
+    if (core.isUrlIgnored(url, this.getConfig().ignoreUrls)) {
       this._diag.debug('ignoring span as url matches ignored url');
       return;
     }
@@ -258,7 +254,7 @@ export class FetchInstrumentation extends InstrumentationBase {
         this._addChildSpan(span, corsPreFlightRequest);
         this._markResourceAsUsed(corsPreFlightRequest);
       }
-      if (!this._getConfig().ignoreNetworkEvents) {
+      if (!this.getConfig().ignoreNetworkEvents) {
         web.addSpanNetworkEvents(span, mainRequest);
       }
     }
@@ -419,7 +415,7 @@ export class FetchInstrumentation extends InstrumentationBase {
     result: Response | FetchError
   ) {
     const applyCustomAttributesOnSpan =
-      this._getConfig().applyCustomAttributesOnSpan;
+      this.getConfig().applyCustomAttributesOnSpan;
     if (applyCustomAttributesOnSpan) {
       safeExecuteInTheMiddle(
         () => applyCustomAttributesOnSpan(span, request, result),

diff --git a/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts
@@ -88,7 +88,7 @@ import {
 import { AttributeValues } from './enums/AttributeValues';
 import { VERSION } from './version';
 
-export class GrpcInstrumentation extends InstrumentationBase {
+export class GrpcInstrumentation extends InstrumentationBase<GrpcInstrumentationConfig> {
   private _metadataCapture: metadataCaptureType;
 
   constructor(config?: GrpcInstrumentationConfig) {
@@ -195,16 +195,7 @@ export class GrpcInstrumentation extends InstrumentationBase {
     ];
   }
 
-  /**
-   * @internal
-   * Public reference to the protected BaseInstrumentation `_config` instance to be used by this
-   * plugin's external helper functions
-   */
-  override getConfig(): GrpcInstrumentationConfig {
-    return super.getConfig();
-  }
-
-  override setConfig(config?: GrpcInstrumentationConfig): void {
+  override setConfig(config: GrpcInstrumentationConfig = {}): void {
     super.setConfig(config);
     this._metadataCapture = this._createMetadataCapture();
   }

diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
@@ -63,7 +63,7 @@ import { SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions';
 /**
  * Http instrumentation instrumentation for Opentelemetry
  */
-export class HttpInstrumentation extends InstrumentationBase {
+export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentationConfig> {
   /** keep track on spans not ended */
   private readonly _spanNotEnded: WeakSet<Span> = new WeakSet<Span>();
   private _headerCapture;
@@ -94,11 +94,7 @@ export class HttpInstrumentation extends InstrumentationBase {
     );
   }
 
-  private _getConfig(): HttpInstrumentationConfig {
-    return this._config;
-  }
-
-  override setConfig(config?: HttpInstrumentationConfig): void {
+  override setConfig(config: HttpInstrumentationConfig = {}): void {
     super.setConfig(config);
     this._headerCapture = this._createHeaderCapture();
   }
@@ -306,7 +302,7 @@ export class HttpInstrumentation extends InstrumentationBase {
     startTime: HrTime,
     metricAttributes: MetricAttributes
   ): http.ClientRequest {
-    if (this._getConfig().requestHook) {
+    if (this.getConfig().requestHook) {
       this._callRequestHook(span, request);
     }
 
@@ -335,7 +331,7 @@ export class HttpInstrumentation extends InstrumentationBase {
           utils.getOutgoingRequestMetricAttributesOnResponse(responseAttributes)
         );
 
-        if (this._getConfig().responseHook) {
+        if (this.getConfig().responseHook) {
           this._callResponseHook(span, response);
         }
 
@@ -370,10 +366,10 @@ export class HttpInstrumentation extends InstrumentationBase {
 
           span.setStatus(status);
 
-          if (this._getConfig().applyCustomAttributesOnSpan) {
+          if (this.getConfig().applyCustomAttributesOnSpan) {
             safeExecuteInTheMiddle(
               () =>
-                this._getConfig().applyCustomAttributesOnSpan!(
+                this.getConfig().applyCustomAttributesOnSpan!(
                   span,
                   request,
                   response
@@ -467,13 +463,13 @@ export class HttpInstrumentation extends InstrumentationBase {
       if (
         utils.isIgnored(
           pathname,
-          instrumentation._getConfig().ignoreIncomingPaths,
+          instrumentation.getConfig().ignoreIncomingPaths,
           (e: unknown) =>
             instrumentation._diag.error('caught ignoreIncomingPaths error: ', e)
         ) ||
         safeExecuteInTheMiddle(
           () =>
-            instrumentation._getConfig().ignoreIncomingRequestHook?.(request),
+            instrumentation.getConfig().ignoreIncomingRequestHook?.(request),
           (e: unknown) => {
             if (e != null) {
               instrumentation._diag.error(
@@ -496,10 +492,10 @@ export class HttpInstrumentation extends InstrumentationBase {
 
       const spanAttributes = utils.getIncomingRequestAttributes(request, {
         component: component,
-        serverName: instrumentation._getConfig().serverName,
+        serverName: instrumentation.getConfig().serverName,
         hookAttributes: instrumentation._callStartSpanHook(
           request,
-          instrumentation._getConfig().startIncomingSpanHook
+          instrumentation.getConfig().startIncomingSpanHook
         ),
       });
 
@@ -525,10 +521,10 @@ export class HttpInstrumentation extends InstrumentationBase {
           context.bind(context.active(), request);
           context.bind(context.active(), response);
 
-          if (instrumentation._getConfig().requestHook) {
+          if (instrumentation.getConfig().requestHook) {
             instrumentation._callRequestHook(span, request);
           }
-          if (instrumentation._getConfig().responseHook) {
+          if (instrumentation.getConfig().responseHook) {
             instrumentation._callResponseHook(span, response);
           }
 
@@ -619,14 +615,14 @@ export class HttpInstrumentation extends InstrumentationBase {
       if (
         utils.isIgnored(
           origin + pathname,
-          instrumentation._getConfig().ignoreOutgoingUrls,
+          instrumentation.getConfig().ignoreOutgoingUrls,
           (e: unknown) =>
             instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e)
         ) ||
         safeExecuteInTheMiddle(
           () =>
             instrumentation
-              ._getConfig()
+              .getConfig()
               .ignoreOutgoingRequestHook?.(optionsParsed),
           (e: unknown) => {
             if (e != null) {
@@ -650,7 +646,7 @@ export class HttpInstrumentation extends InstrumentationBase {
         hostname,
         hookAttributes: instrumentation._callStartSpanHook(
           optionsParsed,
-          instrumentation._getConfig().startOutgoingSpanHook
+          instrumentation.getConfig().startOutgoingSpanHook
         ),
       });
 
@@ -745,10 +741,10 @@ export class HttpInstrumentation extends InstrumentationBase {
       span.updateName(`${request.method || 'GET'} ${route}`);
     }
 
-    if (this._getConfig().applyCustomAttributesOnSpan) {
+    if (this.getConfig().applyCustomAttributesOnSpan) {
       safeExecuteInTheMiddle(
         () =>
-          this._getConfig().applyCustomAttributesOnSpan!(
+          this.getConfig().applyCustomAttributesOnSpan!(
             span,
             request,
             response
@@ -782,8 +778,8 @@ export class HttpInstrumentation extends InstrumentationBase {
      */
     const requireParent =
       options.kind === SpanKind.CLIENT
-        ? this._getConfig().requireParentforOutgoingSpans
-        : this._getConfig().requireParentforIncomingSpans;
+        ? this.getConfig().requireParentforOutgoingSpans
+        : this.getConfig().requireParentforIncomingSpans;
 
     let span: Span;
     const currentSpan = trace.getSpan(ctx);
@@ -826,7 +822,7 @@ export class HttpInstrumentation extends InstrumentationBase {
     response: http.IncomingMessage | http.ServerResponse
   ) {
     safeExecuteInTheMiddle(
-      () => this._getConfig().responseHook!(span, response),
+      () => this.getConfig().responseHook!(span, response),
       () => {},
       true
     );
@@ -837,7 +833,7 @@ export class HttpInstrumentation extends InstrumentationBase {
     request: http.ClientRequest | http.IncomingMessage
   ) {
     safeExecuteInTheMiddle(
-      () => this._getConfig().requestHook!(span, request),
+      () => this.getConfig().requestHook!(span, request),
       () => {},
       true
     );
@@ -857,7 +853,7 @@ export class HttpInstrumentation extends InstrumentationBase {
   }
 
   private _createHeaderCapture() {
-    const config = this._getConfig();
+    const config = this.getConfig();
 
     return {
       client: {

diff --git a/experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts b/experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts
@@ -81,7 +81,7 @@ export interface XMLHttpRequestInstrumentationConfig
 /**
  * This class represents a XMLHttpRequest plugin for auto instrumentation
  */
-export class XMLHttpRequestInstrumentation extends InstrumentationBase {
+export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRequestInstrumentationConfig> {
   readonly component: string = 'xml-http-request';
   readonly version: string = VERSION;
   moduleName = this.component;
@@ -96,10 +96,6 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {
 
   init() {}
 
-  private _getConfig(): XMLHttpRequestInstrumentationConfig {
-    return this._config;
-  }
-
   /**
    * Adds custom headers to XMLHttpRequest
    * @param xhr
@@ -111,7 +107,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {
     if (
       !shouldPropagateTraceHeaders(
         url,
-        this._getConfig().propagateTraceHeaderCorsUrls
+        this.getConfig().propagateTraceHeaderCorsUrls
       )
     ) {
       const headers: Partial<Record<string, unknown>> = {};
@@ -142,7 +138,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {
       const childSpan = this.tracer.startSpan('CORS Preflight', {
         startTime: corsPreFlightRequest[PTN.FETCH_START],
       });
-      if (!this._getConfig().ignoreNetworkEvents) {
+      if (!this.getConfig().ignoreNetworkEvents) {
         addSpanNetworkEvents(childSpan, corsPreFlightRequest);
       }
       childSpan.end(corsPreFlightRequest[PTN.RESPONSE_END]);
@@ -182,7 +178,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {
 
   private _applyAttributesAfterXHR(span: api.Span, xhr: XMLHttpRequest) {
     const applyCustomAttributesOnSpan =
-      this._getConfig().applyCustomAttributesOnSpan;
+      this.getConfig().applyCustomAttributesOnSpan;
     if (typeof applyCustomAttributesOnSpan === 'function') {
       safeExecuteInTheMiddle(
         () => applyCustomAttributesOnSpan(span, xhr),
@@ -244,7 +240,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {
    * @private
    */
   private _clearResources() {
-    if (this._tasksCount === 0 && this._getConfig().clearTimingResources) {
+    if (this._tasksCount === 0 && this.getConfig().clearTimingResources) {
       (otperformance as unknown as Performance).clearResourceTimings();
       this._xhrMem = new WeakMap<XMLHttpRequest, XhrMem>();
       this._usedResources = new WeakSet<PerformanceResourceTiming>();
@@ -296,7 +292,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {
         this._addChildSpan(span, corsPreFlightRequest);
         this._markResourceAsUsed(corsPreFlightRequest);
       }
-      if (!this._getConfig().ignoreNetworkEvents) {
+      if (!this.getConfig().ignoreNetworkEvents) {
         addSpanNetworkEvents(span, mainRequest);
       }
     }
@@ -331,7 +327,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {
     url: string,
     method: string
   ): api.Span | undefined {
-    if (isUrlIgnored(url, this._getConfig().ignoreUrls)) {
+    if (isUrlIgnored(url, this.getConfig().ignoreUrls)) {
       this._diag.debug('ignoring span as url matches ignored url');
       return;
     }

diff --git a/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts
@@ -35,8 +35,11 @@ import {
 /**
  * Base abstract internal class for instrumenting node and web plugins
  */
-export abstract class InstrumentationAbstract implements Instrumentation {
-  protected _config: InstrumentationConfig;
+export abstract class InstrumentationAbstract<
+  ConfigType extends InstrumentationConfig = InstrumentationConfig,
+> implements Instrumentation<ConfigType>
+{
+  protected _config: ConfigType;
 
   private _tracer: Tracer;
   private _meter: Meter;
@@ -46,7 +49,7 @@ export abstract class InstrumentationAbstract implements Instrumentation {
   constructor(
     public readonly instrumentationName: string,
     public readonly instrumentationVersion: string,
-    config: InstrumentationConfig = {}
+    config: ConfigType = {} as ConfigType // assuming ConfigType is an object with optional fields only
   ) {
     this._config = {
       enabled: true,
@@ -131,15 +134,17 @@ export abstract class InstrumentationAbstract implements Instrumentation {
   }
 
   /* Returns InstrumentationConfig */
-  public getConfig(): InstrumentationConfig {
+  public getConfig(): ConfigType {
     return this._config;
   }
 
   /**
    * Sets InstrumentationConfig to this plugin
    * @param InstrumentationConfig
    */
-  public setConfig(config: InstrumentationConfig = {}): void {
+  public setConfig(config: ConfigType = {} as ConfigType): void {
+    // the assertion that {} is compatible with ConfigType may not be correct,
+    // ConfigType should contain only optional fields, but there is no enforcement in place for that
     this._config = Object.assign({}, config);
   }