[Config] Declarative config (phase 1) initial design and scalar values#7413
[Config] Declarative config (phase 1) initial design and scalar values#7413stevejgordon wants to merge 26 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7413 +/- ##
==========================================
+ Coverage 90.05% 90.40% +0.34%
==========================================
Files 284 301 +17
Lines 15230 15834 +604
==========================================
+ Hits 13716 14315 +599
- Misses 1514 1519 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
0003250 to
f9de4e2
Compare
martincostello
left a comment
There was a problem hiding this comment.
Overall approach here looks really good to me.
Introduces limited support for scalar values in the declarative configuration, and adds the necessary plumbing to read configuration from YAML files and environment variables. The implementation is still incomplete, and the feature is not yet enabled by default. Future commits will add support for more complex configuration scenarios, such as lists and dictionaries, and construction of components via factory methods.
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
…iguration.Declarative.csproj Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
…cs/OpenTelemetryDeclarativeConfigurationEventSource.cs Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
…figPropertyState.cs Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
…DeclarativeConfigurationConverter.cs Co-authored-by: Martin Costello <martin@martincostello.com>
…DeclarativeConfigurationConverter.cs Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
…ronmentSubstitution.cs Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
dfaf22a to
8d0d4d7
Compare
8d0d4d7 to
894aa7c
Compare
|
@martincostello All feedback should now be addressed. |
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This looks pretty good and it's great to see declarative config work in the .NET SDK.
Would you consider splitting this before it goes out of draft? Even setting tests aside (which are most of the diff), the 2k lines of source to add the package, the YAML reader, env substitution, the model, the flat-key converter, and the IConfiguration overlay wiring all at once is overwhelming.
PRs of this size are hard to review with confidence and expect a reviewer to hold the whole pipeline in their head.
I think breaking into these broader areas would be helpful:
- package skeleton + public API surface + OTEL1006 plumbing
- YAML parse to typed model (reader, substitution, file-format validation)
- model to IConfiguration overlay (converter + DI wiring + precedence)
| /// </summary> | ||
| /// <param name="config">The typed configuration model to convert.</param> | ||
| /// <param name="data">Dictionary to populate with flat key/value pairs.</param> | ||
| internal static void Convert(DeclarativeConfiguration config, IDictionary<string, string?> data) |
There was a problem hiding this comment.
Maybe more a question of part 2 and not this PR.
Convert emits resource as a flat OTEL_RESOURCE_ATTRIBUTES key. Once component providers build resource (and sampler, id_generator) structurally, those fields get two code paths that can both apply and fight, with registration order picking the winner.
If a section is consumed structurally in part 2, would suppressing its flat emission here so there's one path per field be cleaner? Is that how you're planning the part 1 / part 2 boundary?
There was a problem hiding this comment.
That is a failure mode we need to avoid once a field has both a flat projection and a structural implementation. I'm expecting that shape will shake out more in phase 2.
For resource specifically, phase 2 does not move resource.attributes onto the component-provider path. In the schema, resource.attributes and attributes_list are static resource configuration; the resource plugin-provider shape is under resource.detection/development. I have deferred that experimental detection path for now. Are you aware of any other parts of the spec that maybe I missed?
So for phase 1 and my WIP phase 2 there is still only one path for resource attributes: projection to OTEL_RESOURCE_ATTRIBUTES, reusing the SDK’s existing resource handling. If/when we add structural application via ResourceBuilder, then yes, we should suppress the flat emission for that section. The goal in phase 1 was to avoid changing the consumption of the configuration unless required, and this seemed like a good way to do that, for now.
The Sampler component provider in phase 2 is slightly different. The sampler component is built structurally via the registry, but the ratio is also projected to an internal reloadable IConfiguration key because I hoped to support and prove the capability to have reloadable configuration. That projection feeds live value reload; it is not a competing sampler construction path. Suppressing it would break the reload story.
Longer term, the overlay mode still needs clear per-field ownership rules so flat projection and structural creation do not compete. For any section we consume structurally, we should either suppress the corresponding flat projection or make the flat value an intentional implementation detail, as with the sampler reload key.
|
|
||
| // TODO(strict-mode): branch here on a future DeclarativeConfigurationMode (Default vs Strict). See https://github.com/open-telemetry/opentelemetry-dotnet/issues/6380. | ||
|
|
||
| // Last registered IConfiguration wins in DI. |
There was a problem hiding this comment.
The SDK config spec indicated the file is authoritative when OTEL_CONFIG_FILE is set, meaning other env vars should be ignored. Here the file is just another IConfiguration layer and a later appsettings.json or AddXxx call override it / change behaviour. This is in direct opposition to the spec.
I see there is a proposed strict-mode, but it's gated behind a future mode, so the default behaviour is to overlay.
What is the motivation to allow layering?
There was a problem hiding this comment.
@MikeGoldsmith - This design was taken as it's the idoimatic approach to configuration in .NET. Each source builds on the previously registered sources. Breaking from that may be surprising to .NET developers used to this pattern. I went this way, based on this comment in the isse (see also https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md?plain=1#L342-L343).
|
I'm happy to defer if the consensus is to split this up, but as I've already reviewed it all I'd rather not review three new PRs that are all the same code 😄 (even if they're the same, still got to review them). |
fabc540 to
e6bda43
Compare
| Implements the OpenTelemetry declarative-configuration specification | ||
| as an IConfigurationSource/IConfigurationProvider.</Description> | ||
| <PackageTags>$(PackageTags);configuration;declarative;yaml</PackageTags> | ||
| <MinVerTagPrefix>coreunstable-</MinVerTagPrefix> |
There was a problem hiding this comment.
A way to avoid this being released for now might be to use a different new MinVerTagPrefix temporarily so it doesn't match either core or coreunstable and thus won't get released.
Then once it's "good enough" for a prerelease, we update the tag back to coreunstable?
Would that work @Kielek?
There was a problem hiding this comment.
I agree that we are far behind releasing this as beta.
I would consider putting here very specific tag prefix.
Eg. declarativeconfig- or corealpha-.
I doubt that we can release this as a beta shortly. When we will be ready for this, we can switch to coreunstable.
Some other technical stuff probably needs to be adjusted. Full search of coreunstable could help with this.
|
@martincostello I think I've addressed the follow up feedback. e6bda43aeae5bb64f95f2b2d21f84eab1571fa75 is worth a look. I realised that there's actually a 1.1 schema already which the prior code wouldn't have functioned with. Since no .NET apps would be using the pre-release schema's, I'd dropped support for anything prior to 1.0 and made it more lenient so we can support new minors as they come along and we implement any new elements. For now, I'm letting it support both 1.0 and 1.1 schema as our surface is in both. I expect we'd aim to support all of 1.1 that we can before going stable. |
|
The one failure looks unrelated and is perhaps flaky but I can revisit if that assumption is incorrect. |
Co-authored-by: Martin Costello <martin@martincostello.com>
martincostello
left a comment
There was a problem hiding this comment.
LGTM - just the question regarding publishing needs resolving.
Kielek
left a comment
There was a problem hiding this comment.
Only technical check.
Sorry for late feedback, still need more time to understand full flow for this component,
| @@ -0,0 +1,192 @@ | |||
| # OpenTelemetry .NET Diagnostic: OTEL1006 | |||
There was a problem hiding this comment.
I do not think that we need this at all. As long as we stays on the prerelease channels.
Especially alpha means that anything can change.
The experimental apis feature should be used to expose unstable public API in the already stable packages to indicate that some features will not work with stable releases.
| @@ -0,0 +1,11 @@ | |||
| #nullable enable | |||
There was a problem hiding this comment.
This line is redundant. Nullable is already in shipped.
There was a problem hiding this comment.
This is just a directive for the analyser to understand if the entries are nullable or not. If you removed it, when a new API that is unshipped is added to the file the line would be needed to be added again. It's not about whether nullable as a feature shipped.
There was a problem hiding this comment.
It should inherit this information from Shipped folder. It is not needed to duplicate this info. Not a blocker.
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Contibutes to #6380
Changes
This PR introduces the OpenTelemetry.Configuration.Declarative package - a partial, experimental implementation of the OTel declarative configuration specification for the .NET SDK.
The implementation is gated behind experimental diagnostic
OTEL1006and is scoped to a working subset offile_format: "1.1"to establish the integration pattern before tackling a more complete implementation including component providers.The main experiemental public API is exposes are extension methods on
IConfigurationBuilderto register declarative configuration as a source of .NET configuration. It also introduces extension methods forIOpenTelemetryBuilderto allow registering declarative config directly on the OTel SDK builder, which then registers on the host'sIConfiguration.There is also a specific exception type
DeclarativeConfigurationExceptiondefined for fast-failures during file validation and YAML parsing.In it's current state, the implementation acts in "overlay" mode, composing with the existing configuration sources in a .NET idomatic manner. The YAML file is projected onto flat env-var-style keys (e.g.
resource.attributestoOTEL_RESOURCE_ATTRIBUTES) and fed into the .NETIConfigurationpipeline. The SDK then picks values up via its existingIConfigurationreaders, with no changes required to existing SDK components. This avoids introducing a parallel configuration system but limits coverage to what is already expressible as env-var equivalents.The YAML source is appended after all sources already registered at the time of the call. It therefore overrides env vars and appsettings.json by default, but any source added afterward still wins. This is technically allowable per the spec. A future "strict mode" could restrict configuration to only the declarative config file.
Scalar values support
${VAR},${env:VAR},${VAR:-default}, and$$per the OTel spec. Substitution runs post-parse, so env var values cannot inject YAML structure.Supported fields (part 1 scope):
Extensive diagnostics are introduced via
EventSourceto make supporting the feature easier.If this initial design looks acceptable, then we can build on this. I have started work on part 2 which extends the design to support component providers used to construct components such as samplers, processors etc.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes