Skip to content

File configuration#1249

Merged
brettmc merged 15 commits into
open-telemetry:mainfrom
Nevay:draft/file-configuration
Apr 1, 2024
Merged

File configuration#1249
brettmc merged 15 commits into
open-telemetry:mainfrom
Nevay:draft/file-configuration

Conversation

@Nevay

@Nevay Nevay commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

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

brettmc commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

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

Nevay commented Mar 6, 2024

Copy link
Copy Markdown
Contributor Author

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

brettmc commented Mar 13, 2024

Copy link
Copy Markdown
Contributor

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

Nevay commented Mar 14, 2024

Copy link
Copy Markdown
Contributor Author

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

brettmc commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

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
@Nevay
Nevay force-pushed the draft/file-configuration branch from 7157247 to e617606 Compare March 15, 2024 21:55
@codecov

codecov Bot commented Mar 15, 2024

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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

@Nevay

Nevay commented Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

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 March 27, 2024 21:38
@brettmc

brettmc commented Mar 28, 2024

Copy link
Copy Markdown
Contributor

Currently the packages:validate:installation check is failing

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

@brettmc brettmc left a comment

Copy link
Copy Markdown
Contributor

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
@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.

3 participants