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.

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
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
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
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
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
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