From 157c8112742ecc15bb818914eb21325380dfd38a Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Thu, 2 May 2024 08:21:18 +0300 Subject: [PATCH] feat(instrumentation): generic config type in instrumentation base (#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 --- CHANGELOG.md | 1 + .../src/fetch.ts | 18 +++---- .../src/instrumentation.ts | 13 +---- .../src/http.ts | 48 +++++++++---------- .../src/xhr.ts | 18 +++---- .../src/instrumentation.ts | 15 ++++-- .../src/platform/browser/instrumentation.ts | 11 +++-- .../src/platform/node/instrumentation.ts | 15 ++++-- .../src/types.ts | 14 ++++-- .../test/common/autoLoaderUtils.test.ts | 2 +- .../test/node/InstrumentationBase.test.ts | 2 +- 11 files changed, 79 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f8a273db2..c180e5f4c3 100644 --- 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 index edf6f50cf7..d459d3884f 100644 --- 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 { 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> = {}; @@ -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(); } @@ -201,7 +197,7 @@ export class FetchInstrumentation extends InstrumentationBase { url: string, options: Partial = {} ): 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 index 504fb4fd5f..ae0ac8f7a4 100644 --- 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 { 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 index c13736e4da..7b6e91bf2d 100644 --- 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 { /** keep track on spans not ended */ private readonly _spanNotEnded: WeakSet = new WeakSet(); 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 index 4d1e11df70..57dbe79744 100644 --- 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 { 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> = {}; @@ -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(); this._usedResources = new WeakSet(); @@ -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 index 851b38c231..bd4a280fc8 100644 --- 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 +{ + 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,7 +134,7 @@ export abstract class InstrumentationAbstract implements Instrumentation { } /* Returns InstrumentationConfig */ - public getConfig(): InstrumentationConfig { + public getConfig(): ConfigType { return this._config; } @@ -139,7 +142,9 @@ export abstract class InstrumentationAbstract implements Instrumentation { * 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); } diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/browser/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/browser/instrumentation.ts index 0458357a73..46f3b2cc72 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/browser/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/browser/instrumentation.ts @@ -16,18 +16,21 @@ import { InstrumentationAbstract } from '../../instrumentation'; import * as types from '../../types'; +import { InstrumentationConfig } from '../../types'; /** * Base abstract class for instrumenting web plugins */ -export abstract class InstrumentationBase - extends InstrumentationAbstract - implements types.Instrumentation +export abstract class InstrumentationBase< + ConfigType extends InstrumentationConfig = InstrumentationConfig, + > + extends InstrumentationAbstract + implements types.Instrumentation { constructor( instrumentationName: string, instrumentationVersion: string, - config: types.InstrumentationConfig = {} + config: ConfigType = {} as ConfigType // The cast here may be wrong as ConfigType may contain required fields ) { super(instrumentationName, instrumentationVersion, config); diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 095e6339ae..674b1b963f 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -26,7 +26,10 @@ import { } from './RequireInTheMiddleSingleton'; import type { HookFn } from 'import-in-the-middle'; import * as ImportInTheMiddle from 'import-in-the-middle'; -import { InstrumentationModuleDefinition } from '../../types'; +import { + InstrumentationConfig, + InstrumentationModuleDefinition, +} from '../../types'; import { diag } from '@opentelemetry/api'; import type { OnRequireFn } from 'require-in-the-middle'; import { Hook } from 'require-in-the-middle'; @@ -35,9 +38,11 @@ import { readFileSync } from 'fs'; /** * Base abstract class for instrumenting node plugins */ -export abstract class InstrumentationBase - extends InstrumentationAbstract - implements types.Instrumentation +export abstract class InstrumentationBase< + ConfigType extends InstrumentationConfig = InstrumentationConfig, + > + extends InstrumentationAbstract + implements types.Instrumentation { private _modules: InstrumentationModuleDefinition[]; private _hooks: (Hooked | Hook)[] = []; @@ -48,7 +53,7 @@ export abstract class InstrumentationBase constructor( instrumentationName: string, instrumentationVersion: string, - config: types.InstrumentationConfig = {} + config: ConfigType = {} as ConfigType // The cast here may be wrong as ConfigType may contain required fields ) { super(instrumentationName, instrumentationVersion, config); diff --git a/experimental/packages/opentelemetry-instrumentation/src/types.ts b/experimental/packages/opentelemetry-instrumentation/src/types.ts index 680cffce89..5e74b06b5a 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/types.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/types.ts @@ -18,7 +18,9 @@ import { TracerProvider, MeterProvider } from '@opentelemetry/api'; import { LoggerProvider } from '@opentelemetry/api-logs'; /** Interface Instrumentation to apply patch. */ -export interface Instrumentation { +export interface Instrumentation< + ConfigType extends InstrumentationConfig = InstrumentationConfig, +> { /** Instrumentation Name */ instrumentationName: string; @@ -48,10 +50,10 @@ export interface Instrumentation { setLoggerProvider?(loggerProvider: LoggerProvider): void; /** Method to set instrumentation config */ - setConfig(config: InstrumentationConfig): void; + setConfig(config: ConfigType): void; /** Method to get instrumentation config */ - getConfig(): InstrumentationConfig; + getConfig(): ConfigType; /** * Contains all supported versions. @@ -62,6 +64,12 @@ export interface Instrumentation { supportedVersions?: string[]; } +/** + * Base interface for configuration options common to all instrumentations. + * This interface can be extended by individual instrumentations to include + * additional configuration options specific to that instrumentation. + * All configuration options must be optional. + */ export interface InstrumentationConfig { /** * Whether to enable the plugin. diff --git a/experimental/packages/opentelemetry-instrumentation/test/common/autoLoaderUtils.test.ts b/experimental/packages/opentelemetry-instrumentation/test/common/autoLoaderUtils.test.ts index 2477225004..cdf7e5fd9c 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/common/autoLoaderUtils.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/common/autoLoaderUtils.test.ts @@ -38,7 +38,7 @@ describe('autoLoaderUtils', () => { describe('parseInstrumentationOptions', () => { it('should create a new instrumentation from class', () => { const { instrumentations } = parseInstrumentationOptions([ - FooInstrumentation, + FooInstrumentation as typeof InstrumentationBase, ]); assert.strictEqual(instrumentations.length, 1); const instrumentation = instrumentations[0]; diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts index a26d2c818a..0a36f4e277 100644 --- a/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts +++ b/experimental/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts @@ -33,7 +33,7 @@ const CORE_MODULE = 'random_core'; class TestInstrumentation extends InstrumentationBase { constructor() { - super(MODULE_NAME, MODULE_VERSION); + super(MODULE_NAME, MODULE_VERSION, {}); } init() {}