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

Access patterns to config in instrumentations #4668

Open
blumamir opened this issue May 1, 2024 · 3 comments
Open

Access patterns to config in instrumentations #4668

blumamir opened this issue May 1, 2024 · 3 comments

Comments

@blumamir
Copy link
Member

blumamir commented May 1, 2024

Now that #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 blumamir changed the title Access to config in instrumentations Access patterns to config in instrumentations May 1, 2024
@blumamir
Copy link
Member Author

blumamir commented May 1, 2024

Pros options 1:

  • shorter to write
  • bit more performant (I think)

Pros options 2:

  • will work correctly for advanced cases if the function is overridden
  • can make _config private to decrease API surface.
  • aligns will with tracer, meter and logger class properties, which are private and are only accessed via a getter.

@david-luna
Copy link
Contributor

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
Copy link
Member

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.

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

No branches or pull requests

3 participants