[config] Default config env variable expansion #2231

KSerrania · 2020-12-01T16:06:35Z

Description:

Adds the ability for components to use environment variables in string fields in the default configuration returned by CreateDefaultConfig. These fields get expanded when the component is loaded, in the same way the fields of configuration yaml files are ($FOO is replaced by the value of the FOO environment variable, $$FOO is replaced by $FOO, $$$FOO is replaced by $followed by the contents of FOO).

For instance, if CreateDefaultConfig of a component returns:

&Config{
  ...
  TagsConfig: &TagsConfig{
    Env: "$DD_ENV",
  }
  ...
}

and the DD_ENV environment variable is set to prod, then the resulting struct will contain:

&Config{
  ...
  TagsConfig: &TagsConfig{
    Env: "prod",
  }
  ...
}

Note: The default config is expanded before it's merged with the user-provided config, so as to not mess with the latter.

Link to tracking Issue: n/a

Testing:

Added unit tests to check that the variable expansion works, and that it doesn't crash in edge cases (unexported private fields that can't be modified, uninitialised config object).

Tested behavior with the Datadog exporter.

Documentation: n/a
TODO: add documentation about the feature (where should I add this?).

codecov · 2020-12-01T16:15:02Z

Codecov Report

Merging #2231 (f49e50c) into master (14047fd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2231   +/-   ##
=======================================
  Coverage   91.97%   91.98%           
=======================================
  Files         271      271           
  Lines       15656    15672   +16     
=======================================
+ Hits        14400    14416   +16     
  Misses        854      854           
  Partials      402      402

