[EXPERIMENTAL] Add `env_var` and `deprecated_env_var` configuration options #2936

GustavoCaso · 2023-06-29T12:59:14Z

What does this PR do?

I'm exploring trying to try solve some problems from a private discussion about how to track the configuration option using the ENV variable or the fallback.

The PR explores adding env_var and deprecatated_env_var as configuration options. Those options would be used to fetch the value from ENV. If the values are present, we treat them as being set programmatically by the customer. That way, we can later use the using_default? method to know if a configuration is using the default value or the one set by the customer.

I had to refactor the functions env_to_* into val_to_*, since they no longer deal with ENV variables. The methods still live under Core::Environment::VariableHelpers; that namespace is wrong.

The intent of the PR is for people to see the approach and comment on anything they would change or miss.

If people like this addition, I will address all the missing and failing specs and any feedback from comments. Finish adding o.default to some settings

Motivation

Additional Notes

How to test the change?

delner · 2023-06-29T14:49:28Z

env_var is great. Might I suggest adding type conversion as well? e.g.

env_var 'DD_TRACE_ENABLED', type: :bool
env_var 'DD_TRACE_HEADER_STYLES', type: :array

# Alternatively...

env_var 'DD_TRACE_HEADER_STYLES', type: Array

ivoanjo

Left a few notes :)

Overall I don't have very strong thoughts in terms of this option vs what we had before (e.g. I'm not convinced this one is better, but also not convinced it's worse, just different).

ivoanjo · 2023-07-03T08:49:36Z

lib/datadog/appsec/configuration/settings.rb

+                o.env_var 'DD_APPSEC_ENABLED'
+                o.default DEFAULT_APPSEC_ENABLED
+                o.setter do |value|
+                  val_to_bool(value)
                end


I'll preface my comment by saying that I'm aware we're trading off between several upsides and downsides, so we may not be able to do everything for every use case.

That said, it seems to me a bit weird that we're generalizing the coercion we used to have only for environment variables (which are forced to be strings) and applying it to when users set things as code.

For instance in master (e.g. without this PR), we never forced any types. So you could do this (which doesn't make a lot of sense, hang on):

[2] pry(main)> Datadog.configuration.profiling.enabled => false [3] pry(main)> Datadog.configure { |c| c.profiling.enabled = "????potato" } # ... [4] pry(main)> Datadog.configuration.profiling.enabled => "????potato"

With this PR, the "????potato" gets coerced:

[2] pry(main)> Datadog.configuration.profiling.enabled => false [3] pry(main)> Datadog.configure { |c| c.profiling.enabled = "????potato" } # ... [4] pry(main)> Datadog.configuration.profiling.enabled => false

My claim is that neither of these makes a lot of sense, but that the new behavior seems to be "legitimizing" and erasing the bad input by coercing it into something that is what we expected.

Aka -- whereas in environment variables I think we're making the right choice for the conversion, it seems weird to me that we'd try to force anything that I pass in there into a string and then a boolean, rather than complaining back at the user "hey, you really sure you want to give me this random object as an input for this setting?"

TL;DR: I'm not sure that we should use the same coercion code (e.g. setter) for values from environment variables vs something set as code.

Now, we have enough info in the DSL to realize when a value is coming in from an environment variable vs set by users, so we could make use of that to have separate code paths for them (maybe? just a suggestion).

ivoanjo · 2023-07-03T08:52:39Z

lib/datadog/core/configuration/settings.rb

+              o.default 30.0
+              o.setter do |value|
+                val_to_float(value, 30.0)
+              end


Minor: It seems a bit weird to repeat the default in the setter as well. Perhaps it should get it as an argument? (Or have a way to ask it?)

ivoanjo · 2023-07-03T08:53:12Z

lib/datadog/core/configuration/settings.rb

+              o.env_var Profiling::Ext::ENV_MAX_FRAMES
+              o.setter do |value|
+                val_to_int(value, 400)
+              end


Minor: Following the other changes, should the default here be set as well?

ivoanjo · 2023-07-03T08:56:05Z

lib/datadog/core/configuration/settings.rb

+          o.env_var Core::Environment::Ext::ENV_SERVICE
+          o.default { Core::Environment::Ext::FALLBACK_SERVICE_NAME }


Should this default be updated to not use a block? (There's other defaults in this PR inside a block, maybe worth revisiting as well?)

The intent of this PR is not to modify existing default blocks. Once we merge the PR that removes lazy as a option, we can have a PR that goes over the defaults values that should not be a block.

lib/datadog/core/environment/variable_helpers.rb

ivoanjo · 2023-07-03T09:04:45Z

lib/datadog/tracing/configuration/settings.rb

@@ -51,14 +53,13 @@ def self.extended(base)
                #   otherwise `['Datadog','b3multi','b3']`.
                # @return [Array<String>]
                option :propagation_extract_style do |o|
-                  o.default do
+                  o.deprecated_env_var Tracing::Configuration::Ext::Distributed::ENV_PROPAGATION_STYLE_EXTRACT_OLD


I may have counted them wrong, but it seems to me we're adding deprecated_env_var to support only 2 configuration settings -- this one and ENV_PROPAGATION_STYLE_INJECT_OLD.

It seems a lot of work for only these two use-cases... Could we perhaps use custom logic on the setter or somewhere else to avoid having the configuration system need to support this?

lib/datadog/core/utils/variable_helpers.rb

I noticed that calling `SafeDup.frozen_or_dup({a: :b})` on ruby 3.2 would break; because it tries to call `+(-){a: :b}` which doesn't work. I had to ensure the `SafeDup` module worked for all objects. I decided to extract that work into a separate PR to ease the work of reviewing. The method `frozen_or_dup` and `frozen_dup` for ruby 2.3 and forward only expect to receive String values. This could lead to future errors. By checking if the value is a String we can warranty the same behaviour for all kinds of objects Also, I added some extract refimients to be able to call `#dup` on `true, false, 1, 1.0` values.

GustavoCaso · 2023-07-14T11:06:17Z

Close in favour of #2970

github-actions bot added appsec Application Security monitoring product ci-app CI product for test suite instrumentation core Involves Datadog core libraries integrations Involves tracing integrations profiling Involves Datadog profiling tracing labels Jun 29, 2023

GustavoCaso changed the title ~~[EXPERIMENTAL] Add env_var and deprecated_env_var configuration options~~ [EXPERIMENTAL] Add env_var and deprecated_env_var configuration options Jun 29, 2023

GustavoCaso force-pushed the add-env-var-and-deprecated-env-var-option-for-configuration branch from c9f4ec8 to 56b8128 Compare June 29, 2023 14:48

GustavoCaso force-pushed the add-env-var-and-deprecated-env-var-option-for-configuration branch from 56b8128 to 7166995 Compare July 3, 2023 08:14

ivoanjo reviewed Jul 3, 2023

View reviewed changes

GustavoCaso force-pushed the add-env-var-and-deprecated-env-var-option-for-configuration branch from 7166995 to 611ccb6 Compare July 3, 2023 16:21

github-actions bot removed the profiling Involves Datadog profiling label Jul 3, 2023

github-advanced-security bot found potential problems Jul 3, 2023

View reviewed changes