[configuration] Environment variable substitution syntax is hard to extend

[mx-psi]

The OTel SDK File configuration spec environment variable substitution is difficult to extend in a non-breaking way.

For example, adding the default syntax described on #3948 is a breaking change: currently the value must be left verbatim, while after this change it would be resolved to something. Since the spec is still experimental, breaking it is fine, but I think we should make the spec more resilient to this.

A way to approach this is to explicitly fail when a configuration file has ${...} expressions that have an invalid identifier/special characters. Turning a failure into a specific feature is not a breaking change and would allow us to extend things in the future if necessary.

If we fail on every invalid identifier, then we should have something like #3914, since there are valid cases for using ${...} expressions in other contexts (e.g. on Prometheus relabel config using ${1} is not unusual)

[jack-berg]

currently the value must be left verbatim, while after this change it would be resolved to something

Can you elaborate on why this is breaking with an example of before and after?

[mx-psi]

Sure! So, my understanding of the spec is that taking this example and assuming ENV is unset:

string_key: ${ENV:-default}

The resolved YAML before #3948 would be the same thing, since S{ENV:-default} does not match the regular expression defined on the spec:

string_key: ${ENV:-default}  # type string

The resolved YAML after #3948 would be the default string:

string_key: default  # type string

So the resolved configuration would be different on each of them. I would consider this a breaking change (one that people would rarely face but a breaking change nonetheless).

[TylerHelmuth]

+1 to failing when the configuration includes an invalid ${...} expression. Repeating my thoughts from open-telemetry/opentelemetry-collector#9854 (comment), I think failing fast is a clearer outcome for the user.

For users that mis-typed the env var name, keeping the string verbatim could mean an unintuitive error caused by the unwanted string. If we fail immediately we can log a clear error message that will be easier to troubleshoot.

If the user needs an illegal string like ${env:this-is-illegal} they can use the escape to make it exact: $${env:this-is-illegal}.

To summarize: using the env var syntax is a request to parse an env var - if the provided env var cannot be parsed we should fail.

[jack-berg]

So the resolved configuration would be different on each of them. I would consider this a breaking change (one that people would rarely face but a breaking change nonetheless).

I agree with this assessment. Edge case, but still breaking.

For users that mis-typed the env var name, keeping the string verbatim could mean an unintuitive error caused by the unwanted string. If we fail immediately we can log a clear error message that will be easier to troubleshoot.

We don't keep the string verbatim. If there is no env var set, we replace the substitution reference with empty value (see example).

Failing if the env var isn't set needs to be thought about holistically. Currently, the undefined env vars are replaced by empty values, and the spec says the following when parsing empty values:

If a field is null or unset and a default value is defined, Create MUST ensurethe SDK component is configured with the default value.

This is a nice property because it means we can define YAML like (for more examples see open-telemetry/opentelemetry-configuration#76):

tracer_provider:
  processors:
    - batch:
        exporter:
          otlp:
            protocol: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http/protobuf}
            endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318}
            certificate: ${OTEL_EXPORTER_OTLP_CERTIFICATE}
            client_key: ${OTEL_EXPORTER_OTLP_CLIENT_KEY}
            client_certificate: ${OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE}
            compression: ${OTEL_EXPORTER_OTLP_COMPRESSION:-gzip}
            timeout: ${OTEL_EXPORTER_OTLP_TIMEOUT:-10000}

Notice how the attributes which have a default specify it using the fallback syntax. But that properties like client_key have no default. This allows config to include a placeholder for a certificate without needing to define it at the moment.

Failing when an env var is referenced but is undefined narrows the usefulness of env var substitution syntax.

[jack-berg]

I think I misunderstood the goal. You're not saying to fail when an env var is referenced but unset. You're saying fail when an env var substitution expression doesn't quite match the regexp, and thus would be left as a string unless we fail.

[TylerHelmuth]

You're saying fail when an env var substitution expression doesn't quite match the regexp, and thus would be left as a string unless we fail.

Ya this one

[jack-berg]

Yeah I agree that it makes sense to fail in this case.