Add configuration analysis design documents#7093
Add configuration analysis design documents#7093stevejgordon wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
stevejgordon
force-pushed
the
config-analysis
branch
from
4c2c5fd to
e005c0a
Compare
martincostello
left a comment
martincostello
left a comment
There was a problem hiding this comment.
Only reviewed the overall analysis so far - I haven't looked at the more detailed documents yet.
| | `OTEL_CONFIG_FILE` | Core of declarative config feature | | ||
|
|
||
| **Other unimplemented:** `OTEL_LOG_LEVEL` (Medium - .NET uses EventSource), | ||
| `OTEL_EXPORTER_ZIPKIN_TIMEOUT` (Low), `OTEL_EXPORTER_PROMETHEUS_HOST`/`_PORT` |
| 8. **Two AOT violations identified.** `OtlpExporterBuilder.cs:153` uses | ||
| `services.Configure<T>(IConfiguration)` without suppression - an existing | ||
| IL2026/IL3050 bug. -> [Deep Dive | ||
| F](configuration-analysis-deep-dives.md#f-aot-compatibility-full-analysis) |
There was a problem hiding this comment.
How come this is being highlighted? Isn't it just a general observation about AoT compatibility, rather than anything to do with Declarative Config?
If it's a dependency, this can probably be done sooner rather than later to fix the issue separate to all the other DC work. At the moment it's 10th out of 11 in the list of things to do.
| | Step | What | Prerequisite | Unblocks | | ||
| | ---- | ------------------------------------------------------------------------------------------------ | ------------ | ------------------------------------ | | ||
| | 0a | `IValidateOptions<T>` + `ValidateOnStart` for existing options | None | Safe declarative config; safe reload | | ||
| | 0b | Standard `OnChange` subscriber pattern (shared infrastructure) | None | All reload subscribers | | ||
| | 0c | `TestConfigurationProvider` for reload testing | None | All reload test scenarios | |
There was a problem hiding this comment.
Maybe we should also have centralised E2E configuration tests that model the world as it is today, then we can move easily introduce declarative configuration on top of it later and it should make it easy(-ier) to see that DC is overriding the values are appropriate?
| **Open question:** Should Parse and Create be exposed as explicit public APIs, | ||
| or is the DI-embedded approach sufficient? |
There was a problem hiding this comment.
I would start with it as an internal/private thing - we can validate it in our own tests, then later we can decide if we want/need to expose it to users.
Given the DI integration and how most existing .NET configuration code works, existing service container validation via unit tests should flag issues?
Similarly, the YAML files could be validated against the published JSON schema, rather than us needing to expose an API surface to allow for that.
| **Vendored `EnvironmentVariablesConfigurationProvider`** - Manually vendored | ||
| copy in `src/Shared/EnvironmentVariables/` with no automated sync mechanism; | ||
| divergence risk. -> [Risk | ||
| 1.7](configuration-analysis-risks.md#17-the-sdks-internal-environmentvariablesconfigurationprovider-copy) |
There was a problem hiding this comment.
TIL - this looks like something that would make sense to address early on separate to this work, as this is something we should really have already.
2 participants
Contributes to #6380
Changes
Introduces several markdown design documents to a new
designsubdirectory to aid in planning configuration changes required for declarative config and future telemetry policy features. All were generated with the help of AI agents over several iterations to refine and validate the findings. I've spot checked what I can but some of the finer details may need deeper review as we tackle each area. The overall direction looks reasonable, but involves quite a lot of (non-breaking) changes. Breaking changes would simplify a few of the planned solutions, but is obviously something to avoid if we can.The summary document
configuration-analysis.mdprovide a general human-friendly overview and highlights key findings.configuration-analysis-risks.mdandconfiguration-analysis-deep-dives.mddrill into some of the detail and potential solutions for more complex blockers that were identified.configuration-proposed-issues.mdis an AI generated proposed list of sub issues that break down the required ground work and later declarative config implementation.Merge requirement checklist
[ ] Unit tests added/updated[ ] AppropriateCHANGELOG.mdfiles updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)