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

obecny · 2021-08-17T21:16:12Z

Which problem is this PR solving?

InstrumentationBase calls init on partly initialized Instrumentations #1989

Short description of the changes

Refactoring base class, needed to add now a function loadInstrumentation ti the base class. This function needs to be called in instrumenting class in constructor, there is a check for doing that, as otherwise all patching would not work. This refactoring will require very little change in each instrumentation. Updated the instrumentations as well, and also did some few small cleanups with some old stuff in instrumentation

…ations

codecov · 2021-08-17T21:19:27Z

Codecov Report

Merging #2417 (8e7a3e8) into main (f129138) will increase coverage by 0.70%.
The diff coverage is 100.00%.

❗ Current head 8e7a3e8 differs from pull request most recent head 5069c86. Consider uploading reports for the commit 5069c86 to get more accurate results

@@            Coverage Diff             @@
##             main    #2417      +/-   ##
==========================================
+ Coverage   92.14%   92.84%   +0.70%     
==========================================
  Files         120      137      +17     
  Lines        3998     4979     +981     
  Branches      844     1051     +207     
==========================================
+ Hits         3684     4623     +939     
- Misses        314      356      +42

Impacted Files	Coverage Δ
...emetry-instrumentation-grpc/src/instrumentation.ts	`82.14% <ø> (+5.47%)`	⬆️
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	`97.48% <100.00%> (ø)`
...elemetry-instrumentation-grpc/src/grpc-js/index.ts	`90.90% <100.00%> (+0.09%)`	⬆️
...entelemetry-instrumentation-grpc/src/grpc/index.ts	`93.10% <100.00%> (+0.05%)`	⬆️
...ges/opentelemetry-instrumentation-http/src/http.ts	`95.00% <100.00%> (+0.01%)`	⬆️
...emetry-instrumentation-xml-http-request/src/xhr.ts	`98.03% <100.00%> (ø)`
...entelemetry-instrumentation/src/instrumentation.ts	`70.37% <100.00%> (ø)`
...strumentation/src/platform/node/instrumentation.ts	`39.50% <100.00%> (+15.89%)`	⬆️
.../opentelemetry-exporter-collector/src/transform.ts	`88.69% <0.00%> (ø)`
packages/opentelemetry-sdk-trace-web/src/utils.ts	`94.90% <0.00%> (ø)`
... and 16 more

dyladan · 2021-08-18T14:29:25Z

What is the reason for requiring to call loadInstrumentation in the constructor? This was something we previously called automatically.

Flarna · 2021-08-18T20:29:09Z

packages/opentelemetry-instrumentation-grpc/src/grpc-js/index.ts

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

