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

Add a WithEnv option #415

Closed
MrAlias opened this issue Oct 25, 2023 · 3 comments · Fixed by #417
Closed

Add a WithEnv option #415

MrAlias opened this issue Oct 25, 2023 · 3 comments · Fixed by #417
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Oct 25, 2023

Is your feature request related to a problem? Please describe.

Environment variable precedence is a subjective matter. Some users would like the passed options to take precedence to them and other would like the opposite.

It would be ideal if this was configurable

Describe the solution you'd like

Add a WithEnv InstrumentationOption that will parse the relevant environment variables and set config values.

The precedence of this option will be based on the order it is passed. If first, the following options will have precedence. And if last, it will have precedence.

@MrAlias MrAlias added the enhancement New feature or request label Oct 25, 2023
@MrAlias MrAlias self-assigned this Oct 25, 2023
@pellared
Copy link
Member

pellared commented Oct 25, 2023

The precedence of this option will be based on the order it is passed.

Do we have other places where the order of option parameters matters?

AFAIK it is usually seen as an antipattern.

Reference: https://youtu.be/5uM6z7RnReE?si=clZ6TX3XlETobPiz&t=721

The problem if such design is that the user would need to read and remember the behavior described in the docs which is not obvious.

Maybe better adding and option like WithEnvVarPrecedence?

This is not a strong opinion. I am just giving feedback.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 25, 2023

I'm not sure I follow. We already have a precedence of "last-option-wins" on any conflict. This would just be a continuation of that and it would remove the additional environment linking to each option.

@pellared
Copy link
Member

We already have a precedence of "last-option-wins" on any conflict. This would just be a continuation of that and it would remove the additional environment linking to each option.

This answers my first question. Therefore, I think this issue is OK and I created a separate one #416.

@MrAlias MrAlias added this to the v0.8.0-alpha milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants