-
Notifications
You must be signed in to change notification settings - Fork 820
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -60,6 +60,7 @@ export class GrpcNativeInstrumentation extends InstrumentationBase< | |||||
version: string | ||||||
) { | ||||||
super(name, version, _config); | ||||||
this.loadInstrumentation(this.getInstrumentationsModules()); | ||||||
} | ||||||
|
||||||
public override setConfig( | ||||||
|
@@ -68,7 +69,7 @@ export class GrpcNativeInstrumentation extends InstrumentationBase< | |||||
this._config = Object.assign({}, config); | ||||||
} | ||||||
|
||||||
init() { | ||||||
getInstrumentationsModules() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return [ | ||||||
new InstrumentationNodeModuleDefinition<typeof grpcTypes>( | ||||||
'grpc', | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -65,6 +65,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> { | |||||
VERSION, | ||||||
Object.assign({}, config) | ||||||
); | ||||||
this.loadInstrumentation(this.getInstrumentationsModules()); | ||||||
} | ||||||
|
||||||
private _getConfig(): HttpInstrumentationConfig { | ||||||
|
@@ -75,7 +76,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> { | |||||
this._config = Object.assign({}, config); | ||||||
} | ||||||
|
||||||
init() { | ||||||
getInstrumentationsModules() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return [this._getHttpsInstrumentation(), this._getHttpInstrumentation()]; | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if I understand the browser implemenation correct There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My feeling is that the automatic/implict There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this PR fixes the problem you mentioned too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we not just remove the |
||
throw ('You forgot to call loadInstrumentation in constructor'); | ||
}); | ||
} | ||
|
||
loadInstrumentation() { | ||
clearTimeout(this._timer); | ||
if (this._config.enabled) { | ||
this.enable(); | ||
} | ||
|
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 | ||||
---|---|---|---|---|---|---|
|
@@ -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(() => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will result in a crash. Is this intended? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
InstrumentationModuleDefinition<any> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if differnt types can be used here why is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||||||
} | ||||||
|
@@ -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) { | ||||||
|
@@ -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); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. browser There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
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(); | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.