-
Notifications
You must be signed in to change notification settings - Fork 191
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
File configuration #1249
File configuration #1249
Conversation
I really like this. Could you add an example of how a component could be configured using environment variables? If we can do that, then it looks like this could completely replace the existing SDK configuration code. I understand that this is a draft, but does it make sense for the various component providers to live alongside the component and be registered (via P.S. I'm proposing at SIG this week to soon drop 8.0 support, given its very low usage and being out of official support. That might make some further improvements possible (and I notice that |
I added a configuration example that uses env substitution. We cannot fully replicate the environment variable configuration:
I'm leaning towards yes; for extensions that are not defined by FWIW I have been using an interface Loader {
public function load(EnvResolver $env, LoaderRegistry $registry, Context $context): mixed;
public function name(): string;
} |
I think we can merge this. Let's annotate it as edit: the SPI package looks nicely self-contained, but thinking about future maintainability, could the important parts of |
We should wait until #1256 is merged, I think most workflows will fail otherwise.
Yes (but it should stay independent from the SDK); should it be added to this repository? |
I think it should be in this repository, it doesn't feel right for a core component to depend on a contrib one. I think it can either be bundled into the #1256 has been merged, now. |
[skip ci]
Default value available -> cannot be null.
bed6ea3
to
7157247
Compare
7157247
to
e617606
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1249 +/- ##
============================================
- Coverage 84.58% 77.24% -7.35%
- Complexity 2136 2210 +74
============================================
Files 284 321 +37
Lines 6054 6628 +574
============================================
- Hits 5121 5120 -1
- Misses 933 1508 +575
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@Nevay - are you still planning on working on this PR? |
open-telemetry/opentelemetry-configuration#76 provides a starter template that contains all env vars that map cleanly to file config.
Yes; but not sure when I will find time to move |
@Nevay submitted a PR against your branch which makes this command happy (per your first suggestion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm happy to merge it as-is. I'll create an issue to port otel-sdk-configuration in.
Implements an extensible file configuration format to support opentelemetry-configuration. It uses
symfony/config
for creation and validation of the configuration schema.New components can be added by implementing
ComponentProvider
andServiceLoader::register()
ing them. An example that provides additional samplers can be seen at https://github.com/Nevay/otel-sdk-contrib-sampler (not compatible with this SDK).Opening this now as #1248 should take future file configuration support into account.