Skip to content

Commit

Permalink
feat(instrumentation): generic config type in instrumentation base (#…
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@blumamir
blumamir authored May 2, 2024
1 parent 753f0a6 commit 157c811
Show file tree
Hide file tree
Showing 11 changed files with 79 additions and 78 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
18 changes: 7 additions & 11 deletions experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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>> = {};
Expand Down Expand Up @@ -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>();
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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),
Expand Down
13 changes: 2 additions & 11 deletions experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}
Expand Down
48 changes: 22 additions & 26 deletions experimental/packages/opentelemetry-instrumentation-http/src/http.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -335,7 +331,7 @@ export class HttpInstrumentation extends InstrumentationBase {
utils.getOutgoingRequestMetricAttributesOnResponse(responseAttributes)
);

if (this._getConfig().responseHook) {
if (this.getConfig().responseHook) {
this._callResponseHook(span, response);
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand All @@ -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
),
});

Expand All @@ -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);
}

Expand Down Expand Up @@ -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) {
Expand All @@ -650,7 +646,7 @@ export class HttpInstrumentation extends InstrumentationBase {
hostname,
hookAttributes: instrumentation._callStartSpanHook(
optionsParsed,
instrumentation._getConfig().startOutgoingSpanHook
instrumentation.getConfig().startOutgoingSpanHook
),
});

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
);
Expand All @@ -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
);
Expand All @@ -857,7 +853,7 @@ export class HttpInstrumentation extends InstrumentationBase {
}

private _createHeaderCapture() {
const config = this._getConfig();
const config = this.getConfig();

return {
client: {
Expand Down
18 changes: 7 additions & 11 deletions experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -96,10 +96,6 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {

init() {}

private _getConfig(): XMLHttpRequestInstrumentationConfig {
return this._config;
}

/**
* Adds custom headers to XMLHttpRequest
* @param xhr
Expand All @@ -111,7 +107,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase {
if (
!shouldPropagateTraceHeaders(
url,
this._getConfig().propagateTraceHeaderCorsUrls
this.getConfig().propagateTraceHeaderCorsUrls
)
) {
const headers: Partial<Record<string, unknown>> = {};
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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>();
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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;
}
Expand Down
15 changes: 10 additions & 5 deletions experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}

Expand Down
Loading

0 comments on commit 157c811

Please sign in to comment.