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

Align file configuration with latest changes to spec #6088

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

jack-berg
Copy link
Member

Notes:

  • Change env substitution syntax from ${env:MY_ENV_VAR} to ${MY_ENV_VAR}
  • Rename and combine ConfigurationReader / ConfigurationFactory to FileConfiguration.
  • Change "interpet" to "create", break out dedicate "parse", and "create" methods in addition to the convenience method "parseAndCreate".
  • Document usage in README

@jack-berg jack-berg requested a review from a team December 20, 2023 19:23
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (eea1fe0) 91.12% compared to head (5a15a63) 91.04%.
Report is 48 commits behind head on main.

Files Patch % Lines
...ension/incubator/fileconfig/FileConfiguration.java 79.16% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6088      +/-   ##
============================================
- Coverage     91.12%   91.04%   -0.09%     
+ Complexity     5736     5648      -88     
============================================
  Files           628      618      -10     
  Lines         16810    16443     -367     
  Branches       1662     1663       +1     
============================================
- Hits          15318    14970     -348     
+ Misses         1028     1010      -18     
+ Partials        464      463       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brunobat
Copy link
Contributor

brunobat commented Jan 18, 2024

@jack-berg, Will we still be able to use all functionality of the Autoconfiguration without this file based configuration?
Do we have somewhere an explanation of how the Autoconfiguration will look like after this file config is completed?

@jack-berg
Copy link
Member Author

@jack-berg, Will we still be able to use all functionality of the Autoconfiguration without this file based configuration?
Do we have somewhere an explanation of how the Autoconfiguration will look like after this file config is completed?

@brunobat we are committed to backwards compatibility, so autoconfiguration will always need to continue to work without file based configuration.

In terms of how Autoconfiguration will look after file config is completed, a lot of the reference material for that will be the spec itself. There are things that are unique to java, like the ability to customize configuration via various SPI hooks. Hard to say exactly how all the mechanisms will work with file configuration. I think some of the SPIs only make sense to fill shortcomings of the environment variable scheme, but others make sense to reuse.

I've been struggling to get engagement from the Java SIG so a lot open questions remain.

@jack-berg
Copy link
Member Author

@open-telemetry/java-approvers PTAL if possible. The changes in this PR don't introduce anything new - just changes to align with the spec. Would really like to get this merged so I can work against code / vocabulary that reflects the current state of things and avoid nasty rebases.

sdk-extensions/incubator/README.md Outdated Show resolved Hide resolved
@jack-berg jack-berg merged commit ee6c986 into open-telemetry:main Jan 23, 2024
18 checks passed
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

Successfully merging this pull request may close these issues.

3 participants