Skip to content
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

feat(instrumentation): generic config type in instrumentation base #4659

Merged
merged 17 commits into from
May 2, 2024

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Apr 25, 2024

This PR is aimed to target one specific improvement to the @opentelemetry/instrumentation package in a non-breaking manner. It allows any existing instrumentation to function correctly, but introduces generic types on the base instrumentation config object, which can cleanup some inconsistent, verbose and non-type-safe handling of config in the current instrumentations implementations.

I tried to limit the scope of this PR to as minimum as possible so that it is easy to review and reason about. there are many opportunities for improvements in the places this PR touches which can be applied in followup PRs.

Problem

There are currently dozens of instrumentations in contrib and core, but there seems to be no standard way to handle instrumentation config. The use of config objects is also a popular feature being used by significant part of existing instrumentations.

Most instrumentations define their own config type with custom options like:

export interface GrpcInstrumentationConfig extends InstrumentationConfig {
  /* Omits tracing on any gRPC methods that match any of
   * the IgnoreMatchers in the ignoreGrpcMethods list
   */
  ignoreGrpcMethods?: IgnoreMatcher[];
  /** Map the following gRPC metadata to span attributes. */
  metadataToSpanAttributes?: {
    client?: {
      responseMetadata?: string[];
      requestMetadata?: string[];
    };
    server?: {
      responseMetadata?: string[];
      requestMetadata?: string[];
    };
  };
}

The actual config is generically stored in InstrumentationAbstract base class of InstrumentationBase as:

  protected _config: InstrumentationConfig;

The base class also supplies some services, like the getConfig and setConfig functions, along with defaulting the enabled flag to true if it's undefined.

Then, to then access the config object, instrumentations usually do one of the following:

this:

protected override _config!: AwsSdkInstrumentationConfig;

or this:

  override getConfig(): BunyanInstrumentationConfig {
    return this._config;
  }
  override setConfig(config: BunyanInstrumentationConfig) {
    this._config = Object.assign({}, DEFAULT_CONFIG, config);
  }

or this:

  private _getMaxQueryLength(): number {
    const config = this.getConfig() as CassandraDriverInstrumentationConfig;
    return config.maxQueryLength ?? 65536;
  }

Suggested Fix

I suggest hoisting the config type to the base class, thus removing the patterns above from instrumentations, make them less verbose and align them to use the config in a consistent manner.

Included in this PR

Base Class Config Typing

Added generic type to InstrumentationBase (browser and node), to InstrumentationAbstract and type the config arguments correctly. The type defaults to InstrumentationConfig so instrumentations that are not using config options can omit it safely.

This change also includes modifying existing instrumentations in core to use the new generic type when extending InstrumentationBase

InstrumentationConfig Documentation

The current implementation assumes that the config object can be initialized to default values with {}, but it's not enforced nor documented. I don't want to address this question in the current PR, but added the requirement to the InstrumentationConfig interface documentation.

Align getConfig

The existing instrumentations in core defined a specific _getConfig function which only casted the type to it's known value. Now that getConfig is typed correctly with generics, the base function can be used. I cleaned up the now redundant override functions from instrumentations in core. We will need to do that in contrib as well at any time after release.

These functions are removed:

  private _getConfig(): FetchInstrumentationConfig {
    return this._config;
  }

And usage becomes:

if (!this.getConfig().ignoreNetworkEvents) {

Instead of the _getConfig() alternative.

General Concerns

  • The generic type will now be part of the public API of the instrumentation package. This means that it must transpile successfully in end user applications with only regular dependencies (vs dev dependencies). As the instrumentation config type is already public API (since it is intended to be used by package consumers), this should not be an issue.

  • I tested the new instrumentation package with contrib to verify it works with npm link. Tested both instrumentation with config object (instrumentation-amqplib) and instrumentation that is not using config (instrumentation-nestjs-core)
    -Since it's not a breaking change, we can release the feature in core and then apply it in contrib without blocking any release and with no pressure.

@blumamir blumamir requested a review from a team April 25, 2024 06:14
@blumamir blumamir marked this pull request as draft April 25, 2024 06:14
@@ -46,7 +49,7 @@ export abstract class InstrumentationAbstract implements Instrumentation {
constructor(
public readonly instrumentationName: string,
public readonly instrumentationVersion: string,
config: InstrumentationConfig = {}
config: ConfigType = {} as ConfigType // assuming ConfigType is an object with optional fields only
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This keeps existing behavior, having default value to empty object.
This assignment was typescript incorrect before, as the typed used was InstrumentationConfig which the real config object extends, but can also include required fields.

I choose not to address this issue in the current PR to keep the scope narrow and reviewable.

@blumamir blumamir changed the title feat!(instrumentation): generic config type and no default config value feat(instrumentation): generic config type and no default config value Apr 25, 2024
@blumamir blumamir changed the title feat(instrumentation): generic config type and no default config value feat(instrumentation): generic config type in instrumentation base Apr 25, 2024
Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for handling this! It will go a long way to help making configuration more consistent across instrumentations.

@blumamir blumamir merged commit 157c811 into open-telemetry:main May 2, 2024
19 checks passed
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…pen-telemetry#4659)

* feat!(instrumentation): generic config type and no default config value

* fix: apply type in base Instrumentation interface

* revert: enabled flag rename

* fix: autoloader types

* chore: lint fix

* revert: default config in constructor to empty object

* revert: make constructor config default empty object

* docs: note that instrumentation config fields are optional

* revert: deftaul type for generic

* revert: default object in instrumentation abstract constructor

* chore: lint fix

* chore: changelog

* fix: changelog in merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants