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

File configuration #1249

Merged
merged 15 commits into from Apr 1, 2024
Merged

Conversation

Nevay
Copy link
Contributor

@Nevay Nevay commented Mar 5, 2024

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 and ServiceLoader::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.

@brettmc
Copy link
Collaborator

brettmc commented Mar 6, 2024

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 spi) in the appropriate composer.json, rather than globally registering everything?

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 spi and otel-sdk-configuration require ^8.1)

@Nevay
Copy link
Contributor Author

Nevay commented Mar 6, 2024

I added a configuration example that uses env substitution. We cannot fully replicate the environment variable configuration:

  • variables with list/map value cannot be used, injecting YAML structures is disallowed by spec, e.g. OTEL_PROPAGATORS, OTEL_RESOURCE_ATTRIBUTES
  • variables that select a specific implementation do not map to the configuration format, e.g. OTEL_TRACES_SAMPLER, OTEL_{signal}_EXPORTER
  • variables with fallback behavior are not supported by the environment variable substitution definition, e.g. OTEL_EXPORTER_OTLP_{signal}_*

does it make sense for the various component providers to live alongside the component and be registered (via spi) in the appropriate composer.json, rather than globally registering everything?

I'm leaning towards yes; for extensions that are not defined by opentelemetry-configuration definitely. I mainly kept it all in a separate package for now due to the PHP8.1 requirement.

FWIW I have been using an Env\Loader interface for env configuration which could replace the current Registry + SpanExporterFactory/MetricExporterFactory etc. if we want to use the same registration mechanism for both formats.

interface Loader {

    public function load(EnvResolver $env, LoaderRegistry $registry, Context $context): mixed;

    public function name(): string;
}

@brettmc
Copy link
Collaborator

brettmc commented Mar 13, 2024

I think we can merge this. Let's annotate it as experimental, and then I'll try to apply my config PR over the top.

edit: the SPI package looks nicely self-contained, but thinking about future maintainability, could the important parts of otel-sdk-configuration come into the opentelemetry org?

@Nevay
Copy link
Contributor Author

Nevay commented Mar 14, 2024

We should wait until #1256 is merged, I think most workflows will fail otherwise.

could the important parts of otel-sdk-configuration come into the opentelemetry org?

Yes (but it should stay independent from the SDK); should it be added to this repository?

@brettmc
Copy link
Collaborator

brettmc commented Mar 14, 2024

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 Config directory, or a new one if you feel it doesn't belong in config.

#1256 has been merged, now.

@Nevay Nevay force-pushed the draft/file-configuration branch 2 times, most recently from bed6ea3 to 7157247 Compare March 15, 2024 21:53
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 524 lines in your changes are missing coverage. Please review.

Project coverage is 77.24%. Comparing base (4cb0499) to head (ae980ec).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             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     
Flag Coverage Δ
8.1 77.24% <0.00%> (-7.35%) ⬇️
8.2 77.24% <0.00%> (-7.35%) ⬇️
8.3 77.24% <0.00%> (-7.35%) ⬇️
8.4 77.24% <0.00%> (-7.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/Contrib/Otlp/Protocols.php 0.00% <ø> (ø)
...omponentProvider/Metrics/MetricExporterConsole.php 0.00% <0.00%> (ø)
...mponentProvider/Propagator/TextMapPropagatorB3.php 0.00% <0.00%> (ø)
...ntProvider/Propagator/TextMapPropagatorB3Multi.php 0.00% <0.00%> (ø)
...ntProvider/Propagator/TextMapPropagatorBaggage.php 0.00% <0.00%> (ø)
...entProvider/Propagator/TextMapPropagatorJaeger.php 0.00% <0.00%> (ø)
...vider/Propagator/TextMapPropagatorTraceContext.php 0.00% <0.00%> (ø)
...g/SDK/ComponentProvider/Trace/SamplerAlwaysOff.php 0.00% <0.00%> (ø)
...ig/SDK/ComponentProvider/Trace/SamplerAlwaysOn.php 0.00% <0.00%> (ø)
...entProvider/Metrics/AggregationResolverDefault.php 0.00% <0.00%> (ø)
... and 17 more

... and 13 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cb0499...ae980ec. Read the comment docs.

@bobstrecansky
Copy link
Collaborator

@Nevay - are you still planning on working on this PR?

@Nevay
Copy link
Contributor Author

Nevay commented Mar 27, 2024

Could you add an example of how a component could be configured using environment variables?

open-telemetry/opentelemetry-configuration#76 provides a starter template that contains all env vars that map cleanly to file config.

are you still planning on working on this PR?

Yes; but not sure when I will find time to move otel-sdk-configuration to this repository.
Currently the packages:validate:installation check is failing: we can either copy all data from the package composer.json to the root composer.json and remove the path repository or try to fix the validation check. There are some TODOs that require SDK changes -> we should likely disable these configuration options for now. Besides that this PR should be ready for review.

@Nevay Nevay marked this pull request as ready for review March 27, 2024 21:38
@Nevay Nevay requested a review from a team as a code owner March 27, 2024 21:38
@brettmc
Copy link
Collaborator

brettmc commented Mar 28, 2024

Currently the packages:validate:installation check is failing

@Nevay submitted a PR against your branch which makes this command happy (per your first suggestion)

Copy link
Collaborator

@brettmc brettmc left a 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.

@brettmc brettmc changed the title [Draft] File configuration File configuration Apr 1, 2024
@brettmc brettmc merged commit 7ec0e90 into open-telemetry:main Apr 1, 2024
8 of 9 checks passed
@Nevay Nevay deleted the draft/file-configuration branch April 1, 2024 20:49
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.

None yet

3 participants