-
Notifications
You must be signed in to change notification settings - Fork 792
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
Access patterns to config in instrumentations #4668
Access patterns to config in instrumentations #4668
Comments
Pros options 1:
Pros options 2:
|
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 |
Implemented in open-telemetry/opentelemetry-js-contrib#2289 |
Now that #4659 will soon merge, all instrumentations can apply cleanups to how the config object is consumed.
There are now 2 options:
this._config
fromInstrumentationAbstract
which is now typed correctly.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
objectprivate
instead ofprotected
?Would love to hear more opinions.
The text was updated successfully, but these errors were encountered: