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

Jump to bottom

chore: avoid leaking type definitions in instrumentations #2256

Closed
wants to merge 4 commits into from
Closed

chore: avoid leaking type definitions in instrumentations #2256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a3678b
chore: avoid leaking type definitions in instrumentations
Flarna Jun 4, 2021
5a51636
Merge branch 'main' into avoid-leaking-types
Flarna Jun 4, 2021
e51132f
Merge branch 'main' into avoid-leaking-types
Flarna Jul 1, 2021
b4b31dc
Merge branch 'main' into avoid-leaking-types
Flarna Jul 4, 2021
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
6 changes: 2 additions & 4 deletions packages/opentelemetry-instrumentation-fetch/src/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,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 All @@ -84,7 +82,7 @@ export class FetchInstrumentation extends InstrumentationBase<
);
}

init() {}
protected init() {}

private _getConfig(): FetchInstrumentationConfig {
return this._config;
Expand Down
3 changes: 2 additions & 1 deletion packages/opentelemetry-instrumentation-grpc/src/grpc-js/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ export class GrpcJsInstrumentation extends InstrumentationBase {
this._config = Object.assign({}, config);
}

init() {
// use InstrumentationNodeModuleDefinition<any>[] to avoid leaking grpc types
protected init(): InstrumentationNodeModuleDefinition<any>[] {
Copy link
Member Author

Choose a reason for hiding this comment

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

actually this is the change really required to avoid leaking grpc types here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And this is where the "generic" (interface) should be used (I may be over simplifying)
protected init(): InstrumentationNodeModuleDefinition<T>[] {

Copy link
Member Author

Choose a reason for hiding this comment

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

init() is called by base class constructor (and this is the only one who should call init) and base class has no need for the types.
But generated type definitions hold full types for public and protected methods therefore this causes that types leak and we have to add the @types packages as production dependencies (see open-telemetry/opentelemetry-js-contrib#460 for more details).

Moving init() to private would solve this also but this is not allowed in typescript (it would be in C++...).

return [
new InstrumentationNodeModuleDefinition<typeof grpcJs>(
'@grpc/grpc-js',
Expand Down
7 changes: 3 additions & 4 deletions packages/opentelemetry-instrumentation-grpc/src/grpc/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@ import { AttributeNames } from '../enums/AttributeNames';
*/
let grpcClient: typeof grpcTypes;

export class GrpcNativeInstrumentation extends InstrumentationBase<
typeof grpcTypes
> {
export class GrpcNativeInstrumentation extends InstrumentationBase {
constructor(
protected override _config: GrpcInstrumentationConfig & InstrumentationConfig = {},
name: string,
Expand All @@ -68,7 +66,8 @@ export class GrpcNativeInstrumentation extends InstrumentationBase<
this._config = Object.assign({}, config);
}

init() {
// use InstrumentationNodeModuleDefinition<any>[] to avoid leaking grpc types
protected init(): InstrumentationNodeModuleDefinition<any>[] {
return [
new InstrumentationNodeModuleDefinition<typeof grpcTypes>(
'grpc',
Expand Down
13 changes: 7 additions & 6 deletions packages/opentelemetry-instrumentation-http/src/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
import { suppressTracing } from '@opentelemetry/core';
import type * as http from 'http';
import type * as https from 'https';
import { Socket } from 'net';
import type { Socket } from 'net';
import * as semver from 'semver';
import * as url from 'url';
import {
Expand All @@ -55,7 +55,7 @@ import { RPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core';
/**
* 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 readonly _version = process.versions.node;
Expand All @@ -76,7 +76,8 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
this._config = Object.assign({}, config);
}

init() {
// use InstrumentationNodeModuleDefinition<any>[] here to avoid leaking http types
protected init(): InstrumentationNodeModuleDefinition<any>[] {
return [this._getHttpsInstrumentation(), this._getHttpInstrumentation()];
}

Expand Down Expand Up @@ -169,7 +170,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
/**
* Creates spans for incoming requests, restoring spans' context if applied.
*/
protected _getPatchIncomingRequestFunction(component: 'http' | 'https') {
private _getPatchIncomingRequestFunction(component: 'http' | 'https') {
return (original: (event: string, ...args: unknown[]) => boolean) => {
return this._incomingRequestFunction(component, original);
};
Expand All @@ -179,13 +180,13 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
* Creates spans for outgoing requests, sending spans' context for distributed
* tracing.
*/
protected _getPatchOutgoingRequestFunction(component: 'http' | 'https') {
private _getPatchOutgoingRequestFunction(component: 'http' | 'https') {
return (original: Func<http.ClientRequest>): Func<http.ClientRequest> => {
return this._outgoingRequestFunction(component, original);
};
}

protected _getPatchOutgoingGetFunction(
private _getPatchOutgoingGetFunction(
clientRequest: (
options: http.RequestOptions | string | url.URL,
...args: HttpRequestArgs
Expand Down
4 changes: 2 additions & 2 deletions 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 @@ -78,7 +78,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 All @@ -97,7 +97,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
);
}

init() {}
protected init() {}

private _getConfig(): XMLHttpRequestInstrumentationConfig {
return this._config;
Expand Down
7 changes: 3 additions & 4 deletions packages/opentelemetry-instrumentation/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ import * as types from './types';
/**
* Base abstract internal class for instrumenting node and web plugins
*/
export abstract class InstrumentationAbstract<T = any>
implements types.Instrumentation {
export abstract class InstrumentationAbstract implements types.Instrumentation {
Copy link
Contributor

Choose a reason for hiding this comment

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

My Preference would be to keep the type so that any user can retain the type safety.

It seems to me that what we should do is (where necessary) to have the instrumentation export a "public" interface, any if the specific instrumentation doesn't want to do that, it can use "any" or "unknown".

Copy link
Member Author

@Flarna Flarna Jun 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The type which was used here had no impact on the concrete instrumentation. The concrete type to use was (and still is) specified in the concrete implementation to instantiate the InstrumentationNodeModuleDefinitions.

Maybe we could keep the <T = any> here but I see no value in it. Just take a look at e.g. xhr instrumenation. It has to specifies a type which is not used anywhere afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

But, for any instrumentation where they want to support their own extensions (properties or functions) this would provide additional type safety and they can choose to still hide the actual internal workings (from type safety / code completion / documentation / exported types) but provide some sort of public API etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, that specifying any doesn't have any advantages for what I said above. As you are basically saying that there is no type safety and do what you want. (and yes I know that is pure JS you can anyway)

Copy link
Member Author

Choose a reason for hiding this comment

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

Users can (and should) still use concrete types when creating a InstrumentationNodeModuleDefinition.
They can even make their class extending InstrumentationAbstract a generic and supply a type if this helps anywhere.

But within the @opentelemetry/instrumentation module the type has no value for type safety to my understanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree the type has Zero benefit for the internal workings, it's purely for consumers.

Copy link
Member

@obecny obecny Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Originally the types were added to keep consistency to what we had previously - plugins had defined type too. Some of the instrumentations already uses mixed types and quite often the last thing you can do is to use any. If that would also help to resolve issue with having to include types for the package in dependencies I would be fully into removing this anyway then.

Copy link
Member

Choose a reason for hiding this comment

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

It seems to me this conversation is resolved no?

protected _config: types.InstrumentationConfig;

private _tracer: Tracer;
Expand Down Expand Up @@ -121,7 +120,7 @@ export abstract class InstrumentationAbstract<T = any>
* methods
*/
protected abstract init():
| InstrumentationModuleDefinition<T>
| InstrumentationModuleDefinition<T>[]
| InstrumentationModuleDefinition<unknown>
| InstrumentationModuleDefinition<unknown>[]
| void;
}
11 changes: 4 additions & 7 deletions packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,9 @@ import { diag } from '@opentelemetry/api';
/**
* Base abstract class for instrumenting node plugins
*/
export abstract class InstrumentationBase<T = any>
extends InstrumentationAbstract
export abstract class InstrumentationBase extends InstrumentationAbstract
implements types.Instrumentation {
private _modules: InstrumentationModuleDefinition<T>[];
private _modules: InstrumentationModuleDefinition<unknown>[];
private _hooks: RequireInTheMiddle.Hooked[] = [];
private _enabled = false;

Expand All @@ -45,7 +44,7 @@ export abstract class InstrumentationBase<T = any>
modules = [modules];
}

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

if (this._modules.length === 0) {
diag.warn(
Expand Down Expand Up @@ -131,9 +130,7 @@ export abstract class InstrumentationBase<T = any>
{ internals: true },
(exports, name, baseDir) => {
return this._onRequire<typeof exports>(
(module as unknown) as InstrumentationModuleDefinition<
typeof exports
>,
module as InstrumentationModuleDefinition<typeof exports>,
exports,
name,
baseDir
Expand Down