-
Notifications
You must be signed in to change notification settings - Fork 947
feat(opentelemetry-configuration): add more attributes to config model #5862
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5862 +/- ##
==========================================
+ Coverage 95.02% 95.10% +0.07%
==========================================
Files 315 315
Lines 8225 8396 +171
Branches 1666 1727 +61
==========================================
+ Hits 7816 7985 +169
- Misses 409 411 +2
🚀 New features to boost your workflow:
|
experimental/packages/opentelemetry-configuration/src/EnvironmentConfigProvider.ts
Show resolved
Hide resolved
experimental/packages/opentelemetry-configuration/src/EnvironmentConfigProvider.ts
Show resolved
Hide resolved
|
Hey! Sorry for the late review. So long story short, the Specification defines:
As an example, in the Java ecosystem 1) and 2) exist as separate packages, although 1) uses 2) if it is present at runtime (if it detects the proper env var setting the configuration file). But that's mostly an implementation detail and is up to you to define them in the same or in separate packages, and it is your decision what to do organization wise. The PR looks fine otherwise! Let me know if this clarifies a little bit the current situation. |
|
Thank you for your input @carlosalberto! I talked with the group defining the spec for the config and we discussed how my implementation differs from the Java (two places vs having everything in one), and they actually said they like the way I'm doing and wished the others were doing it, so I agree is a matter of implementation, and the way I'm doing is still aligned with the specs I'm trying to break down the PRs as most as possible to make it easier for review, so this one is still preparing things, are handling env variables, there is another one that starts the actual parsing of a config file, that you can find here #5875 |
|
Symbolic approval, as I'm not an approver (went through the validation code and it all looks good). |
Part of #5826
This PR focus on adding all attributes and corresponding setup of all environment variables required to migrate from Env Variable approach to a Declarative Config approach, based on the environment variables used here