[configuration] Clarify that the boolean value environment variable guidance is not applicable to other configuration interfaces#4723
Conversation
…uidance is not applicable to other configuration interfaces
a7872e5 to
2e2811e
Compare
dashpole
left a comment
There was a problem hiding this comment.
I agree with the intent of the change, but it seems like it will create more confusion to me, as it implies (but doesn't specify) that the rest of the document applies to declarative configuration. I would prefer splitting the env var spec into a "common" config spec (excluding the boolean spec), and the env var spec with the current bool spec + anything else that only applies to env vars.
|
Makes sense, I will wait for a few more days and apply the suggested change if there is no dissent :) |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> Marks `configoptional.AddEnabledField` as beta. I have verified that it would work correctly for cookies configuration and made a PoC for keepalives (I will push a PR for it after this PR has been merged). I spent some time updating open-telemetry/opentelemetry-specification/issues/4344 and filed open-telemetry/opentelemetry-specification/pull/4723 as a way to explicitly state that the guidance only applies to environment variables. The issue remains unresolved, but given the current usages of `enabled` (see open-telemetry/opentelemetry-specification#4344 (comment)) I don't see a reason to not go forward with this. #### Link to tracking issue Updates #14021
@dashpole I pushed an attempt to split the guidance:
Other parts of the spec were using these sections as "here are the type definitions" and now the links for "type definitions" are split among two documents. We were never too consistent with this, so I feel like it is okay to do this |
|
cc @open-telemetry/configuration-approvers |
jack-berg
left a comment
There was a problem hiding this comment.
Couple small comments but I agree with the direction 👍
jack-berg
left a comment
There was a problem hiding this comment.
I prefer "configuration interfaces" over "configuration sources", but LGTM 👍
|
I think we can merge this, specially after @jack-berg gave his blessing? |
|
@mx-psi We need a final pass to make the linter happy ;)
|
That should be fixed now. There are other broken links on files not modified by this PR. I merged main into this branch to see if that would fix any of the issues |
Moved clarification about boolean environment variables open-telemetry#4723 to the SDK Configuration section.
### Context - Make the W3C randomness flag required. ([#4761](#4761)) ### Traces - Deprecate Zipkin exporter document and make exporter implementation optional. ([#4715](#4715)) - Add spec for `AlwaysRecord` sampler ([#4699](#4699)) ### Metrics - Stabilize `Enabled` API for synchronous instruments. ([#4746](#4746)) - Allow instrument `Enabled` implementation to have additional optimizations and features. ([#4747](#4747)) ### Logs - Stabilize `LogRecordProcessor.Enabled`. ([#4717](#4717)) ### SDK Configuration - Clarifies that guidance related to boolean environment variables is not applicable to other configuration interfaces. ([#4723](#4723)) --------- Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
…le negatives (#4823) (this change has been unblocked by #4723) This will help avoid double negatives (`disabled: false`) in declarative configuration, e.g. instead of [ExperimentalTracerConfigurator_kitchen_sink.yaml](https://github.com/open-telemetry/opentelemetry-configuration/blob/17d87ca756fe2cc5106d3f84be03c5dcc0184ef7/snippets/ExperimentalTracerConfigurator_kitchen_sink.yaml#L8-L15): ``` tracer_provider: tracer_configurator/development: default_config: disabled: true tracers: - name: io.opentelemetry.contrib.* config: disabled: false ``` if this PR is accepted, we would change this to: ``` tracer_provider: tracer_configurator/development: default_config: enabled: false tracers: - name: io.opentelemetry.contrib.* config: enabled: true ``` FWIW, this matches similar existing usage of enabled (over disabled) in the Java agent: - `otel.instrumentation.common.default-enabled` - `otel.instrumentation.<name>.enabled`
Updates #4344
Changes
Clarifies that environment variable guidance regarding boolean values is not applicable to other configuration interfaces. This does not explicitly state what the guidance should be in other cases.