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

fix: instrumentation base calls init on partly initialized instrumentations #2417

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 2 additions & 6 deletions packages/opentelemetry-instrumentation-fetch/src/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,6 @@ export interface FetchInstrumentationConfig extends InstrumentationConfig {
export class FetchInstrumentation extends InstrumentationBase<
Promise<Response>
> {
readonly component: string = 'fetch';
readonly version: string = VERSION;
moduleName = this.component;
private _usedResources = new WeakSet<PerformanceResourceTiming>();
private _tasksCount = 0;

Expand All @@ -82,10 +79,9 @@ export class FetchInstrumentation extends InstrumentationBase<
VERSION,
Object.assign({}, config)
);
this.loadInstrumentation();
}

init() {}

private _getConfig(): FetchInstrumentationConfig {
return this._config;
}
Expand Down Expand Up @@ -196,7 +192,7 @@ export class FetchInstrumentation extends InstrumentationBase<
return this.tracer.startSpan(spanName, {
kind: api.SpanKind.CLIENT,
attributes: {
[AttributeNames.COMPONENT]: this.moduleName,
[AttributeNames.COMPONENT]: this.instrumentationName,
[SemanticAttributes.HTTP_METHOD]: method,
[SemanticAttributes.HTTP_URL]: url,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export class GrpcJsInstrumentation extends InstrumentationBase {
version: string
) {
super(name, version, _config);
this.loadInstrumentation(this.getInstrumentationsModules());
}

public override setConfig(
Expand All @@ -69,7 +70,7 @@ export class GrpcJsInstrumentation extends InstrumentationBase {
this._config = Object.assign({}, config);
}

init() {
getInstrumentationsModules() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
getInstrumentationsModules() {
private _getInstrumentationsModules() {

return [
new InstrumentationNodeModuleDefinition<typeof grpcJs>(
'@grpc/grpc-js',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export class GrpcNativeInstrumentation extends InstrumentationBase<
version: string
) {
super(name, version, _config);
this.loadInstrumentation(this.getInstrumentationsModules());
}

public override setConfig(
Expand All @@ -68,7 +69,7 @@ export class GrpcNativeInstrumentation extends InstrumentationBase<
this._config = Object.assign({}, config);
}

init() {
getInstrumentationsModules() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
getInstrumentationsModules() {
private _getInstrumentationsModules() {

return [
new InstrumentationNodeModuleDefinition<typeof grpcTypes>(
'grpc',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,6 @@ export class GrpcInstrumentation {
return this._config;
}

init() {
// sub instrumentations will already be init when constructing them
return;
}

enable() {
this._grpcJsInstrumentation.enable();
this._grpcNativeInstrumentation.enable();
Expand Down
3 changes: 2 additions & 1 deletion packages/opentelemetry-instrumentation-http/src/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
VERSION,
Object.assign({}, config)
);
this.loadInstrumentation(this.getInstrumentationsModules());
}

private _getConfig(): HttpInstrumentationConfig {
Expand All @@ -75,7 +76,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
this._config = Object.assign({}, config);
}

init() {
getInstrumentationsModules() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
getInstrumentationsModules() {
private _getInstrumentationsModules() {

return [this._getHttpsInstrumentation(), this._getHttpInstrumentation()];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,6 @@ export interface XMLHttpRequestInstrumentationConfig
* This class represents a XMLHttpRequest plugin for auto instrumentation
*/
export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRequest> {
readonly component: string = 'xml-http-request';
readonly version: string = VERSION;
moduleName = this.component;

private _tasksCount = 0;
private _xhrMem = new WeakMap<XMLHttpRequest, XhrMem>();
private _usedResources = new WeakSet<PerformanceResourceTiming>();
Expand All @@ -95,10 +91,9 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
VERSION,
Object.assign({}, config)
);
this.loadInstrumentation();
}

init() {}

private _getConfig(): XMLHttpRequestInstrumentationConfig {
return this._config;
}
Expand Down Expand Up @@ -515,7 +510,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
* implements enable function
*/
override enable() {
this._diag.debug('applying patch to', this.moduleName, this.version);
this._diag.debug('applying patch to', this.instrumentationName, this.instrumentationVersion);

if (isWrapped(XMLHttpRequest.prototype.open)) {
this._unwrap(XMLHttpRequest.prototype, 'open');
Expand All @@ -535,7 +530,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
* implements disable function
*/
override disable() {
this._diag.debug('removing patch from', this.moduleName, this.version);
this._diag.debug('removing patch from', this.instrumentationName, this.instrumentationVersion);

this._unwrap(XMLHttpRequest.prototype, 'open');
this._unwrap(XMLHttpRequest.prototype, 'send');
Expand Down
11 changes: 1 addition & 10 deletions packages/opentelemetry-instrumentation/src/instrumentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,12 @@ import {
} from '@opentelemetry/api';
import { Meter, MeterProvider, metrics } from '@opentelemetry/api-metrics';
import * as shimmer from 'shimmer';
import { InstrumentationModuleDefinition } from './platform/node';
import * as types from './types';

/**
* Base abstract internal class for instrumenting node and web plugins
*/
export abstract class InstrumentationAbstract<T = any>
export abstract class InstrumentationAbstract
implements types.Instrumentation {
protected _config: types.InstrumentationConfig;

Expand Down Expand Up @@ -116,12 +115,4 @@ export abstract class InstrumentationAbstract<T = any>
/* Enable plugin */
public abstract disable(): void;

/**
* Init method in which plugin should define _modules and patches for
* methods
*/
protected abstract init():
| InstrumentationModuleDefinition<T>
| InstrumentationModuleDefinition<T>[]
| void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,29 @@

import { InstrumentationAbstract } from '../../instrumentation';
import * as types from '../../types';
import { InstrumentationBaseBrowser } from './types';

/**
* Base abstract class for instrumenting web plugins
*/
export abstract class InstrumentationBase
extends InstrumentationAbstract
implements types.Instrumentation {
implements InstrumentationBaseBrowser {
private _timer?: number;

constructor(
instrumentationName: string,
instrumentationVersion: string,
config: types.InstrumentationConfig = {}
) {
super(instrumentationName, instrumentationVersion, config);
this._timer = window?.setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

if I understand the browser implemenation correct loadInstrumentation() could be called in base class once for all.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is exactly the situation we have now, calling this in "upper" class makes privates to be unreachable

Copy link
Member

Choose a reason for hiding this comment

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

But it seems this is only a problem for node which does module patching

Copy link
Member Author

Choose a reason for hiding this comment

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

check the code the load also enables the package, you can't enable this earlier because you will hit exactly the same problem - no privates, so this iw what it looks like. Either no privates or enforcing the loadInstrumentation in constructor to be called immediately. This will keeps the consistency too. If you have better way of solving such issue that you will have privates and you can call loadInstrumentation in "upper" class, I'm open for suggestion. So far I don't see anything better, if you know how to eat cookie and have cookie pls do tell I will be happy to make it :)

Copy link
Member

Choose a reason for hiding this comment

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

My feeling is that the automatic/implict enable() is the problem. See also #2410 (comment)

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 PR fixes the problem you mentioned too

Copy link
Member

Choose a reason for hiding this comment

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

Why do we not just remove the enable option from the config and rely on calling enable() externally instead? Then in enable all constructors are guaranteed to be finished and there is no problem.

throw ('You forgot to call loadInstrumentation in constructor');
});
}

loadInstrumentation() {
clearTimeout(this._timer);
if (this._config.enabled) {
this.enable();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { Instrumentation } from '../../types';

export interface InstrumentationBaseBrowser extends Instrumentation {
loadInstrumentation(): void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,41 +19,57 @@ import * as path from 'path';
import * as RequireInTheMiddle from 'require-in-the-middle';
import { satisfies } from 'semver';
import { InstrumentationAbstract } from '../../instrumentation';
import { InstrumentationModuleDefinition } from './types';
import {
InstrumentationBaseNode,
InstrumentationModuleDefinition
} from './types';
import { diag } from '@opentelemetry/api';

/**
* Base abstract class for instrumenting node plugins
*/
export abstract class InstrumentationBase<T = any>
extends InstrumentationAbstract
implements types.Instrumentation {
private _modules: InstrumentationModuleDefinition<T>[];
implements InstrumentationBaseNode<T> {
private _modules?: InstrumentationModuleDefinition<any>[];
private _hooks: RequireInTheMiddle.Hooked[] = [];
private _enabled = false;
private _timer?: NodeJS.Timeout;

constructor(
instrumentationName: string,
instrumentationVersion: string,
config: types.InstrumentationConfig = {}
) {
super(instrumentationName, instrumentationVersion, config);
this._timer = setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This will result in a crash. Is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

see here #2417 (comment)

throw new Error('You forgot to call loadInstrumentation in constructor');
});
}

loadInstrumentation(instrumentationModuleDefinitions:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
loadInstrumentation(instrumentationModuleDefinitions:
protected loadInstrumentation(instrumentationModuleDefinitions:

InstrumentationModuleDefinition<any>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
InstrumentationModuleDefinition<any>
InstrumentationModuleDefinition<T>

Copy link
Member Author

@obecny obecny Aug 18, 2021

Choose a reason for hiding this comment

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

no module definition can have different type than a main, consider the situation when you are patching 2 modules, they will have different type

Copy link
Member

Choose a reason for hiding this comment

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

if differnt types can be used here why is T then possible in _onRequire which is called for all types passed in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is already some left overs from past, which can be handled in different PR, this PR is about fixing one thing mainly

| InstrumentationModuleDefinition<any>[]
| void
) {
if (typeof this._timer !== 'undefined'){
clearTimeout(this._timer);
}

let modules = this.init();
let modules = instrumentationModuleDefinitions;

if (modules && !Array.isArray(modules)) {
modules = [modules];
}

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

if (this._modules.length === 0) {
diag.warn(
'No modules instrumentation has been defined,' +
' nothing will be patched'
);
}

if (this._config.enabled) {
this.enable();
}
Expand Down Expand Up @@ -109,6 +125,10 @@ export abstract class InstrumentationBase<T = any>
}
this._enabled = true;

if (!this._modules) {
throw new Error('Modules not loaded, please call "loadInstrumentation" in' +
' constructor with InstrumentationModuleDefinition as param');
}
// already hooked, just call patch again
if (this._hooks.length > 0) {
for (const module of this._modules) {
Expand Down Expand Up @@ -149,7 +169,10 @@ export abstract class InstrumentationBase<T = any>
return;
}
this._enabled = false;

if (!this._modules) {
throw new Error('Modules not loaded, please call "loadInstrumentation" in' +
' constructor with InstrumentationModuleDefinition as param');
}
for (const module of this._modules) {
if (typeof module.unpatch === 'function' && module.moduleExports) {
module.unpatch(module.moduleExports, module.moduleVersion);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@
* limitations under the License.
*/

import { Instrumentation } from '../../types';

export interface InstrumentationBaseNode<T = any> extends Instrumentation {
loadInstrumentation(instrumentationModuleDefinitions: InstrumentationModuleDefinition<T>
| InstrumentationModuleDefinition<T>[]
| void
Copy link
Member

Choose a reason for hiding this comment

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

why | void?

Copy link
Member Author

Choose a reason for hiding this comment

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

browser

Copy link
Member

Choose a reason for hiding this comment

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

But this is in file platform/node/types.ts

): void;
}

export interface InstrumentationModuleFile<T> {
/** Name of file to be patched with relative path */
name: string;
Expand Down
8 changes: 0 additions & 8 deletions packages/opentelemetry-instrumentation/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,6 @@ export interface Instrumentation {

/** Method to get instrumentation config */
getConfig(): InstrumentationConfig;

/**
* Contains all supported versions.
* All versions must be compatible with [semver](https://semver.org/spec/v2.0.0.html) format.
* If the version is not supported, we won't apply instrumentation patch (see `enable` method).
* If omitted, all versions of the module will be patched.
*/
supportedVersions?: string[];
}

export interface InstrumentationConfig {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as assert from 'assert';
import { InstrumentationConfig } from '../../src';
import {
InstrumentationBase,
} from '../../src/platform/browser';

interface TestInstrumentationConfig extends InstrumentationConfig {
isActive?: boolean;
}

class TestInstrumentationWithMissingLoad extends InstrumentationBase {
constructor(config: TestInstrumentationConfig & InstrumentationConfig = {}) {
super('test', '1.0.0', Object.assign({}, config));
}

override enable() {}

override disable() {}
}

describe('BaseInstrumentation', () => {
describe('constructor', () => {
describe('when instrumentation does NOT call loadInstrumentation in constructor', () => {
it('should raise an error', done => {
window.onerror = error => {
assert.strictEqual(error, 'Uncaught You forgot to call loadInstrumentation in constructor');
done();
};
new TestInstrumentationWithMissingLoad();
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@ interface TestInstrumentationConfig extends InstrumentationConfig {
class TestInstrumentation extends InstrumentationBase {
constructor(config: TestInstrumentationConfig & InstrumentationConfig = {}) {
super('test', '1.0.0', Object.assign({}, config));
this.loadInstrumentation();
}

override enable() {}

override disable() {}
init() {}
}

describe('BaseInstrumentation', () => {
Expand Down
Loading