[metrics] MeterConfig disabled does not follow Collector enabled convention

[mx-psi]

The MeterConfig's disabled parameter (introduced in #3877) conflicts with the Collector convention which is to use enabled (this is not documented, this search shows many places where this is used in the YAML configuration). Using enabled should be considered for consistency across the whole project.

Another argument against disabled: false is that it has a double negative which is harder to parse. IIRC this is the reason we use enabled instead in the Collector configuration.

disabled: false however is more idiomatic for Go because of false being the zero value of booleans.

cc @jack-berg @codeboten

[marcalff]

The fact that boolean environment variables have a default of false comes from the spec.

See this conversation:

• Definition of boolean environment variables on the SDK #2755 (comment)

[mx-psi]

@marcalff Thanks for the link! This is not an environment variable though, I am not sure I see the relation

[jack-berg]

We've carried the boolean env var convention into the naming / defaults of component properties. The boolean env var convention is sensible enough, and following it when introducing new component properties (i.e. not env vars) is the path of least resistance for contributors.

Another example of following this guidance where the property doesn't have a standard env variable are these prometheus exporter properties: without_units, without_type_suffix, without_scope_info, without_target_info

This is not an environment variable though, I am not sure I see the relation

By this logic, the collector YAML config you reference is unrelated theMeterConfig.disabled, since YAML config != programmatic config 😉. I think the fact of the matter is that programmatic config, YAML config, and env var config are all intertwined with each other, and conventions that apply to one tend to carry into others.

For example, we carry many of the characteristics of programmatic and env var properties into declarative config, including naming and defaults.

[mx-psi]

By this logic, the collector YAML config you reference is unrelated theMeterConfig.disabled, since YAML config != programmatic config 😉. I think the fact of the matter is that programmatic config, YAML config, and env var config are all intertwined with each other, and conventions that apply to one tend to carry into others.

I guess the point I didn't understand is that we still intend to add environment variables to configure the SDK even when we have the declarative configuration. If that were not the case, then it could make sense to not consider the "env argument".

I think this only makes sense to reconsider if:

1. This is the first disabled example
2. We don't intend to add further environment variables

I suspect (2) is not the case, but could you confirm?

[pellared]

conflicts with the Collector convention which is to use enabled

Isn't the case for the collector that all usages of enabled are false by default so the user has to explicitly enable them? If so then Collector also follows the spec:

The fact that boolean environment variables have a default of false comes from the spec.

[mx-psi]

Isn't the case for the collector that all usages of enabled are false by default so the user has to explicitly enable them? If so then Collector also follows the spec:

No, for example these are all enabled by default:

• https://github.com/open-telemetry/opentelemetry-collector/blob/4cf491babd7c019f4bd534ce4f5e016073b1d1c1/exporter/exporterqueue/config.go#L30
• https://github.com/open-telemetry/opentelemetry-collector/blob/4cf491babd7c019f4bd534ce4f5e016073b1d1c1/config/configretry/backoff.go#L16
• https://github.com/open-telemetry/opentelemetry-collector/blob/4cf491babd7c019f4bd534ce4f5e016073b1d1c1/exporter/exporterhelper/internal/queue_sender.go#L43
• https://github.com/open-telemetry/opentelemetry-collector/blob/4cf491babd7c019f4bd534ce4f5e016073b1d1c1/exporter/exporterbatcher/config.go#L64

[pellared]

Another argument against disabled: false is that it has a double negative which is harder to parse. IIRC this is the reason we use enabled instead in the Collector configuration.

For the same reason the OpenTelemetry .NET Automatic Instrumentation has _ENABLED env vars even if the default value is true. E.g. OTEL_DOTNET_AUTO_RESOURCE_DETECTOR_ENABLED. See: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/main/docs/config.md. We got enough feedback to not go with the _DISABLED env vars.

[trask]

my 2c from #2755 (comment):

I prefer *enabled flags over *disabled flags (even if that means the default value is true), and we use this pattern for all boolean flags in the otel java agent

[jack-berg]

So there are two questions that seem relevant to answer:

1. Do we want to re-litigate the boolean env var naming convention decided in Definition of boolean environment variables on the SDK #2755?
2. Do we like following the env var guidance in the programmatic configuration interface?

From my perspective, having different conventions for different configuration interfaces (env vars vs. programmatic) is a bad design. I'd like to see us consistent (to the extent it makes sense), which here seems to mean either accepting the convention, or re-litigating the discussion. On re-ligating: the environment variable spec doc along with the boolean convention is stable. The specific sentence that we're arguing about uses normative "SHOULD". Coupled with the fact that there are very few env vars which follow this advice, I think there's technically some wiggle room to reverse course.

All Boolean environment variables SHOULD be named and defined such that false is the expected safe default behavior. Renaming or changing the default value MUST NOT happen without a major version upgrade.

With that said, I don't think re-litigating this is a good use of time, so I would personally not participate in that discussion (or block any result).

[mx-psi]

From my perspective, having different conventions for different configuration interfaces (env vars vs. programmatic) is a bad design

We are not just building the specification, we are also building the Collector, the Java Agent, the .NET instrumentation... I think based on your argument, any option is a bad design for OpenTelemetry as a whole, since we are bound to have an inconsistency somewhere (be it within the specification or within the wider official OTel ecosystem).

[jack-berg]

I think based on your argument, any option is a bad design for OpenTelemetry as a whole, since we are bound to have an inconsistency somewhere (be it within the specification or within the wider official OTel ecosystem).

Any inconsistency is bad design, IMO. But the degree of bad depends on the component. I.e. its more problematic if we're not even be able to maintain consistency between configuration interfaces defined within the spec. Ideally, SIGs which are not explicitly bound to the guidance of the specification reach the conclusion that consistency is important.

[trask]

just sharing a data point, I suspect we won't change OTEL_INSTRUMENTATION_[NAME]_ENABLED in the Java agent, since some of the instrumentations are enabled by default and some are disabled by default, and so it would feel weird (and inconsistent in a different way) to have some specific instrumentations using OTEL_INSTRUMENTATION_[NAME]_ENABLED and other specific instrumentations using OTEL_INSTRUMENTATION_[NAME]_DISABLED

(and an additional wrinkle is that we have occasionally changed the default for specific instrumentations from enabled to disabled in major version bumps)