open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.57k stars 743 forks source link

Access patterns to config in instrumentations #4668

Open blumamir opened 2 months ago

blumamir commented 2 months ago

Now that https://github.com/open-telemetry/opentelemetry-js/pull/4659 will soon merge, all instrumentations can apply cleanups to how the config object is consumed.

There are now 2 options:

  1. using this._config from InstrumentationAbstract which is now typed correctly.
  2. using the getConfig() function from the Instrumentation interface.

I think we should decide which one is superior and refactor all existing instrumentations to use a consistent pattern. I tend to prefer the getConfig option as the getter can potentially be overridden with custom logic which will be lost if we simply access the _config property.

If we choose option (2), should we make the _config object private instead of protected?

Would love to hear more opinions.

blumamir commented 2 months ago

Pros options 1:

Pros options 2:

david-luna commented 2 months ago

Here is my opinion. If we already have a public API to access the field I think we should leverage that for accessing even in subclasses. Also I'm not sure about the significance the perf gain of having direct access to the config which is the most appealing pro of option 1.

pichlermarc commented 2 months ago

Here is my opinion. If we already have a public API to access the field I think we should leverage that for accessing even in subclasses. Also I'm not sure about the significance the perf gain of having direct access to the config which is the most appealing pro of option 1.

I agree. IMO we should go with getConfig() and make _config private.

blumamir commented 3 weeks ago

Implemented in https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2289 Would appreciate any review on this PR 🙏