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

Refactor env var parsing #3766

Closed
pellared opened this issue Feb 23, 2023 · 5 comments
Closed

Refactor env var parsing #3766

pellared opened this issue Feb 23, 2023 · 5 comments
Assignees

Comments

@pellared
Copy link
Member

pellared commented Feb 23, 2023

We have similar (sometimes the same) env var parsing in a few places:

I am not convinced that if we want have one package with all possible env vars like https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/internal/env/env.go. I feel such package would not be "cohesive". I would prefer to have a package with code that helps parsing env vars. We should define the env var keys and use these helper functions in packages that need these env vars.

I propose to create the env var parsing code in go.opentelemetry.io/otel/internal/env. The reason is that go.opentelemetry.io/otel/exporters/* does not have access to the current go.opentelemetry.io/otel/sdk/internal/env .

The package could have following functions (I do not think we need more):

 func String(defaultValue string, keys ...string) string // for endoints, names
 func Duration(defaultValue time.Duration, keys ...string) time.Duration  // for timeouts and intervals
 func Int(defaultValue int, keys ...string) int // for limits

PS. If anything is not clear than maybe creating a PR will be easier for me 😅

@pellared
Copy link
Member Author

@dmathieu PTAL

@pellared
Copy link
Member Author

pellared commented Feb 23, 2023

I saw #3548 and now I have concerns. This kind of refactoring may introduce the same problem in future if we do breaking changes in go.opentelemetry.io/otel/internal/env.

Maybe we should use code generation to copy the same source code into multiple packages instead of using an internal package? Alternatively, we could simply make the package "public" as go.opentelemetry.io/otel/env.

In OTel .NET SDK, to mitigate this kind of internal API incompatibility problems we include source code files into multiple C# projects (it works like a soft link).

EDIT: AFAIK there is a non-written policy that the user should use the "same" (meaning the once that were released together) version of OTel packages.

@pellared
Copy link
Member Author

pellared commented Mar 6, 2023

The current proposal is to try using code generation to make sure that modules are not coupled because of "internal dependencies".

From #3805 (comment):

Code generation that produces duplicates and handles any minor changes between copies seems like the best approach I've heard thus far. Doing that should also allow us to keep track of where duplicated code is being used.

@pellared
Copy link
Member Author

Blocked by #3846

@pellared pellared added blocked:design Waiting on design work to be completed before implementation can start. and removed blocked:design Waiting on design work to be completed before implementation can start. labels Mar 14, 2023
@pellared
Copy link
Member Author

Closing as I think the value is not worth the cost.

@pellared pellared closed this as not planned Won't fix, can't repro, duplicate, stale Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

1 participant