You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Finding (measured): the boot config loader turns an explicit false, 0 or "" in config.yaml back into the field's env-default. With otel.enabled: true and otel.traces.enabled: false in the file, config.Load returns otel.traces.enabled = true. An operator who turns a signal off in YAML gets it back on, and nothing reports it. The same value set through the WH_* env var works.
The env vars are honoured, but the fields that only the YAML set still come back as defaults.
Mechanism (inferred from cleanenv v1.5.0's source, and consistent with the measurement above):ReadConfig decodes the YAML first and then runs readEnvVars. For each field with no env var set, it applies env-default if the field is still at its zero value. At that point it cannot tell "the YAML set this to false/0/""" apart from "the YAML did not set this".
Affected fields: every env-default that is not a zero value
field
default
would an operator want the zero value?
otel.traces.enabled
true
Yes. Metrics and logs without traces is a normal setup. Measured: silently re-enabled.
otel.metrics.enabled
true
Yes. For example, when Prometheus scraping is the metrics path and OTLP carries only traces and logs. Measured: silently re-enabled.
otel.logs.enabled
true
Yes. For example, when stdout is already scraped by Loki. Measured: silently re-enabled.
otel.traces.sample_rate
1.0
Yes.0 is a valid rate, and Validate accepts [0,1]. Measured: becomes 1.0, so everything is sampled. That is the opposite of what the operator asked for, and it costs money.
otel.logs.sample_rate
1.0
Yes, for the same reason. It drops DEBUG/INFO from OTLP export. Measured: becomes 1.0.
server.shutdown_timeout
10
Plausible. Validate allows 0, which means no drain. Measured: becomes 10.
cache.l1_max_cost
67108864
Unlikely. 0 is not a usable ristretto size. Measured: silently becomes 64 MiB instead of failing.
prometheus.path
/metrics
No. An empty path is not meaningful. Only the silent rewrite matters here.
data_dir
./data
No. Empty would be a misconfiguration. Only the silent rewrite matters here.
server.port
8080
No. 0 fails Validate.
Fields whose default is already the zero value (otel.enabled, prometheus.enabled, prometheus.port, clickhouse.max_total_conns) are not affected.
Impact (inferred): the worst cases are the three OTel signal toggles and the two sample rates. An operator who turns telemetry down in YAML gets full-rate export, with extra egress and backend cost. No error or warning appears, and config.yaml says the opposite of what is running. This will also affect every future field that gets a non-zero env-default. PR #630 has already had to work around it for cache.redis.compress_min_bytes: -1 means "never compress" because a 0 in the file reads as unset. Each new knob would need a similar workaround.
Fix direction
Any of these would work. The first is the smallest change:
Apply defaults before the decode, and drop env-default. Build the Config with its defaults in Go, decode the YAML over it, and then apply WH_* overrides. A key the YAML sets, including to false/0, wins over the default. This also removes the need for PR feat(config): cache.backend=redis selects the shared cache #630's -1 sentinel.
A pre-pass that records which keys the YAML set.rejectUnknownKeys already walks the YAML with yaml.v3. Collect the set paths there, and after cleanenv runs, restore the zero value for any key the file set explicitly.
Pointer fields (*bool, *float64) for the affected knobs, so nil can be told apart from an explicit zero. This is more invasive and spreads nil checks to every consumer.
Whichever fix is chosen, add a regression test that loads a YAML with otel.traces.enabled: false and otel.traces.sample_rate: 0 through Load, and asserts the loaded values rather than a hand-built struct.
Finding (measured): the boot config loader turns an explicit
false,0or""inconfig.yamlback into the field'senv-default. Withotel.enabled: trueandotel.traces.enabled: falsein the file,config.Loadreturnsotel.traces.enabled = true. An operator who turns a signal off in YAML gets it back on, and nothing reports it. The same value set through theWH_*env var works.Repro (measured,
main@ 93d8019, cleanenv v1.5.0)I copied the non-test files of
internal/configinto a scratch module that uses the repo'sgo.mod, then called the realconfig.Loadon this file:With no
WH_*env set:otel.enabled: truewas read, because its default isfalse. Every other value that the YAML set to its zero value came back as the default.With
WH_OTEL_TRACES_ENABLED=false WH_OTEL_TRACES_SAMPLE_RATE=0 WH_CACHE_L1_MAX_COST=0 WH_SERVER_SHUTDOWN_TIMEOUT=0on the same file:The env vars are honoured, but the fields that only the YAML set still come back as defaults.
Mechanism (inferred from cleanenv v1.5.0's source, and consistent with the measurement above):
ReadConfigdecodes the YAML first and then runsreadEnvVars. For each field with no env var set, it appliesenv-defaultif the field is still at its zero value. At that point it cannot tell "the YAML set this tofalse/0/""" apart from "the YAML did not set this".Affected fields: every
env-defaultthat is not a zero valueotel.traces.enabledtrueotel.metrics.enabledtrueotel.logs.enabledtrueotel.traces.sample_rate1.00is a valid rate, andValidateaccepts[0,1]. Measured: becomes1.0, so everything is sampled. That is the opposite of what the operator asked for, and it costs money.otel.logs.sample_rate1.01.0.server.shutdown_timeout10Validateallows0, which means no drain. Measured: becomes10.cache.l1_max_cost671088640is not a usable ristretto size. Measured: silently becomes 64 MiB instead of failing.prometheus.path/metricsdata_dir./dataserver.port80800failsValidate.Fields whose default is already the zero value (
otel.enabled,prometheus.enabled,prometheus.port,clickhouse.max_total_conns) are not affected.Impact (inferred): the worst cases are the three OTel signal toggles and the two sample rates. An operator who turns telemetry down in YAML gets full-rate export, with extra egress and backend cost. No error or warning appears, and
config.yamlsays the opposite of what is running. This will also affect every future field that gets a non-zeroenv-default. PR #630 has already had to work around it forcache.redis.compress_min_bytes:-1means "never compress" because a0in the file reads as unset. Each new knob would need a similar workaround.Fix direction
Any of these would work. The first is the smallest change:
env-default. Build theConfigwith its defaults in Go, decode the YAML over it, and then applyWH_*overrides. A key the YAML sets, including tofalse/0, wins over the default. This also removes the need for PR feat(config): cache.backend=redis selects the shared cache #630's-1sentinel.rejectUnknownKeysalready walks the YAML withyaml.v3. Collect the set paths there, and after cleanenv runs, restore the zero value for any key the file set explicitly.*bool,*float64) for the affected knobs, so nil can be told apart from an explicit zero. This is more invasive and spreads nil checks to every consumer.Whichever fix is chosen, add a regression test that loads a YAML with
otel.traces.enabled: falseandotel.traces.sample_rate: 0throughLoad, and asserts the loaded values rather than a hand-built struct.