could we *not* `.enable()` instrumentations in their constructor?

[trentm]

trafficstars

• [x] This only affects the JavaScript OpenTelemetry library

Motivation

why-u-no-warn.js

const ioredis = require('ioredis');  // **Oops, required ioredis before instrumenting it.**

const {NodeSDK} = require('@opentelemetry/sdk-node');
const {OTLPTraceExporter} = require('@opentelemetry/exporter-trace-otlp-http');
const {IORedisInstrumentation} = require("@opentelemetry/instrumentation-ioredis");

const sdk = new NodeSDK({
    traceExporter: new OTLPTraceExporter(),
    instrumentations: [new IORedisInstrumentation()]
});

sdk.start();

async function main() {
    // Do something with ioredis.
}

main();

This code has made an error by requireing the ioredis package before instrumenting it. That might mean that the instrumentation doesn't work. There is a diag.warn that is meant to warn about this case. However that diag.warn doesn't work with the above usage:

% OTEL_NODE_RESOURCE_DETECTORS=none OTEL_LOG_LEVEL=debug node why-u-no-warn.js
@opentelemetry/api: Registered a global for diag v1.9.0.
@opentelemetry/api: Registered a global for trace v1.9.0.
@opentelemetry/api: Registered a global for context v1.9.0.
@opentelemetry/api: Registered a global for propagation v1.9.0.

The reason it doesn't work is because the _warnOnPreloadedModules is called in <Instrumentation>.enable(), which is called in the Instrumentation constructor (via InstrumentationBase). This is before the NodeSDK has a chance to process the OTEL_LOG_LEVEL envvar and set a DiagConsoleLogger.

My understanding is that the normal course of operation is that registerInstrumentations() is eventually called to enable the instrumentations. I don't understand why .enable() is called in the InstrumentationBase constructor. (I also haven't yet dug into the history behind it.)

Question

Do you think we could get away from .enable()ing instrumentations in their constructor?