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

chore: cleanup setting config in instrumentations #2522

Merged
merged 7 commits into from
Oct 27, 2021
Merged

chore: cleanup setting config in instrumentations #2522

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac222ee
chore: cleanup setting config in instrumentations
Flarna Oct 6, 2021
44e0828
Merge branch 'main' into cleanup-instrumentations
vmarchaud Oct 10, 2021
03354fc
Merge branch 'main' into cleanup-instrumentations
Flarna Oct 13, 2021
4699183
Merge branch 'main' into cleanup-instrumentations
Flarna Oct 20, 2021
50eaf18
Merge branch 'main' into cleanup-instrumentations
Flarna Oct 23, 2021
21cb795
Merge branch 'main' into cleanup-instrumentations
dyladan Oct 26, 2021
1e19790
Merge branch 'main' into cleanup-instrumentations
vmarchaud Oct 27, 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
4 changes: 2 additions & 2 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 @@ -76,11 +76,11 @@ export class FetchInstrumentation extends InstrumentationBase<
private _usedResources = new WeakSet<PerformanceResourceTiming>();
private _tasksCount = 0;

constructor(config: FetchInstrumentationConfig = {}) {
constructor(config?: FetchInstrumentationConfig) {
super(
'@opentelemetry/instrumentation-fetch',
VERSION,
Object.assign({}, config)
config
);
}

Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-instrumentation-grpc/src/grpc-js/clientUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export function getMethodsToWrap(

// For a method defined in .proto as "UnaryMethod"
Object.entries(methods).forEach(([name, { originalName }]) => {
if (!_methodIsIgnored(name, this._config.ignoreGrpcMethods)) {
if (!_methodIsIgnored(name, this.getConfig().ignoreGrpcMethods)) {
methodList.push(name); // adds camel case method name: "unaryMethod"
if (
originalName &&
Expand Down
23 changes: 9 additions & 14 deletions experimental/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 @@ -19,10 +19,7 @@ import {
InstrumentationNodeModuleDefinition,
isWrapped,
} from '@opentelemetry/instrumentation';
import {
InstrumentationBase,
InstrumentationConfig,
} from '@opentelemetry/instrumentation';
import { InstrumentationBase } from '@opentelemetry/instrumentation';
import { GrpcInstrumentationConfig } from '../types';
import {
ServerCallWithMeta,
Expand Down Expand Up @@ -56,17 +53,11 @@ import { AttributeNames } from '../enums/AttributeNames';

export class GrpcJsInstrumentation extends InstrumentationBase {
constructor(
protected override _config: GrpcInstrumentationConfig & InstrumentationConfig = {},
name: string,
version: string
version: string,
config?: GrpcInstrumentationConfig,
) {
super(name, version, _config);
}

public override setConfig(
config: GrpcInstrumentationConfig & InstrumentationConfig = {}
) {
this._config = Object.assign({}, config);
super(name, version, config);
}

init() {
Expand Down Expand Up @@ -125,6 +116,10 @@ export class GrpcJsInstrumentation extends InstrumentationBase {
];
}

override getConfig(): GrpcInstrumentationConfig {
Copy link
Member

Choose a reason for hiding this comment

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

are you overriding this just to have correct type, if so I would rather do it differently by refactoring abstract class - something like this

export abstract class InstrumentationAbstract<T = types.InstrumentationConfig>
  implements types.Instrumentation {
  protected _config: T;
  constructor(
    public readonly instrumentationName: string,
    public readonly instrumentationVersion: string,
    config: T

It would require small refactoring of init method as well as InstrumentationModuleDefinition

export interface InstrumentationModuleDefinition<T = any> {

But then you would not have to worry about overriding the getConfig to have typed config from instrumentation class WDYT ?

Copy link
Member Author

@Flarna Flarna Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I tried that a while ago but failed. typescript complained at various places like

  • the different default arguments in constructor of the concrete instrumentations
  • no compatible base type for registerInstrumentation() arguments.

There is already a stalled PR from @dyladan regarding this: #2388

Copy link
Member

Choose a reason for hiding this comment

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

My stalled PR was stalled because it was more difficult to get typescript to be happy than I anticipated when I started it. For now, I think doing it this way is probably fine.

I would like to make the types smarter eventually. I know it should be possible if someone has the time and inclination.

return super.getConfig();
}

/**
* Patch for grpc.Server.prototype.register(...) function. Provides auto-instrumentation for
* client_stream, server_stream, bidi, unary server handler calls.
Expand All @@ -134,7 +129,7 @@ export class GrpcJsInstrumentation extends InstrumentationBase {
) => ServerRegisterFunction {
const instrumentation = this;
return (originalRegister: ServerRegisterFunction) => {
const config = this._config;
const config = this.getConfig();
instrumentation._diag.debug('patched gRPC server');
return function register<RequestType, ResponseType>(
this: grpcJs.Server,
Expand Down
19 changes: 8 additions & 11 deletions experimental/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 @@ -19,7 +19,6 @@ import {
InstrumentationNodeModuleDefinition,
InstrumentationNodeModuleFile,
InstrumentationBase,
InstrumentationConfig,
isWrapped,
} from '@opentelemetry/instrumentation';
import {
Expand Down Expand Up @@ -55,17 +54,11 @@ export class GrpcNativeInstrumentation extends InstrumentationBase<
typeof grpcTypes
> {
constructor(
protected override _config: GrpcInstrumentationConfig & InstrumentationConfig = {},
name: string,
version: string
version: string,
config?: GrpcInstrumentationConfig
) {
super(name, version, _config);
}

public override setConfig(
config: GrpcInstrumentationConfig & InstrumentationConfig = {}
) {
this._config = Object.assign({}, config);
super(name, version, config);
}

init() {
Expand Down Expand Up @@ -107,6 +100,10 @@ export class GrpcNativeInstrumentation extends InstrumentationBase<
];
}

override getConfig(): GrpcInstrumentationConfig {
return super.getConfig();
}

private _getInternalPatchs() {
const onPatch = (
moduleExports: GrpcInternalClientTypes,
Expand Down Expand Up @@ -268,7 +265,7 @@ export class GrpcNativeInstrumentation extends InstrumentationBase<

// For a method defined in .proto as "UnaryMethod"
Object.entries(methods).forEach(([name, { originalName }]) => {
if (!_methodIsIgnored(name, this._config.ignoreGrpcMethods)) {
if (!_methodIsIgnored(name, this.getConfig().ignoreGrpcMethods)) {
methodList.push(name); // adds camel case method name: "unaryMethod"
if (
originalName &&
Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-instrumentation-grpc/src/grpc/serverUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,6 @@ export const shouldNotTraceServerCall = function (
const parsedName = name.split('/');
return _methodIsIgnored(
parsedName[parsedName.length - 1] || name,
this._config.ignoreGrpcMethods
this.getConfig().ignoreGrpcMethods
);
};
25 changes: 11 additions & 14 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 @@ -14,7 +14,6 @@
* limitations under the License.
*/

import { InstrumentationConfig } from '@opentelemetry/instrumentation';
import { GrpcInstrumentationConfig } from './types';
import { VERSION } from './version';
import { GrpcNativeInstrumentation } from './grpc';
Expand All @@ -34,35 +33,33 @@ export class GrpcInstrumentation {
public readonly instrumentationVersion: string = VERSION;

constructor(
protected _config: GrpcInstrumentationConfig & InstrumentationConfig = {}
config?: GrpcInstrumentationConfig
) {
this._grpcJsInstrumentation = new GrpcJsInstrumentation(
_config,
this.instrumentationName,
this.instrumentationVersion
this.instrumentationVersion,
config
);
this._grpcNativeInstrumentation = new GrpcNativeInstrumentation(
_config,
this.instrumentationName,
this.instrumentationVersion
this.instrumentationVersion,
config
);
}

public setConfig(
config: GrpcInstrumentationConfig & InstrumentationConfig = {}
) {
this._config = Object.assign({}, config);
this._grpcJsInstrumentation.setConfig(this._config);
this._grpcNativeInstrumentation.setConfig(this._config);
public setConfig(config?: GrpcInstrumentationConfig) {
this._grpcJsInstrumentation.setConfig(config);
this._grpcNativeInstrumentation.setConfig(config);
}

/**
* @internal
* Public reference to the protected BaseInstrumentation `_config` instance to be used by this
* plugin's external helper functions
*/
public getConfig() {
return this._config;
public getConfig(): GrpcInstrumentationConfig {
// grpcNative and grpcJs have their own config copy which should be identical so just pick one
return this._grpcJsInstrumentation.getConfig();
}

init() {
Expand Down
9 changes: 4 additions & 5 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 @@ -44,7 +44,6 @@ import * as utils from './utils';
import { VERSION } from './version';
import {
InstrumentationBase,
InstrumentationConfig,
InstrumentationNodeModuleDefinition,
isWrapped,
safeExecuteInTheMiddle,
Expand All @@ -60,11 +59,11 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
private readonly _version = process.versions.node;
private _headerCapture;

constructor(config: HttpInstrumentationConfig & InstrumentationConfig = {}) {
constructor(config?: HttpInstrumentationConfig) {
super(
'@opentelemetry/instrumentation-http',
VERSION,
Object.assign({}, config)
config
);

this._headerCapture = this._createHeaderCapture();
Expand All @@ -74,8 +73,8 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
return this._config;
}

override setConfig(config: HttpInstrumentationConfig & InstrumentationConfig = {}): void {
this._config = Object.assign({}, config);
override setConfig(config?: HttpInstrumentationConfig): void {
super.setConfig(config);
this._headerCapture = this._createHeaderCapture();
}

Expand Down
6 changes: 2 additions & 4 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 @@ -89,13 +89,11 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
private _xhrMem = new WeakMap<XMLHttpRequest, XhrMem>();
private _usedResources = new WeakSet<PerformanceResourceTiming>();

constructor(
config: XMLHttpRequestInstrumentationConfig & InstrumentationConfig = {}
) {
constructor(config?: XMLHttpRequestInstrumentationConfig) {
super(
'@opentelemetry/instrumentation-xml-http-request',
VERSION,
Object.assign({}, config)
config
);
}

Expand Down