feat(instrumentation): remove default value for config in base instrumentation constructor

[blumamir]

This PR makes the config object for base instrumentations (node + browser), required instead of optional with default value empty object.

Motivation

The current implementation of instrumentation base classes requires a config object to be set. The 2 functions that accept a config object (constructor and setConfig) currently define the function parameter as optional, with default value {}. This creates the following problems:

• By making this property optional, we are unable to add additional parameters to the constructor function in a clean way, which is something I want to do soon.
• if the config object is not supplied, it is assumed to be empty object {}, but there is no typescript enforcement that the config object has only optional properties. Thus, this assumption may break and we needed as ConfigType typescript casting to ignore this justified incompatibility.
• setConfig is already defined as required in the Instrumentation interface. Thus the definition of it as optional in the InstrumentationAbstract implementaion is not consistent with the interface.
• It allows instrumentations not to pass any config object to the base class, which renders them unuseful to be included in distributions, which needs to at least be able to pass a config object that includes the enabled flag.

Change

• apply the constructor pattern we settled upon in chore: align config constructor pattern across instrumentations opentelemetry-js-contrib#2162 to core instrumentations
• Make the config argument required is constructor signatures and setConfig signatures.
• Removing the typescript casting which was needed due to type incompatibility with empty object
• Replace Object.assign({}, config) with {...config} so we are consistent in both places

Distributions Considerations

This PR will make it so that instrumentations cannot be written like so:

export class FooInstrumentation extends InstrumentationBase {

  constructor() {
    super('@opentelemetry/instrumentation-foo', VERSION);
  }

Since the config object is now required. Even if FooInstrumentation does not offer any specific config options, thus might be tempted to exclude it altogether from the constructor, it makes it so users are not able to pass generic config object which defines enabled value.

Making the argument required, communicates that instrumentation need to pass a value and promote better-practices and more robust implementations. This has been aligned in contrib with open-telemetry/opentelemetry-js-contrib#2162

Breaking Change

• Contrib PRs has all been aligned to support this change with chore: align config constructor pattern across instrumentations opentelemetry-js-contrib#2162 .
• Core instrumentations are also aligned to use the new constructor pattern in this PR.
• There may exist other 3rd party instrumentations out there which do not pass a config object. for them this change might be breaking, but the fix is easy: there will be a typescript error about missing argument, and instrumentation will have the chance to apply good practice and refactor the constructor to receive a config object from the user.

[david-luna]

By making this property optional, we are unable to add additional parameters to the constructor function in a clean way, which is something I want to do soon.

Which is going the be the next change then that requires these additional parameters? what's the use case?

[blumamir]

@david-luna Thank you for the feedback.

By making this property optional, we are unable to add additional parameters to the constructor function in a clean way, which is something I want to do soon.

Which is going the be the next change then that requires these additional parameters? what's the use case?

Currently, one can extract the instrumentation npm package name and version from the instrumentation instance at runtime, which is useful for distributions to create tooling and report runtime statuses.

There is currently an instrumentationDescription property for the instrumentation interface which I want to set as well.

Other than that, it would be very useful to also include this metadata per instrumentation:

• instrumented package/s (with supported versions) of the user-facing packages - which might differ from the modules returned by init(). I want a programmatic way to extract this information. fix!: standardize supported versions and set upper bound limit opentelemetry-js-contrib#2196 already documents and aligns this info in each instrumentation README, but I am looking for a structural, programmatically, and explicit way to use it.
• repository (open-telemetry/opentelemetry-js / open-telemetry/opentelemetry-js-contrib and possibly others) + the path of the instrumentation in the repo (/plugins/node/insftrumentation-foo) which will allow tools of distributions to do stuff like auto generate READMEs, aggregate data from git etc. this information is already present in package.json but it's less convenient to use at the moment.

I intend to address these topics in follow-up PRs and issues, so we will have a chance to discuss them in depth. I also plan to use these new features to introduce some improvements to the @opentelemetry/auto-instrumentations-node package README:

• auto-generate the list of supported instrumentation in README to make sure it is consistent with the actual instrumentations in the code.
• add description per instrumentation to give a better user experience for users.
• include the name of the user-facing packages with supported versions.

I also plan to use this information in odigos to report back instrumentation statuses to odigos control plane and auto-generate odigos documentation for the js instrumentations (so odigos users will have all the instrumentations info aggregated in one auto-generated markdown).

Regardless of the above usecases, I think there are other valid reasons to make the config object required which are listed in PR description.

[dyladan]

LGTM just minor wording nit in the changelog

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.79%. Comparing base (2610122) to head (c8f96d8).
Report is 33 commits behind head on main.

❗ Current head c8f96d8 differs from pull request most recent head f24c103. Consider uploading reports for the commit f24c103 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4695      +/-   ##
==========================================
- Coverage   90.77%   89.79%   -0.99%     
==========================================
  Files          90      149      +59     
  Lines        1930     3242    +1312     
  Branches      417      706     +289     
==========================================
+ Hits         1752     2911    +1159     
- Misses        178      331     +153     

Files	Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	95.00% <100.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts	97.60% <100.00%> (ø)
...entelemetry-instrumentation/src/instrumentation.ts	87.23% <100.00%> (ø)
...umentation/src/platform/browser/instrumentation.ts	100.00% <100.00%> (ø)

... and 98 files with indirect coverage changes