Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support default values for configuration environment variables #5228

Closed
Mario-Hofstaetter opened this issue Apr 15, 2022 · 15 comments · Fixed by #10907
Closed

Support default values for configuration environment variables #5228

Mario-Hofstaetter opened this issue Apr 15, 2022 · 15 comments · Fixed by #10907

Comments

@Mario-Hofstaetter
Copy link

Is your feature request related to a problem? Please describe.

When using Configuration Environment Variables,

if a variable is NOT SET on otelcol start, the value will be empty and probably break configuration.

Please provide a way to set default values in case ENV variables are not available.

Describe the solution you'd like

Allow definition of a default value if an ENV variable is not set. Via ${VAR:default_value} , example:

processors:
  attributes/example:
    actions:
      - key: "${DB_KEY:CustomerDefaultKey}"
        action: "${OPERATION:CustomerDefaultOperation}"

This mimics the syntax of grafana promtail

Another example is docker-compose , where "Default values can be defined inline using typical shell syntax"
https://github.com/docker/docker.github.io/blob/db650b4772f605683ba4143d0b0de44d0a187db4/compose/compose-file/index.md#interpolation

  • ${VARIABLE:-default} evaluates to default if VARIABLE is unset or
    empty in the environment.
  • ${VARIABLE-default} evaluates to default only if VARIABLE is unset
    in the environment.

and more, which is more complex however.

Describe alternatives you've considered

For example, grafana allows to Override configuration with environment variables

For OTEL Collector, this could look like this. Given this configuration file (section):

exporters:
  jaeger:
    endpoint: my_default_server:14250

Providing an environment variable crafted like so:

export OTELCOL_EXPORTERS_JAEGER_ENDPOINT=some_other_server:23456

Could overwrite the value from default configuration or configuration files.

(Grafana is supporting even more variable substituation ways, but this is certainly overkill for OTEL Collector.)

At the moment, the configuration file (including ENV vars within the file) is the only way to configure OTEL Collector (?), which may be a deliberate design choice. It is not possible to overwrite specific configuration keys by using:

  • CLI Flags
  • ENV variables directly addressing those keys (so described approach above)

Which is not bad by all means, since mixing all those possiblities creates complex and confusing configuration.

However ENV variables and CLI Flags are easier to use when running OtelCol as a container, in comparison to a config file. When not running on kubernetes opentelemetry-operator is no alternative.

@bogdandrutu
Copy link
Member

It is not possible to overwrite specific configuration keys by using:

This is wrong, look into "yaml:" provider (https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/mapprovider/yamlmapprovider/mapprovider.go) you can do something like:

--config=file:my_file --config="yaml:service::telemetry::metrics::address: localhost:8000"

Or using the --set flag.

Allow definition of a default value if an ENV variable is not set. Via ${VAR:default_value}

We will follow the same model as defined by our providers so will be something like ${env:FOO?default=bar} or something like that. I would like to see a PR that changes the envmapprovider to support default value.

@jpkrohling
Copy link
Member

This is wrong, look into "yaml:" provider

Is this documented somewhere? This does deserve to be available in user-facing docs. I can write that doc if we have readme files available for all the supported providers.

@neelayu
Copy link
Contributor

neelayu commented May 20, 2022

@bogdandrutu

We will follow the same model as defined by our providers so will be something like ${env:FOO?default=bar} or something like that. I would like to see a PR that changes the envmapprovider to support default value.

Correct me if I am wrong, but the envmapprovider is basically a replacement for file or yaml map providers, i.e you can have a partial or full configuration stored in an environment variable which you can specify in the --config flag.
What the need is to support the the substitution in expandmapconverter, similar to what we had in #4458

I believe these are two different provisions.
If you agree, I can re-open the previous PR to fit this use-case.

@bogdandrutu
Copy link
Member

bogdandrutu commented Jun 2, 2022

We are very close to propose for merging #4742 which will use the same providers also inside the config yaml.

@DewaldDeJager
Copy link

We are very close to propose for merging #4742 which will use the same providers also inside the config yaml.

I see this PR was merged. Does that mean we can now use this syntax for default values? ${env:FOO?default=bar}

CC @mx-psi

@mx-psi
Copy link
Member

mx-psi commented Apr 14, 2023

We are very close to propose for merging #4742 which will use the same providers also inside the config yaml.

I see this PR was merged. Does that mean we can now use this syntax for default values? ${env:FOO?default=bar}

CC @mx-psi

No, this only means that you can use ${env:FOO} instead of the legacy ${FOO}. We don't support the default= syntax currently.

@ringerc
Copy link

ringerc commented Nov 14, 2023

See #2534 (comment) for a workaround

It looks like someone did a draft PR at #4458 but it got stalled.

@systems1
Copy link

systems1 commented Dec 14, 2023

