elastic / elastic/apm-agent-nodejs
chore: proper fallback value for boolean "bogus" options
- Dominant language
- JavaScript
- Stars
- 594
- Forks
- 244
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 16
Description
# Description
When refactoring our configuration tests to run them in separate processes (#3762) an unwanted behaviour was discovered when a "bogus" boolean value is set in the configuration. This is when the value is not any of `true`, `false`, `"true"` or `"false"`.
Regarding bogus values the spec says
```
If an invalid value for a configuration option is provided (for example: breakdown_metrics="yes" or apiRequestTime="1h") then the agent MUST ignore the value (falling back to a config source with lower precedence) and SHOULD emit a log warning about the ignored value.
```
The node agent logs the warning but instead of falling back to a config source with lower precedence the value gets resolved to `undefined` ignoring sources of lower precedence like the defaults. As an example this behaviour with defaults would be okay for the options with `false` value as default (contextPropagationOnly, disableSend, errorOnAbortedRequests, opentelemetryBridgeEnabled, useElasticTraceparentHeader, usePathAsTransactionName) but not for options with dafault value set to `true` (active, breakdownMetrics, captureExceptions, captureHeaders, centralConfig, instrument, instrumentIncomingHTTPRequests, spanCompressionEnabled, verifyServerCert).
As a example we have this test that sets a bogus value for `ELASTIC_APM_ACTIVE` env var. this value wins over the other sources and when resolved we get `undefined` resulting in the agent being inactive.
https://github.com/elastic/apm-agent-nodejs/blob/0e3e002efbba39311e6c0e3c55192e3eb7fdf4f2/test/config/config.test.js#L345
# Possible approach
The agent should keep reference of the options provided from all sources instead of squashing them into a single object. then the normalization process should be aware of these sources to do the proper fallback if one option has a bogus value.
NOTE: as discussed internally this will be actioned when the test refactoring is done
Contributor guide
Assessment
This issue has not been assessed yet.