Impacted Files	Coverage Δ
config/config.go	`98.95% <100.00%> (+0.08%)`	⬆️
internal/data/traceid.go	`84.61% <0.00%> (-7.70%)`	⬇️
internal/collector/telemetry/telemetry.go	`94.44% <0.00%> (+4.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14047fd...f49e50c. Read the comment docs.

mx-psi · 2020-12-01T16:50:07Z

config/config.go

@@ -305,6 +306,7 @@ func LoadReceiver(componentConfig *viper.Viper, typeStr configmodels.Type, fullN
 	// Create the default config for this receiver.
 	receiverCfg := factory.CreateDefaultConfig()
 	receiverCfg.SetName(fullName)
+	expandEnvLoadedConfig(receiverCfg)


Shouldn't this be done in loadReceivers instead of here? It looks odd to me that this is different that for the rest of component kinds

This is because receivers are handled differently since #658, it's by design. Consider this solved.

tigrannajaryan · 2020-12-01T17:29:19Z

@KSerrania is there an issue open for this? Was it discussed with maintainers? If not then this may need to wait. The bar for new core features is high now, we are focused on making a stable release right now.

KSerrania · 2020-12-02T09:13:55Z

Hi @tigrannajaryan,

This is a feature we'd like to use in our exporter, but it is not urgent: it can wait until the work needed to prepare the stable release is done.

This hasn't been discussed with maintainers yet, I meant to open this PR as a means to open the discussion while providing a concrete example on how to do this. I can also open an issue if we want to discuss the proposal and its implementation separately.

tigrannajaryan · 2020-12-02T20:28:29Z

@KSerrania I like the change.

To make sure we are not breaking existing code can you please check that no existing component uses the $ character anywhere in their default config? Please check this repo and also https://github.com/open-telemetry/opentelemetry-collector-contrib/ repo.

tigrannajaryan

LGTM.
Can be merged once we have a confirmation that we are not breaking existing default configs.

tigrannajaryan · 2020-12-02T20:29:33Z

config/config_test.go

@@ -571,3 +571,227 @@ func loadConfigFile(t *testing.T, fileName string, factories component.Factories
 	}
 	return cfg, ValidateConfig(cfg, zap.NewNop())
 }
+
+type NestedConfig struct {


Nit: this does not need be exported.

tigrannajaryan · 2020-12-02T20:29:41Z

config/config_test.go

+	NestedIntValue    int
+}
+
+type TestConfig struct {


KSerrania · 2020-12-03T12:19:04Z

I searched, in all go files, for instances of $ followed by an alphanumeric character, an underscore or {, ( or [, which should cover most cases of env variables., with ag '\$[\w_\{\(\[]' -G ".*\.go"

In opentelemetry-collector, no such occurrences appear in default configs.

In opentelemetry-collector-contrib, there's one such instance in a config object (https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/471b7c69266e29de1bfbd99d6ae3c09195547b1d/receiver/dockerstatsreceiver/docker_test.go#L43-L45), but it's in a test, not a default config.

To double-check, I then searched for all NewFactory functions (with ag 'func.*NewFactory' -G ".*\.go"). They all either define their config directly in this function (I didn't find any $ there), or use a createDefaultConfig function.

Finally, I checked all createDefaultConfig functions with (ag 'func.*[cC]reateDefaultConfig' -G ".*\.go") and I also didn't find any case of $ there.

Therefore, I am reasonably sure that this shouldn't break existing default configs.

Notes:

For opentelemetry-collector, I did the search on commit 2962d95 (latest master as of this morning).

For opentelemetry-collector-contrib, I did the search on commit 471b7c6 (latest master as of this morning).

I think this feature should be added to developer docs somewhere, to make sure contributors do not get surprised by this in the future. What would be a good place to document this?

tigrannajaryan · 2020-12-03T14:27:24Z

@KSerrania thanks for the thorough search.

I think this feature should be added to developer docs somewhere, to make sure contributors do not get surprised by this in the future. What would be a good place to document this?

There is currently no good place with high visibility that I can recommend. Ideally we would have a "How to create a new component" document and could add a note there, but no such doc exists currently.

However, I think it is OK for now not document it. If they accidentally use the $ character in the default config it is going to get expanded unconditionally on startup and will fail fast with high visibility.

Uses the feature added in open-telemetry/opentelemetry-collector#2231 to allow users to set config options with environment vairables. Allowed environment variables: - `DD_API_KEY` -> `Config.API.Key` - `DD_SITE` -> `Config.API.Site` - `DD_HOST` -> `Config.TagsConfig.Hostname` - `DD_ENV` -> `Config.TagsConfig.Env` - `DD_SERVICE` -> `Config.TagsConfig.Service` - `DD_VERSION` -> `Config.TagsConfig.Version` - `DD_TAGS` -> `Config.TagsConfig.EnvVarTags` (replaces `Config.TagsConfig.Tags` if not set) - `DD_URL` -> `Config.Metrics.TCPAddr.Endpoint` - `DD_APM_URL` -> `Config.Traces.TCPAddr.Endpoint` For `DD_TAGS`, since `Config.TagsConfig.Tags` is a map, and the env variable replacement only operates on string fields, a new placeholder string option `Config.TagsConfig.EnvVarTags` was created to temporarily get the environment variable content. **Testing:** Added unit tests, and tested in our exporter E2E test environment **Documentation:** Updated the example config to explain which environment variables can be used.

) * pin working docker connections dep * validate observer discovery configs on loading * add missing testcontainers helpers * Add docker observer discovery mode test * make: don't repeat integration tests

KSerrania added 12 commits December 1, 2020 15:01

Add utility to expand env variables in config objects

98b7265

Expand config variables in exporter config objects

9267a77

Expand config variables in processor config objects

36cc662

Early stop in expandEnvLoadedConfigPointer

a019b0d

Whitespace fix

e24dd8b

Expand config variables in extension config objects

46f45ef

Expand config variables in receiver config objects

add96b0

Lint

20a312a

Comment fix

607a811

Support string pointers

fb65ee3

Properly unset env variables in tests

26f49f3

Clearer comments

ba793e6

KSerrania changed the title ~~[config] Loaded config expansion~~ [config] Default config env variable expansion Dec 1, 2020

KSerrania added 2 commits December 1, 2020 17:20

Improve test coverage

e68b1c0

Lint

555bf8d

KSerrania marked this pull request as ready for review December 1, 2020 16:39

KSerrania requested a review from a team December 1, 2020 16:39

mx-psi reviewed Dec 1, 2020

View reviewed changes

tigrannajaryan reviewed Dec 2, 2020

View reviewed changes

Do not export test structs

f49e50c

tigrannajaryan approved these changes Dec 3, 2020

View reviewed changes

tigrannajaryan merged commit d8e65bd into open-telemetry:master Dec 3, 2020

KSerrania mentioned this pull request Dec 23, 2020

[datadogexporter] Add env variables support for config options open-telemetry/opentelemetry-collector-contrib#1897

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[config] Default config env variable expansion #2231

[config] Default config env variable expansion #2231

KSerrania commented Dec 1, 2020 •

edited

Loading

codecov bot commented Dec 1, 2020 •

edited

Loading

mx-psi Dec 1, 2020

mx-psi Dec 1, 2020

tigrannajaryan commented Dec 1, 2020

KSerrania commented Dec 2, 2020

tigrannajaryan commented Dec 2, 2020 •

edited

Loading

tigrannajaryan left a comment

tigrannajaryan Dec 2, 2020

tigrannajaryan Dec 2, 2020

KSerrania commented Dec 3, 2020 •

edited

Loading

tigrannajaryan commented Dec 3, 2020

[config] Default config env variable expansion #2231

[config] Default config env variable expansion #2231

Conversation

KSerrania commented Dec 1, 2020 • edited Loading

codecov bot commented Dec 1, 2020 • edited Loading

Codecov Report

mx-psi Dec 1, 2020

Choose a reason for hiding this comment

mx-psi Dec 1, 2020

Choose a reason for hiding this comment

tigrannajaryan commented Dec 1, 2020

KSerrania commented Dec 2, 2020

tigrannajaryan commented Dec 2, 2020 • edited Loading

tigrannajaryan left a comment

Choose a reason for hiding this comment

tigrannajaryan Dec 2, 2020

Choose a reason for hiding this comment

tigrannajaryan Dec 2, 2020

Choose a reason for hiding this comment

KSerrania commented Dec 3, 2020 • edited Loading

tigrannajaryan commented Dec 3, 2020

KSerrania commented Dec 1, 2020 •

edited

Loading

codecov bot commented Dec 1, 2020 •

edited

Loading

tigrannajaryan commented Dec 2, 2020 •

edited

Loading

KSerrania commented Dec 3, 2020 •

edited

Loading