docs: add configuration policy guidance#8429
Conversation
| | [other-tasks.md](other-tasks.md) | Dev environment setup, benchmarks, composite builds, native image tests, OTLP protobuf updates | | ||
| | File | Load when | | ||
| |------------------------------------------------|------------------------------------------------------------------------------------------------| | ||
| | [configuration-policy.md](configuration-policy.md) | Configuration changes, env vars/system properties, declarative config | |
There was a problem hiding this comment.
Looking at the comment posted by @jack-berg , this content could be a part of the api-design.md file.
There was a problem hiding this comment.
Yes I think api-design.md is appropriate. Config IS part of our API.
| Declarative config should remain a strict superset of environment variable and system property | ||
| configuration. | ||
|
|
||
| If a change adds a new environment variable or system property, it should also add equivalent |
There was a problem hiding this comment.
I would imagine this issue would require stronger language:
..., it **must** also add equivalent declarative config support.
| ## PR expectations for configuration changes | ||
|
|
||
| PRs that add configuration should include: | ||
|
|
There was a problem hiding this comment.
nit: start bullet points with Capital letters.
| PRs that add configuration should include: | ||
|
|
||
| - the motivation for introducing new configuration | ||
| - the environment variable and system property names, if any |
There was a problem hiding this comment.
nit: Is 'if any' required?
This policy would be in-effect only if the environment variables and system properties are added, correct?
|
|
||
| - the motivation for introducing new configuration | ||
| - the environment variable and system property names, if any | ||
| - the equivalent declarative config form, where applicable |
There was a problem hiding this comment.
Similarly, if declarative config is supposed to be a strict superset, IMO, the 'where applicable' should be dropped because it will always be applicable.
Let me know if you feel this is wrong.
| - the equivalent declarative config form, where applicable | ||
| - tests and documentation updates covering the new configuration path | ||
|
|
||
| ## Use judgment |
There was a problem hiding this comment.
IMO, this should be towards the top as it should be the first step when considering adding new config options.
|
|
||
| ## PR expectations for configuration changes | ||
|
|
||
| PRs that add configuration should include: |
There was a problem hiding this comment.
Suggestion:
PRs that introduce new configuration options should include:
57a49f8 to
b5df7fb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8429 +/- ##
============================================
+ Coverage 90.96% 91.02% +0.05%
- Complexity 7809 7817 +8
============================================
Files 892 893 +1
Lines 23702 23719 +17
Branches 2361 2364 +3
============================================
+ Hits 21561 21590 +29
+ Misses 1420 1408 -12
Partials 721 721 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@psx95 @jack-berg Updated — moved guidance into |
| Be judicious about adding new environment variables or system properties. | ||
|
|
||
| - Prefer existing configuration when it can already express the behavior. | ||
| - Include a clear justification for any new config key, including why existing configuration is not |
There was a problem hiding this comment.
I think by default - new config options should be declarative config only
| Breaking changes are not allowed in stable modules outside of a major version bump. The build | ||
| enforces this via japicmp — see [japicmp](#japicmp) below. | ||
|
|
||
| ## Configuration |
There was a problem hiding this comment.
I think this captures everything you have plus a few more things I think are important. Writing in paragraph style without sections allows the content to blur together as needed, rather than adhering to strictness section boundaries suggest.
Let me know what you think.
| ## Configuration | |
| ## Configuration | |
| There are [3 configuration interfaces](https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/configuration#configuration-interfaces): | |
| - Programmatic configuration: Invoke Java APIs to build components. This is the base of all other configuration interfaces. | |
| - Env var / system property configuration: Set flat env vars / system properties, which are interpreted to equivalent calls to the programmatic configuration interface | |
| - Declarative configuration: Structured, YAML based configuration, which is interpreted to equivalent calls to the programmatic configuration interface | |
| As a general rule, all configuration interfaces are bound by the spec. However, we make certain exceptions with java-specific additions in places where the lack of such features is detrimental to the OpenTelemetry Java ecosystem. For example: | |
| - The OTLP exporters have programmatic proxy configuration options, since despite not being part of the is a common enough use case that not supporting would be a glaring oversight. | |
| - Env var / system property configuration has a `otel.java.metrics.cardinality.limit` property for configuring a global cardinality limit. Not part of the spec, but an important / common enough use case that we added a java-specific property for it. | |
| Configuration options outside the spec are added at maintainers' discretion. If proposing an option outside the spec, please start with programmatic configuration, which we are much more likely to accept. | |
| Declarative configuration is stable at the spec level and was designed to solve the expressiveness shortcomings of env var / system property configuration. Its the priority interface for current and future configuration enhancements. This means: | |
| - We are judicious about extending the env var / system property configuration interface with new options. Additions are at maintainers' discretion. | |
| - Declarative configuration must be a strict superset of env var / system property configuration. In the event new env var / system properties are added, there must be equivalent capabilities in declarative config, with the caveat that naming and structure do not need to be identical. | |
| Configuration is part of our public API surface area, subject to our [versioning policy](https://github.com/open-telemetry/opentelemetry-java/blob/main/VERSIONING.md) unless explicitly marked unstable (i.e. `*.experimental.*` for env vars / system properties, `*/development` for declarative config). |
There was a problem hiding this comment.
I think this captures everything you have plus a few more things I think are important. Writing in paragraph style without sections allows the content to blur together as needed, rather than adhering to strictness section boundaries suggest.
Let me know what you think.
@jack-berg Thanks I agree, this reads better. The paragraph-style version makes the configuration model and extension guidance clearer, so I updated the section in that direction.
|
|
||
| ### PR expectations for configuration changes | ||
|
|
||
| PRs that introduce new configuration options should include: |
There was a problem hiding this comment.
This is pretty boilerplate. Most of it applies to all changes. I think we simply lay out what the configuration interfaces are, what our guidance / requirements are for extending, and let the rest be implied.
|
@jack-berg @zeitlinger Updated the configuration section to follow the paragraph-style guidance, removed the PR boilerplate, and incorporated declarative config as the default priority interface for new non-programmatic options. I also re-ran . |
|
@jack-berg @zeitlinger Updated the configuration section to follow the paragraph-style guidance, removed the PR boilerplate, incorporated declarative config as the default priority interface for new non-programmatic options, and re-ran spotlessCheck. |
Fixes #8300.
Why
What
docs/knowledge/configuration-policy.mddocumenting the configuration policydocs/knowledge/README.mdso it is discoverable from the knowledge indexTesting
./gradlew --no-daemon --max-workers=1 -Dorg.gradle.jvmargs="-Xmx512m -XX:MaxMetaspaceSize=192m" spotlessCheck