Instrumentations are enabled twice on bootstrap

[kkruk-sumo]

trafficstars

What version of OpenTelemetry are you using?

"@opentelemetry/api": "^1.0.2",
"@opentelemetry/instrumentation": "^0.24.0",
"@opentelemetry/instrumentation-document-load": "^0.24.0",
"@opentelemetry/web": "^0.24.0",

What version of Node are you using?

12.7.0

Please provide the code you used to setup the OpenTelemetry SDK

import { registerInstrumentations } from '@opentelemetry/instrumentation';
import { DocumentLoadInstrumentation } from '@opentelemetry/instrumentation-document-load';
import { WebTracerProvider } from '@opentelemetry/web';

const provider = new WebTracerProvider({});

registerInstrumentations({
  tracerProvider: provider,
  instrumentations: [
    new DocumentLoadInstrumentation(),
  ],
});

Then bundled using webpack@^5.50.0.

What did you do?

I run the provided code in Firefox and set breakpoints on enable and disable methods of DocumentLoadInstrumentation.

What did you expect to see?

The DocumentLoadInstrumentation.prototype.enable being called only once.

What did you see instead?

The DocumentLoadInstrumentation.prototype.enable was called twice with no calls on disable.

Additional context

It looks that all instrumentations are enabling twice on bootstrap firstly by InstrumentationBase and secondly by registerInstrumentations.

[legendecas]

The problem also prevents derived instrumentation class from defining their own fields, like:

export class SomeInstrumentation extends InstrumentationBase {
  someField: SomeType;
  constructor(someField: SomeType) {
    super(info.name, info.version);
    this.someField = someField;
  }

  enable() {
    // this.someField is not initialized yet.
  }
}

The Instrumentation.enable should be called after the derived class constructor is finished.

[legendecas]

Currently, the documentation of instrumentation all show two approaches to enable instrumentation:

1. call instrumentation.enable manually, (https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-instrumentation#usage-in-web)
2. 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)

IIUC, it is still left there for compatibility. Maybe it is time to remove the invocation in the InstrumentationBase constructor and the option InstrumentationConfig.enable now.

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions[bot]]

This issue was closed because it has been stale for 14 days with no activity.

[t2t2]

Was brought up in sig today, already fixed in #2610 (10 Nov 2021)

separate question about how good current logic is (as in when enable should be called, during instrumentation instance init or during registerInstrumentation call):

instrumentation config:	{enabled: true}	{enabled: false}
during new Instrumentation()	enable called	-
during registerInstrumentation	-	enable called