$FOO and ${FOO}` both do not work (exporters)

if they are removed or never included

and ${env:FOO} doesnt work in processors
but $FOO and ${FOO} both work in processors
processors:
attributes:
actions:
- action: insert
key: env
#value: dev # works in all
value: ${env:DEPLOY_ENV} # doesnt work
#value: $DEPLOY_ENV # works
#value: "${DEPLOY_ENV}" # works

tested with otel-col-contriub 0.91.0 and 0.72.0

No, this only means that you can use ${env:FOO} instead of the legacy ${FOO}. We don't support the default= syntax currently.

@mx-psi
Copy link
Member

mx-psi commented Dec 14, 2023

@systems1 this seems like a separate issue, can you report this on a new issue and post a full configuration example that reproduces the issue?

@yurishkuro
Copy link
Member

The ticket is 2+ years old, and there is no path forward, #4458 with shell syntax was not accepted.

Is the consensus to go with ${env:VAR?default=xyz} syntax?

@mx-psi
Copy link
Member

mx-psi commented Jul 17, 2024

@yurishkuro The OpenTelemetry Configuration WG now admits the shell syntax for the SDK configuration files. Since we aim to be aligned with them, it makes sense in my opinion to add support in the Collector for this as well.

jvoravong added a commit to signalfx/splunk-otel-collector that referenced this issue Jul 23, 2024
@jvoravong
Copy link
Contributor

I’d like to bump prioritization for this ticket due to the recent changes introduced in issue #9532. The 9532 change, introduced via a feature gate, breaks my previous usage of optional environment variables in collector configurations.

Before #9532:
Optional env var OTEL_MEMORY_LIMIT_MIB could be left empty, and limit_mib would default to an appropriate value.

memory_limiter:
  check_interval: 2s
  limit_mib: ${OTEL_MEMORY_LIMIT_MIB}

After #9532:
If env var OTEL_MEMORY_LIMIT_MIB is left empty the following error occurs due to stricter type checking:

Error: failed to get config: cannot unmarshal the configuration: 1 error(s) decoding:
error decoding 'processors': error reading configuration for "memory_limiter": 1 error(s) decoding:
'limit_mib' expected type 'uint32', got unconvertible type 'string', value: '460'
Hint: Temporarily restore the previous behavior by disabling 
      the `confmap.strictlyTypedInput` feature gate. More details at:
      https://github.com/open-telemetry/opentelemetry-collector/issues/10552

To work around this, I would like to use default values for configuration environment variables as proposed in this ticket. I would use env var OTEL_MEMORY_LIMIT_MIB in my configuration like such:

memory_limiter:
  check_interval: 2s
  limit_mib: ${OTEL_MEMORY_LIMIT_MIB:-512}

This will help avoid breaking changes while retaining previous functionality.

@mx-psi
Copy link
Member

mx-psi commented Jul 24, 2024

@jvoravong I don't get the same error as you do. With this configuration file:

Config file (click to expand)
receivers:
  nop:

exporters:
  nop:

processors:
  memory_limiter:
    check_interval: 2s
    limit_mib: ${OTEL_MEMORY_LIMIT_MIB}

service:
  pipelines:
    metrics:
        receivers: [nop]
        exporters: [nop]
        processors: [memory_limiter]

I get the following error:

2024-07-24T16:24:25.375+0200	warn	[email protected]/provider.go:51	Configuration references unset environment variable	{"name": "OTEL_MEMORY_LIMIT_MIB"}
Error: invalid configuration: processors::memory_limiter: 'limit_mib' or 'limit_percentage' must be greater than zero
2024/07/24 16:24:25 collector server run finished with error: invalid configuration: processors::memory_limiter: 'limit_mib' or 'limit_percentage' must be greater than zero

Which I believe is the same error you would have gotten before the feature gate.

Could you give more details about your setup? Are you using multiple configuration files? Are you using a Helm chart?

@cforce
Copy link
Contributor

cforce commented Jul 28, 2024

Currently Otle colector yaml file do not support default values like

${OTEL_MEMORY_LIMIT_MIB:-512}
only they support ev vars without default
${OTEL_MEMORY_LIMIT_MIB}

Please support default values

@mx-psi mx-psi closed this as completed in 6082ac7 Sep 10, 2024
dmathieu pushed a commit to dmathieu/opentelemetry-collector that referenced this issue Sep 10, 2024
…ry#10907)

#### Description
Support shell-style default value and error message for env var
expansion.
```
// A default value for unset variable can be provided after :- suffix, for example:
// `env:NAME_OF_ENVIRONMENT_VARIABLE:-default_value`
```

#### Link to tracking issue
Fixes open-telemetry#5228

#### Testing
Unit tests

#### Documentation
* [x] Provider Go docs 
* [ ] Probably needs some other docs changes?

---------

Signed-off-by: Yuri Shkuro <[email protected]>
@cforce
Copy link
Contributor

cforce commented Sep 11, 2024

Highly appreciated feature . Tx a lot @yurishkuro @mx-psi for contributing 👏

yurishkuro pushed a commit to jaegertracing/jaeger that referenced this issue Sep 29, 2024
## Which problem is this PR solving?
- Resolves #6027 

## Description of the changes
- Leverages the functionality added in
open-telemetry/opentelemetry-collector#5228 to
remove the logic for rewriting the config and simply setting an
environment variable for the Kafka integration tests.

## How was this change tested?
- The change is to an integration test so the CI passing is a good test
for this change

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Mahad Zaryab <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.