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): remove moduleExports generic type from instrumentation registration #4598

Merged
merged 18 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :boom: Breaking Change

* feat!(instrumentation): remove moduleExports generic type from instrumentation registration [#4598](https://github.com/open-telemetry/opentelemetry-js/pull/4598) @blumamir
* breaking for instrumentation authors that depend on
* `InstrumentationBase`
* `InstrumentationNodeModuleDefinition`
* `InstrumentationNodeModuleFile`

### :rocket: (Enhancement)

* feat(sdk-trace-base): log resource attributes in ConsoleSpanExporter [#4605](https://github.com/open-telemetry/opentelemetry-js/pull/4605) @pichlermarc
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ export interface FetchInstrumentationConfig extends InstrumentationConfig {
/**
* This class represents a fetch plugin for auto instrumentation
*/
export class FetchInstrumentation extends InstrumentationBase<
Promise<Response>
> {
export class FetchInstrumentation extends InstrumentationBase {
readonly component: string = 'fetch';
readonly version: string = VERSION;
moduleName = this.component;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export class GrpcInstrumentation extends InstrumentationBase {

init() {
return [
new InstrumentationNodeModuleDefinition<any>(
new InstrumentationNodeModuleDefinition(
'@grpc/grpc-js',
['1.*'],
(moduleExports, version) => {
Expand Down
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<Http> {
export class HttpInstrumentation extends InstrumentationBase {
/** keep track on spans not ended */
private readonly _spanNotEnded: WeakSet<Span> = new WeakSet<Span>();
private _headerCapture;
Expand Down Expand Up @@ -104,18 +104,18 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
}

init(): [
InstrumentationNodeModuleDefinition<Https>,
InstrumentationNodeModuleDefinition<Http>,
InstrumentationNodeModuleDefinition,
InstrumentationNodeModuleDefinition,
] {
return [this._getHttpsInstrumentation(), this._getHttpInstrumentation()];
}

private _getHttpInstrumentation() {
const version = process.versions.node;
return new InstrumentationNodeModuleDefinition<Http>(
return new InstrumentationNodeModuleDefinition(
'http',
['*'],
moduleExports => {
(moduleExports: Http): Http => {
this._diag.debug(`Applying patch for http@${version}`);
if (isWrapped(moduleExports.request)) {
this._unwrap(moduleExports, 'request');
Expand Down Expand Up @@ -143,7 +143,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
);
return moduleExports;
},
moduleExports => {
(moduleExports: Http) => {
if (moduleExports === undefined) return;
this._diag.debug(`Removing patch for http@${version}`);

Expand All @@ -156,10 +156,10 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {

private _getHttpsInstrumentation() {
const version = process.versions.node;
return new InstrumentationNodeModuleDefinition<Https>(
return new InstrumentationNodeModuleDefinition(
'https',
['*'],
moduleExports => {
(moduleExports: Https): Https => {
this._diag.debug(`Applying patch for https@${version}`);
if (isWrapped(moduleExports.request)) {
this._unwrap(moduleExports, 'request');
Expand Down Expand Up @@ -187,7 +187,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
);
return moduleExports;
},
moduleExports => {
(moduleExports: Https) => {
if (moduleExports === undefined) return;
this._diag.debug(`Removing patch for https@${version}`);

Expand Down
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<XMLHttpRequest> {
export class XMLHttpRequestInstrumentation extends InstrumentationBase {
readonly component: string = 'xml-http-request';
readonly version: string = VERSION;
moduleName = this.component;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export class MyInstrumentation extends InstrumentationBase {
* the plugin should patch multiple modules or versions.
*/
protected init() {
const module = new InstrumentationNodeModuleDefinition<typeof module_name_to_be_patched>(
const module = new InstrumentationNodeModuleDefinition(
'module_name_to_be_patched',
['1.*'],
this._onPatchMain,
Expand All @@ -63,8 +63,8 @@ export class MyInstrumentation extends InstrumentationBase {
this._unwrap(moduleExports, 'mainMethodName');
}

private _addPatchingMethod(): InstrumentationNodeModuleFile<typeof module_name_to_be_patched> {
const file = new InstrumentationNodeModuleFile<typeof module_name_to_be_patched>(
private _addPatchingMethod(): InstrumentationNodeModuleFile {
const file = new InstrumentationNodeModuleFile(
'module_name_to_be_patched/src/some_file.js',
this._onPatchMethodName,
this._onUnPatchMethodName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ import {
/**
* Base abstract internal class for instrumenting node and web plugins
*/
export abstract class InstrumentationAbstract<T = any>
implements Instrumentation
{
export abstract class InstrumentationAbstract implements Instrumentation {
protected _config: InstrumentationConfig;

private _tracer: Tracer;
Expand Down Expand Up @@ -116,7 +114,7 @@ export abstract class InstrumentationAbstract<T = any>
*
* @returns an array of {@link InstrumentationModuleDefinition}
*/
public getModuleDefinitions(): InstrumentationModuleDefinition<T>[] {
public getModuleDefinitions(): InstrumentationModuleDefinition[] {
const initResult = this.init() ?? [];
if (!Array.isArray(initResult)) {
return [initResult];
Expand Down Expand Up @@ -172,7 +170,7 @@ export abstract class InstrumentationAbstract<T = any>
* methods.
*/
protected abstract init():
| InstrumentationModuleDefinition<T>
| InstrumentationModuleDefinition<T>[]
| InstrumentationModuleDefinition
| InstrumentationModuleDefinition[]
| void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,18 @@ import {
InstrumentationModuleFile,
} from './types';

export class InstrumentationNodeModuleDefinition<T>
implements InstrumentationModuleDefinition<T>
export class InstrumentationNodeModuleDefinition
implements InstrumentationModuleDefinition
{
files: InstrumentationModuleFile<T>[];
files: InstrumentationModuleFile[];
constructor(
public name: string,
public supportedVersions: string[],
public patch?: (exports: T, moduleVersion?: string) => T,
public unpatch?: (exports: T, moduleVersion?: string) => void,
files?: InstrumentationModuleFile<any>[]
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public patch?: (exports: any, moduleVersion?: string) => any,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public unpatch?: (exports: any, moduleVersion?: string) => void,
files?: InstrumentationModuleFile[]
) {
this.files = files || [];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,17 @@
import { InstrumentationModuleFile } from './types';
import { normalize } from './platform/index';

export class InstrumentationNodeModuleFile<T>
implements InstrumentationModuleFile<T>
export class InstrumentationNodeModuleFile
implements InstrumentationModuleFile
{
public name: string;
constructor(
name: string,
public supportedVersions: string[],
public patch: (moduleExports: T, moduleVersion?: string) => T,
public unpatch: (moduleExports?: T, moduleVersion?: string) => void
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public patch: (moduleExports: any, moduleVersion?: string) => any,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public unpatch: (moduleExports?: any, moduleVersion?: string) => void
) {
this.name = normalize(name);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ import { readFileSync } from 'fs';
/**
* Base abstract class for instrumenting node plugins
*/
export abstract class InstrumentationBase<T = any>
export abstract class InstrumentationBase
extends InstrumentationAbstract
implements types.Instrumentation
{
private _modules: InstrumentationModuleDefinition<T>[];
private _modules: InstrumentationModuleDefinition[];
private _hooks: (Hooked | Hook)[] = [];
private _requireInTheMiddleSingleton: RequireInTheMiddleSingleton =
RequireInTheMiddleSingleton.getInstance();
Expand All @@ -58,7 +58,7 @@ export abstract class InstrumentationBase<T = any>
modules = [modules];
}

this._modules = (modules as InstrumentationModuleDefinition<T>[]) || [];
this._modules = (modules as InstrumentationModuleDefinition[]) || [];

if (this._modules.length === 0) {
diag.debug(
Expand Down Expand Up @@ -143,7 +143,7 @@ export abstract class InstrumentationBase<T = any>
};

private _warnOnPreloadedModules(): void {
this._modules.forEach((module: InstrumentationModuleDefinition<T>) => {
this._modules.forEach((module: InstrumentationModuleDefinition) => {
const { name } = module;
try {
const resolvedModule = require.resolve(name);
Expand Down Expand Up @@ -174,7 +174,7 @@ export abstract class InstrumentationBase<T = any>
}

private _onRequire<T>(
module: InstrumentationModuleDefinition<T>,
module: InstrumentationModuleDefinition,
exports: T,
name: string,
baseDir?: string | void
Expand Down Expand Up @@ -216,7 +216,8 @@ export abstract class InstrumentationBase<T = any>
return supportedFileInstrumentations.reduce<T>((patchedExports, file) => {
file.moduleExports = patchedExports;
if (this._enabled) {
return file.patch(patchedExports, module.moduleVersion);
// patch signature is not typed, so we cast it assuming it's correct
return file.patch(patchedExports, module.moduleVersion) as T;
}
return patchedExports;
}, exports);
Expand Down Expand Up @@ -246,20 +247,10 @@ export abstract class InstrumentationBase<T = any>
this._warnOnPreloadedModules();
for (const module of this._modules) {
const hookFn: HookFn = (exports, name, baseDir) => {
return this._onRequire<typeof exports>(
module as unknown as InstrumentationModuleDefinition<typeof exports>,
exports,
name,
baseDir
);
return this._onRequire<typeof exports>(module, exports, name, baseDir);
};
const onRequire: OnRequireFn = (exports, name, baseDir) => {
return this._onRequire<typeof exports>(
module as unknown as InstrumentationModuleDefinition<typeof exports>,
exports,
name,
baseDir
);
return this._onRequire<typeof exports>(module, exports, name, baseDir);
Copy link
Member

Choose a reason for hiding this comment

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

This now looks so much cleaner and easier to reason about!

};

// `RequireInTheMiddleSingleton` does not support absolute paths.
Expand Down
22 changes: 12 additions & 10 deletions experimental/packages/opentelemetry-instrumentation/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,29 +82,30 @@ export interface ShimWrapped extends Function {
__original: Function;
}

export interface InstrumentationModuleFile<T> {
export interface InstrumentationModuleFile {
/** Name of file to be patched with relative path */
name: string;

moduleExports?: T;
moduleExports?: unknown;

/** Supported version this file */
supportedVersions: string[];

/** Method to patch the instrumentation */
patch(moduleExports: T, moduleVersion?: string): T;
patch(moduleExports: unknown, moduleVersion?: string): unknown;

/** Method to patch the instrumentation */

/** Method to unpatch the instrumentation */
unpatch(moduleExports?: T, moduleVersion?: string): void;
unpatch(moduleExports?: unknown, moduleVersion?: string): void;
}

export interface InstrumentationModuleDefinition<T> {
export interface InstrumentationModuleDefinition {
/** Module name or path */
name: string;

moduleExports?: T;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
moduleExports?: any;
blumamir marked this conversation as resolved.
Show resolved Hide resolved

/** Instrumented module version */
moduleVersion?: string;
Expand All @@ -113,15 +114,16 @@ export interface InstrumentationModuleDefinition<T> {
supportedVersions: string[];

/** Module internal files to be patched */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
files: InstrumentationModuleFile<any>[];
files: InstrumentationModuleFile[];
blumamir marked this conversation as resolved.
Show resolved Hide resolved

/** If set to true, the includePrerelease check will be included when calling semver.satisfies */
includePrerelease?: boolean;

/** Method to patch the instrumentation */
patch?: (moduleExports: T, moduleVersion?: string) => T;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
patch?: (moduleExports: any, moduleVersion?: string) => any;
blumamir marked this conversation as resolved.
Show resolved Hide resolved

/** Method to unpatch the instrumentation */
unpatch?: (moduleExports: T, moduleVersion?: string) => void;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
unpatch?: (moduleExports: any, moduleVersion?: string) => void;
blumamir marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ describe('BaseInstrumentation', () => {
});

describe('getModuleDefinitions', () => {
const moduleDefinition: InstrumentationModuleDefinition<unknown> = {
const moduleDefinition: InstrumentationModuleDefinition = {
name: 'foo',
patch: moduleExports => {},
unpatch: moduleExports => {},
Expand Down