Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(instrumentation): generic config type in instrumentation base #4659

Merged
merged 17 commits into from
May 2, 2024
Merged

feat(instrumentation): generic config type in instrumentation base #4659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e6eea0a
feat!(instrumentation): generic config type and no default config value
blumamir Apr 25, 2024
d97f38b
fix: apply type in base Instrumentation interface
blumamir Apr 25, 2024
fdb1fff
Merge remote-tracking branch 'upstream/main' into inst-type-generic
blumamir Apr 25, 2024
192d3eb
revert: enabled flag rename
blumamir Apr 25, 2024
ff3d60d
fix: autoloader types
blumamir Apr 25, 2024
f620570
chore: lint fix
blumamir Apr 25, 2024
e64f9f8
revert: default config in constructor to empty object
blumamir Apr 25, 2024
ed3589b
revert: make constructor config default empty object
blumamir Apr 25, 2024
93f0c55
docs: note that instrumentation config fields are optional
blumamir Apr 25, 2024
fbb948f
revert: deftaul type for generic
blumamir Apr 25, 2024
74b812e
revert: default object in instrumentation abstract constructor
blumamir Apr 25, 2024
14023db
chore: lint fix
blumamir Apr 25, 2024
c5db3f8
chore: changelog
blumamir Apr 25, 2024
16c9d1d
Merge branch 'main' into inst-type-generic
blumamir Apr 29, 2024
4b34418
fix: changelog in merge
blumamir Apr 30, 2024
81fe89c
Merge remote-tracking branch 'origin/inst-type-generic' into inst-typ…
blumamir Apr 30, 2024
a6c9674
Merge branch 'main' into inst-type-generic
blumamir May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This keeps existing behavior, having default value to empty object.
This assignment was typescript incorrect before, as the typed used was InstrumentationConfig which the real config object extends, but can also include required fields.

I choose not to address this issue in the current PR to keep the scope narrow and reviewable.

) {
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