-  init() {
+  getInstrumentationsModules() {


Suggested change

getInstrumentationsModules() {

private _getInstrumentationsModules() {

Flarna · 2021-08-18T20:29:32Z

packages/opentelemetry-instrumentation-grpc/src/grpc/index.ts

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

-  init() {
+  getInstrumentationsModules() {


Suggested change

getInstrumentationsModules() {

private _getInstrumentationsModules() {

Flarna · 2021-08-18T20:34:01Z

packages/opentelemetry-instrumentation-http/src/http.ts

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

-  init() {
+  getInstrumentationsModules() {


Suggested change

getInstrumentationsModules() {

private _getInstrumentationsModules() {

Flarna · 2021-08-18T20:41:12Z

packages/opentelemetry-instrumentation/src/platform/browser/instrumentation.ts

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


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

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

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

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 :)

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

this PR fixes the problem you mentioned too

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.

Flarna · 2021-08-18T20:42:59Z

packages/opentelemetry-instrumentation/src/platform/node/types.ts

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


why | void?

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

Flarna · 2021-08-18T20:47:17Z

packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

+  }
+
+  loadInstrumentation(instrumentationModuleDefinitions:
+                        InstrumentationModuleDefinition<any>


Suggested change

InstrumentationModuleDefinition<any>

InstrumentationModuleDefinition<T>

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

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

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

Flarna · 2021-08-18T20:48:24Z

packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts


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


This will result in a crash. Is this intended?

see here #2417 (comment)

Flarna · 2021-08-18T20:49:05Z

packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

+    });
+  }
+
+  loadInstrumentation(instrumentationModuleDefinitions:


Suggested change

loadInstrumentation(instrumentationModuleDefinitions:

protected loadInstrumentation(instrumentationModuleDefinitions:

Flarna · 2021-08-18T20:59:32Z

What is the reason for requiring to call loadInstrumentation in the constructor? This was something we previously called automatically.

see #1989
if loadInstrumentation() is called by derived class at the end of constructor the object is completly initialized.

But this solution is still weak as someone might subclass an instrumentation and then the probelm appears again.

obecny · 2021-08-18T21:25:10Z

What is the reason for requiring to call loadInstrumentation in the constructor? This was something we previously called automatically.

see #1989
if loadInstrumentation() is called by derived class at the end of constructor the object is completly initialized.

But this solution is still weak as someone might subclass an instrumentation and then the probelm appears again.

The previous solution was exactly to prevent it, it had it cons that you should not be using any privates etc., just create a pure definition for the patches, but it looks like this approach was hard to understand hence the ticket that was created for it. Current solution requires a user to call it in constructor, then by the time patching is done all is already initialised so you will have private initialised too. If you don't do this you will get an error saying wha you are missing so this will be catch'ed already during development by the author, long before it will be used. Patch needs to be done synchronously before everything else, it cannot be postponed that's why you have to call it immediately.

legendecas · 2021-08-21T06:52:40Z

In the documents I'm aware of, they are suggesting users to call Instrumentation.enable outside the instrumentation constructor:

call instrumentation.enable manually, (https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation#usage-in-web)
use autoLoader: registerInstrumentations({ instrumentations: [new Instrumentation()] }), (https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation#node---auto-loader, https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation-http#usage). The auto loaders call the instrumentation.enable.

Is it intended that we still automatically call enable in instrumentation constructors? Is it intended to call instrumentation.enable twice for the suggested approaches in documentation?

dyladan · 2021-08-24T19:34:06Z

In the documents I'm aware of, they are suggesting users to call Instrumentation.enable outside the instrumentation constructor:

call instrumentation.enable manually, (https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation#usage-in-web)

use autoLoader: registerInstrumentations({ instrumentations: [new Instrumentation()] }), (https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation#node---auto-loader, https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation-http#usage). The auto loaders call the instrumentation.enable.

Is it intended that we still automatically call enable in instrumentation constructors? Is it intended to call instrumentation.enable twice for the suggested approaches in documentation?

I think calling enable twice is likely to cause bugs in many instrumentations which are hard to debug. It should be avoided if possible.

obecny · 2021-08-24T19:50:10Z

In the documents I'm aware of, they are suggesting users to call Instrumentation.enable outside the instrumentation constructor:

call instrumentation.enable manually, (https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation#usage-in-web)

use autoLoader: registerInstrumentations({ instrumentations: [new Instrumentation()] }), (https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation#node---auto-loader, https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation-http#usage). The auto loaders call the instrumentation.enable.

Is it intended that we still automatically call enable in instrumentation constructors? Is it intended to call instrumentation.enable twice for the suggested approaches in documentation?

I think calling enable twice is likely to cause bugs in many instrumentations which are hard to debug. It should be avoided if possible.

I think you are missing here one thing and we had similar discussion a year ago to, the instrumentation needs to be enabled and patched right after it is created. You can't wait as if anyone will require the class before instrumentation patched that all auto instrumentation is gone. So by default the instrumentation is enabled and patched. If you know what you are doing you have to explicitly add "enable: false" to the config and then you have full control over it, with all consequences.
If you don't do that, then wait a bit and you will have hundreds of issues being created that the instrumentation is not working.

function enable can be called multiple times as it doesn't matter how many times you call it things will be enabled once

  public enable() {
    if (this._enabled) {
      return;
    }

rauno56 · 2021-09-07T11:12:01Z

Having an async timer and a method that has to be called feels very fragile. I'd move towards having less "business-logic" in the instrumentation and making it more declarative or "datalike" than the opposite. init basically only sets an attribute on the instance, which in most cases is static anyway - it's a list of instructions, it might as well be assigned on the prototype instead and have nothing to do with the constructor - latter should be pretty much empty of any logic.

Enough of philosophy...

Perhaps that's what @dyladan meant here, but the enable config option could also essentially be moved to the autoloaders or whatnot(which, intuitively kinda makes sense, right?), so from the perspective of the end-user nothing has to change.

I think you are missing here one thing and we had similar discussion a year ago to, the instrumentation needs to be enabled and patched right after it is created

That can be done either in the autoloader, or by the user before any require calls - I don't see a problem here, but perhaps I'm missing something?

If you don't do that, then wait a bit and you will have hundreds of issues being created that the instrumentation is not working.

Just to be sure I understand: In other words, do you think that if we don't enable instrumentations by default it will cause too many problems?

obecny · 2021-09-08T22:20:14Z

Guys I would strongly encourage you to try to fix the original raised issue (#1989) and at the same time create a bullet proof solution that will ensure the instrumentation is patched correctly having in mind all possible scenarios that a user can have when using any instrumentation including a 3rd party one. Also when using single instrumentation with and without loader. If you know how to fix it better I would be happy to see such solution.

fix: instrumentation base calls init on partly initialized instrument…

f130349

…ations

obecny added the breaking label Aug 17, 2021

obecny requested a review from a team August 17, 2021 21:16

Merge branch 'main' into instrumentations

5069c86

Flarna reviewed Aug 18, 2021

View reviewed changes

legendecas linked an issue Aug 21, 2021 that may be closed by this pull request

InstrumentationBase calls init on partly initialized Instrumentations #1989

Open

obecny closed this Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

obecny commented Aug 17, 2021

codecov bot commented Aug 17, 2021 •

edited

Loading

dyladan commented Aug 18, 2021

Flarna Aug 18, 2021

Flarna Aug 18, 2021

Flarna Aug 18, 2021

Flarna Aug 18, 2021

obecny Aug 18, 2021

Flarna Aug 18, 2021

obecny Aug 18, 2021

Flarna Aug 20, 2021

obecny Aug 23, 2021

dyladan Sep 1, 2021

Flarna Aug 18, 2021

obecny Aug 18, 2021

Flarna Aug 18, 2021

Flarna Aug 18, 2021

obecny Aug 18, 2021 •

edited

Loading

Flarna Aug 18, 2021

obecny Aug 18, 2021

Flarna Aug 18, 2021

obecny Aug 18, 2021

Flarna Aug 18, 2021

Flarna commented Aug 18, 2021

obecny commented Aug 18, 2021

legendecas commented Aug 21, 2021 •

edited

Loading

dyladan commented Aug 24, 2021

obecny commented Aug 24, 2021

rauno56 commented Sep 7, 2021

obecny commented Sep 8, 2021

	getInstrumentationsModules() {
	private _getInstrumentationsModules() {

	InstrumentationModuleDefinition<any>
	InstrumentationModuleDefinition<T>

	loadInstrumentation(instrumentationModuleDefinitions:
	protected loadInstrumentation(instrumentationModuleDefinitions:

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

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

Conversation

obecny commented Aug 17, 2021

Which problem is this PR solving?

Short description of the changes

codecov bot commented Aug 17, 2021 • edited Loading

Codecov Report

dyladan commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flarna commented Aug 18, 2021

obecny commented Aug 18, 2021

legendecas commented Aug 21, 2021 • edited Loading

dyladan commented Aug 24, 2021

obecny commented Aug 24, 2021

rauno56 commented Sep 7, 2021

obecny commented Sep 8, 2021

codecov bot commented Aug 17, 2021 •

edited

Loading

obecny Aug 18, 2021 •

edited

Loading

legendecas commented Aug 21, 2021 •

edited

Loading