-
Notifications
You must be signed in to change notification settings - Fork 909
Description
The "@opentelemetry/instrumentation" defines a generic InstrumentationConfig interface:
export interface InstrumentationConfig {
/**
* Whether to enable the plugin.
* @default true
*/
enabled?: boolean;
}
This interface defines an optional configuration for all instrumentations, named "enabled", with the default value "true". The default value is enforced In InstrumentationAbstract:
this._config = {
enabled: true,
...config,
};
Which introduces the following issues:
Inconsistent With OpenTelemetry Specification
The OpenTelemetry specification for environment variables says:
All Boolean environment variables SHOULD be named and defined such that false is the expected safe default behavior
The concept is applied in the specification with the OTEL_SDK_DISABLED
env variable.
Although this configuration is not an environment variable in the exact context, having the default value as true
creates inconsistencies in the OpenTelemetry JS implementation which can be a source of confusions for end users and code maintainers, and increases cognitive load for consumers which should be aware of default values per use-case.
For node-sdk implementation, the property is used with compliance to spec:
export class NodeSDK {
...
private _disabled?: boolean;
...
}
Harder to Maintain
When a user chooses not to set the enabled
flag (which is very common) leaving it as undefined
, the implementation code needs to be aware of the default and make sure to use nullish-coalescing or some other fallback mechnisim everywhere it is accessed.
This practice requires awareness which is not straightforward to comply with, as the enabled
flag is a public API property and can be consumed in OpenTelemetry package but also by any user of the @opentelemetry/instrumentation
package.
Buggy
The current implementation of InstrumentationAbstract
assumes that the config object is sanitized in the constructor:
constructor(
public readonly instrumentationName: string,
public readonly instrumentationVersion: string,
config: ConfigType = {}
) {
this._config = {
enabled: true,
...config,
};
And then read the value freely without checking for undefined
and defaulting the true
. For example, the value is read as-is here.
Since the logic to change enabled undefined
to true
is not applied in the InstrumentationAbstract
class with setConfig
function:
/**
* Sets InstrumentationConfig to this plugin
* @param InstrumentationConfig
*/
public setConfig(config: InstrumentationConfig = {}): void {
this._config = Object.assign({}, config);
}
any call to setConfig
will invalidate the assumption which is a bug. Even if we fix it for the base class, It is still extremely easy for instrumentation authors that override setConfig
to miss this nuance and introduce bugs to the implementation.
Fix
I can think of 2 ways to fix this issue:
- align with opentelemetry specification and change the
enabled
property todisabled
, and change the default value tofalse
. This will be a potential breaking change for the package which we can introduce at the moment but not after the package is stable. - Keep the current property as
enabled
but fix the places where it is used, as well as add documentation for consumers on how to use it safely.
I personally think that since the package is still experimental and breaking changes are allowed, it is a good opportunity to introduce changes that will promote consistent and robust use of the package.
Would love to hear more opinions and to work on changing this property if there isn't any push-back.