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.

Sign up for GitHub

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

Jump to bottom

Core configuration add env var and casting functionality #2970

Closed
wants to merge 18 commits into from
Closed

Core configuration add env var and casting functionality #2970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
602f7f1
Add `env_var` and `deprecated_env_var` configuration options
GustavoCaso Jul 14, 2023
aaddc4c
update span_attribute_schema to not look for ENV variables
GustavoCaso Jul 14, 2023
8bcd916
update configuration to use `env_var` and `type`
GustavoCaso Jul 14, 2023
61d37b7
update specs to make sure the play nicely with the new configurations…
GustavoCaso Jul 14, 2023
750c721
Exclude appsec setting from steep. Unknown issue
GustavoCaso Jul 14, 2023
cd33917
remobe nil from validation list
GustavoCaso Jul 14, 2023
81c2b96
do not use leading positional argument with default value on SpanAttr…
GustavoCaso Jul 17, 2023
3aa907f
add comments to appsec seetings
GustavoCaso Jul 17, 2023
0fae4d7
raise exception if invalid type used for option
GustavoCaso Jul 17, 2023
00e1bc4
fix time_now_provider and transport_options option definitions
GustavoCaso Jul 17, 2023
d8d6755
allow to set error_handler to nil
GustavoCaso Jul 17, 2023
d09e2fd
add more specs cases for env_var and deprecated_env_var. Refactor set…
GustavoCaso Jul 18, 2023
15246fc
Add comments for 2.0 release
GustavoCaso Jul 19, 2023
765a287
improve int and floar validation. We do not need to have precise floa…
GustavoCaso Jul 19, 2023
d6ab986
move env parsing complxity into env_parser block
GustavoCaso Jul 19, 2023
6cacb85
rename env_var and deprecated_env_var option to env and deprecated_env
GustavoCaso Jul 19, 2023
345454a
rename nil: true option to nilable: true
GustavoCaso Jul 19, 2023
87d739d
revert === to equal? on service_without_fallback
GustavoCaso Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Steepfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ target :ddtrace do

ignore 'lib/datadog/appsec.rb'
ignore 'lib/datadog/appsec/component.rb'
ignore 'lib/datadog/appsec/configuration/settings.rb'
ignore 'lib/datadog/appsec/contrib/auto_instrument.rb'
ignore 'lib/datadog/appsec/contrib/integration.rb'
ignore 'lib/datadog/appsec/contrib/rack/gateway/request.rb'
Expand Down
69 changes: 33 additions & 36 deletions lib/datadog/appsec/configuration/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@ def self.add_settings!(base)
base.class_eval do
settings :appsec do
option :enabled do |o|
o.default { env_to_bool('DD_APPSEC_ENABLED', DEFAULT_APPSEC_ENABLED) }
o.setter do |v|
v ? true : false
end
o.type :bool
o.env 'DD_APPSEC_ENABLED'
o.default DEFAULT_APPSEC_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels weird, seems like it should read o.default true.

Indeed whatever else depends on DEFAULT_APPSEC_ENABLED should instead read the configuration setting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is legacy from when the appsec setting was not using the core configuration code.

We can remove all those constants on a separate PR 😄

end

define_method(:instrument) do |integration_name|
Expand All @@ -53,73 +52,71 @@ def self.add_settings!(base)
end

option :ruleset do |o|
o.default { ENV.fetch('DD_APPSEC_RULES', DEFAULT_APPSEC_RULESET) }
o.env 'DD_APPSEC_RULES'
o.default DEFAULT_APPSEC_RULESET
end

option :ip_denylist do |o|
o.default { [] }
o.type :array
o.default []
Copy link
Contributor

@lloeki lloeki Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a difference between block and value here:

  1. [] would be eval'd when that line is evaluated, so ever produces a single array instance
  2. { [] } would be eval'd when the block is evaluated, so may or may not produce a single array instance

This led me to wonder if we should freeze that default in the 1. case to ensure it does not get mutated.

This then led me to wonder if we should in fact (transparently) freeze all configuration setting values, because once they are set they should not be mutated.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Option#default_value we wrap the default with a call to Core::Utils::SafeDup.frozen_or_dup(...) so we dup it, rather than using this exact object.

(I had wondered the same thing you did and ended up spotting it)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to wrap it on Core::Utils::SafeDup.frozen_or_dup(...) because a test validates that when modifying the default value doesn't modify it by reference.

describe '#writer_options' do
  subject(:writer_options) { settings.tracing.test_mode.writer_options }

  it { is_expected.to eq({}) }

  context 'when modified' do
    it 'does not modify the default by reference' do
      settings.tracing.test_mode.writer_options[:foo] = :bar
      expect(settings.tracing.test_mode.writer_options).to_not be_empty
      expect(settings.tracing.test_mode.options[:writer_options].default_value).to be_empty
    end
  end
end

The options were like this before our changes:

option :writer_options do |o|
  o.default { {} }
end

This is how they are after our changes:

option :writer_options do |o|
  o.type :hash
  o.default({})
end

So I believe this was an oversight on my part in which using a default value as a block also meant that calling default_value would return a hash every time. To fix that particular spec, I had to use Core::Utils::SafeDup.frozen_or_dup

@lloeki suggested that we should be freezing the configuration value it gets set, so any modification to the configuration value should have to use dup first

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think freezing instead of dup'ing seems reasonable as well, although I wonder if somewhere we may be doing something weird that doesn't like it (but perhaps in that situation it'd be worth fixing that code instead of not freezing)

end

option :user_id_denylist do |o|
o.default { [] }
o.type :array
o.default []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should have logic that would have o.type :array imply o.default [].

Then, o.type :array, nilable: true would probably imply o.default nil (debatable but it seems to make the most sense)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a great suggestion and will certainly reduce the amount of boilerplate.

I would leave that exercise for a subsequent PR

end

option :waf_timeout do |o|
o.default { ENV.fetch('DD_APPSEC_WAF_TIMEOUT', DEFAULT_APPSEC_WAF_TIMEOUT) } # us
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
o.env 'DD_APPSEC_WAF_TIMEOUT' # us
o.default DEFAULT_APPSEC_WAF_TIMEOUT
o.setter do |v|
Datadog::Core::Utils::Duration.call(v.to_s, base: :us)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already like this (so I'd say this comment is not for this PR), but wow damn this helper is extremely complex for something that is only parsed as integer-microseconds in our other libraries (and this helper is only used once!).

Consider maybe going back to o.type :int in the future?

Copy link
Contributor

@lloeki lloeki Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in our other libraries

Except in Go, coz it's parsed as a time.Duration using time.ParseDuration, which doesn't accept unitless strings, and the env var got used with a suffix in system tests coz Go was the first one there, and then it got in the wild, so I had to implement this parser otherwise both system tests and Go customers having already set env vars in some global way that would reach Ruby apps would crash.

But they fixed since by appending us when it's missing and IIRC system tests have been adjusted to not use the suffix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider maybe going back to o.type :int in the future?

I'm wondering if an alternative would be to have a o.type :duration shorthand and apply that wherever we have durations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra context!

On the topic of :duration, to be honest, I'd favor keeping it simple -- it's not that common to see environment variables including units, so I'd avoid it unless we really need to.

end
end

option :waf_debug do |o|
o.default { env_to_bool('DD_APPSEC_WAF_DEBUG', DEFAULT_APPSEC_WAF_DEBUG) }
o.setter do |v|
v ? true : false
end
o.env 'DD_APPSEC_WAF_DEBUG'
o.default DEFAULT_APPSEC_WAF_DEBUG
o.type :bool
end

option :trace_rate_limit do |o|
o.default { env_to_int('DD_APPSEC_TRACE_RATE_LIMIT', DEFAULT_APPSEC_TRACE_RATE_LIMIT) } # trace/s
o.type :int
o.env 'DD_APPSEC_TRACE_RATE_LIMIT' # trace/s
o.default DEFAULT_APPSEC_TRACE_RATE_LIMIT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already like this, so no need to change in this PR, but is it just me that finds weird this structure of splitting the definition of the defaults at the top of the file with the definition of the setting:

              DEFAULT_APPSEC_WAF_DEBUG = false

              # ...

              option :trace_rate_limit do |o|
                o.type :int
                o.env_var 'DD_APPSEC_TRACE_RATE_LIMIT' # trace/s
                o.default DEFAULT_APPSEC_TRACE_RATE_LIMIT
              end

I kinda prefer how we do it in other places:

            option :force_enable_gc_profiling do |o|
              o.env_var 'DD_PROFILING_FORCE_ENABLE_GC'
              o.type :bool
              o.default false
            end

...as it avoids the redundancy + avoids any other parts of the code accessing the default constants where they should always go through the settings object.

I claim the settings should be the source of truth for the defaults.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will cleanup in a separate PR

end

option :obfuscator_key_regex do |o|
o.default { ENV.fetch('DD_APPSEC_OBFUSCATION_PARAMETER_KEY_REGEXP', DEFAULT_OBFUSCATOR_KEY_REGEX) }
o.type :string
o.env 'DD_APPSEC_OBFUSCATION_PARAMETER_KEY_REGEXP'
o.default DEFAULT_OBFUSCATOR_KEY_REGEX
end

option :obfuscator_value_regex do |o|
o.default do
ENV.fetch(
'DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP',
DEFAULT_OBFUSCATOR_VALUE_REGEX
)
end
o.type :string
o.env 'DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP'
o.default DEFAULT_OBFUSCATOR_VALUE_REGEX
end

settings :track_user_events do
option :enabled do |o|
o.default do
ENV.fetch(
'DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING',
DEFAULT_APPSEC_AUTOMATED_TRACK_USER_EVENTS_ENABLED
)
end
o.setter do |v|
if v
v.to_s != 'disabled'
o.env 'DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING'
o.default DEFAULT_APPSEC_AUTOMATED_TRACK_USER_EVENTS_ENABLED
o.setter do |value|
if value.is_a?(String)
value != 'disabled'
else
false
!!value # rubocop:disable Style/DoubleNegation
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
end
end
end

option :mode do |o|
o.default do
ENV.fetch('DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING', DEFAULT_APPSEC_AUTOMATED_TRACK_USER_EVENTS_MODE)
end
o.env 'DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING'
o.default DEFAULT_APPSEC_AUTOMATED_TRACK_USER_EVENTS_MODE
o.setter do |v|
string_value = v.to_s
if APPSEC_VALID_TRACK_USER_EVENTS_MODE.include?(string_value)
string_value
if APPSEC_VALID_TRACK_USER_EVENTS_MODE.include?(v.to_s)
v.to_s
else
Datadog.logger.warn(
'The appsec.track_user_events.mode value provided is not supported.' \
Expand Down
11 changes: 6 additions & 5 deletions lib/datadog/ci/configuration/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ def self.add_settings!(base)
base.class_eval do
settings :ci do
option :enabled do |o|
o.default { env_to_bool(CI::Ext::Settings::ENV_MODE_ENABLED, false) }
o.type :bool
o.env CI::Ext::Settings::ENV_MODE_ENABLED
o.default false
end

# DEV: Alias to Datadog::Tracing::Contrib::Extensions::Configuration::Settings#instrument.
Expand All @@ -34,12 +36,11 @@ def self.add_settings!(base)
# TODO: Deprecate in the next major version, as `instrument` better describes this method's purpose
alias_method :use, :instrument

option :trace_flush do |o|
o.default { nil }
end
option :trace_flush

option :writer_options do |o|
o.default { {} }
o.type :hash
o.default({})
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
Expand Down
9 changes: 7 additions & 2 deletions lib/datadog/ci/contrib/cucumber/configuration/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,20 @@ module Configuration
# TODO: mark as `@public_api` when GA
class Settings < Datadog::Tracing::Contrib::Configuration::Settings
option :enabled do |o|
o.default { env_to_bool(Ext::ENV_ENABLED, true) }
o.type :bool
o.env Ext::ENV_ENABLED
o.default true
end

option :service_name do |o|
o.type :string
o.default { Datadog.configuration.service_without_fallback || Ext::SERVICE_NAME }
end

option :operation_name do |o|
o.default { ENV.fetch(Ext::ENV_OPERATION_NAME, Ext::OPERATION_NAME) }
o.type :string
o.env Ext::ENV_OPERATION_NAME
o.default Ext::OPERATION_NAME
end
end
end
Expand Down
9 changes: 7 additions & 2 deletions lib/datadog/ci/contrib/minitest/configuration/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,20 @@ module Configuration
# TODO: mark as `@public_api` when GA
class Settings < Datadog::Tracing::Contrib::Configuration::Settings
option :enabled do |o|
o.default { env_to_bool(Ext::ENV_ENABLED, true) }
o.type :bool
o.env Ext::ENV_ENABLED
o.default true
end

option :service_name do |o|
o.type :string
o.default { Datadog.configuration.service_without_fallback || Ext::SERVICE_NAME }
end

option :operation_name do |o|
o.default { ENV.key?(Ext::ENV_OPERATION_NAME) ? ENV[Ext::ENV_OPERATION_NAME] : Ext::OPERATION_NAME }
o.type :string
o.env Ext::ENV_OPERATION_NAME
o.default Ext::OPERATION_NAME
end
end
end
Expand Down
9 changes: 7 additions & 2 deletions lib/datadog/ci/contrib/rspec/configuration/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,20 @@ module Configuration
# TODO: mark as `@public_api` when GA
class Settings < Datadog::Tracing::Contrib::Configuration::Settings
option :enabled do |o|
o.default { env_to_bool(Ext::ENV_ENABLED, true) }
o.type :bool
o.env Ext::ENV_ENABLED
o.default true
end

option :service_name do |o|
o.type :string
o.default { Datadog.configuration.service_without_fallback || Ext::SERVICE_NAME }
end

option :operation_name do |o|
o.default { ENV.fetch(Ext::ENV_OPERATION_NAME, Ext::OPERATION_NAME) }
o.type :string
o.env Ext::ENV_OPERATION_NAME
o.default Ext::OPERATION_NAME
end
end
end
Expand Down
2 changes: 0 additions & 2 deletions lib/datadog/core/configuration/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ def settings(name, &block)
value.reset! if value.respond_to?(:reset!)
value
end

o.type settings_class
end
end

Expand Down
118 changes: 115 additions & 3 deletions lib/datadog/core/configuration/option.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# frozen_string_literal: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta note on this PR: This is the kind of PR that has the potential to have undetected conflicts with other ongoing PRs -- e.g. any PRs that touch settings that branched out before this PR have the potential to cause issues.

Here's my suggestion -- before merging this PR to master:

  1. Diff master and the base of the core-configuration-add-env-var-and-casting-functionality branch, and see if there's any changes that haven't been picked up
  2. (if needed) Merge master into this branch, include any updates
  3. Do a quick round of open PRs and see if any touch the settings; If they do, ask them to hold off until this gets merged, and after that, ask them to rebase on top of master.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta note on this PR

Meta-question: this has nothing to do with the # frozen_string_literal: true comment, and only uses that first line as anchor for a comment, correct?

Meta-suggestion: maybe it should go as a review comment in "Finish your review"? but then it's not a "threaded" discussion I guess (subsequent discussion would happen in the global PR discussion tab).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta-suggestion: maybe it should go as a review comment in "Finish your review"? but then it's not a "threaded" discussion I guess (subsequent discussion would happen in the global PR discussion tab).

Yes, it's for the whole review, and I only picked elsewhere because I find people sometimes miss the comment posted with the review itself, and I thought this comment was important enough not to be missed.

Plus, as you pointed out, it's really annoying to not have threading for those comments.

So I'm just working around GitHub UX limitations >_>


require_relative '../utils/safe_dup'

module Datadog
module Core
module Configuration
Expand Down Expand Up @@ -49,7 +51,7 @@ def set(value, precedence: Precedence::PROGRAMMATIC)
end

old_value = @value
(@value = context_exec(value, old_value, &definition.setter)).tap do |v|
(@value = context_exec(validate_type(value), old_value, &definition.setter)).tap do |v|
@is_set = true
@precedence_set = precedence
context_exec(v, old_value, &definition.on_set) if definition.on_set
Expand All @@ -62,7 +64,7 @@ def get
elsif definition.delegate_to
context_eval(&definition.delegate_to)
else
set(default_value, precedence: Precedence::DEFAULT)
set_value_from_env_or_default
end
end

Expand All @@ -84,7 +86,7 @@ def default_value
if definition.default.instance_of?(Proc)
context_eval(&definition.default)
else
definition.experimental_default_proc || definition.default
definition.experimental_default_proc || Core::Utils::SafeDup.frozen_or_dup(definition.default)
end
end

Expand All @@ -94,6 +96,93 @@ def default_precedence?

private

def coerce_env_variable(value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the intention here is to replace Datadog::Core::Environment::VariableHelpers. Should we go ahead and mark all those methods as deprecated for removal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with that. There is still one setting that still uses Datadog::Core::Environment::VariableHelpers

settings :client_ip do
  # Whether client IP collection is enabled. When enabled client IPs from HTTP requests will
  #   be reported in traces.
  #
  # Usage of the DD_TRACE_CLIENT_IP_HEADER_DISABLED environment variable is deprecated.
  #
  # @see https://docs.datadoghq.com/tracing/configure_data_security#configuring-a-client-ip-header
  #
  # @default `DD_TRACE_CLIENT_IP_ENABLED` environment variable, otherwise `false`.
  # @return [Boolean]
  option :enabled do |o|
    o.type :bool
    o.default do
      disabled = env_to_bool(Tracing::Configuration::Ext::ClientIp::ENV_DISABLED)

      enabled = if disabled.nil?
                  false
                else
                  Datadog::Core.log_deprecation do
                    "#{Tracing::Configuration::Ext::ClientIp::ENV_DISABLED} environment variable is deprecated, use #{Tracing::Configuration::Ext::ClientIp::ENV_ENABLED} instead."
                  end

                  !disabled
                end

      # ENABLED env var takes precedence over deprecated DISABLED
      env_to_bool(Tracing::Configuration::Ext::ClientIp::ENV_ENABLED, enabled)
    end
  end
end

I guess it is ok to mark the methods from Datadog::Core::Environment::VariableHelpers as deprecated.

I would like to that on separate PR to avoid touching more parts of the codebase on this single PR

return context_exec(value, &@definition.env_parser) if @definition.env_parser

case @definition.type
when :int
# DEV-2.0: Change to a more strict coercion method. Integer(value).
value.to_i
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
when :float
# DEV-2.0: Change to a more strict coercion method. Float(value).
value.to_f
when :array
values = if value.include?(',')
value.split(',')
else
value.split(' ') # rubocop:disable Style/RedundantArgument
end
Comment on lines +110 to +114
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this become value.split(',') only? Since the default is to not support the spaces-variant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the default is not to support the spaces-variant?

Where is that defined?

The code for env_to_list did not define any default value for def env_to_list(var, default = [], comma_separated_only:, deprecation_warning: true)

Doing a search on comma_separated_only shows three instances of true, and four instances of false.

I'm happy to remove the if condition, I just to make sure to understand if we have to

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the default for the DSL should be to not support it, because in my view the "spaces separator" is the weird use-case, so it should require extra work on the part of the settings that want it.

(As I mention below, accepting spaces on top of commas has the potential of creating more inconsistency across datadog libraries)


values.map! do |v|
v.gsub!(/\A[\s,]*|[\s,]*\Z/, '')

v.empty? ? nil : v
end

values.compact!
values
Comment on lines +109 to +123
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recognize this slice as an inlined version of the env_to_list helper, but crucially, without the comma_separated_only.

This means that in some settings, we previously only supported "foo,bar,baz" but now we also support "foo bar baz".

For legacy stuff, there's not a lot we can do, but it's kinda weird to allow the bizarre alternative format for all array-like options.

So I think we should somehow restore the comma_separated_only setting (it can even be a custom type, such as :array_comma_separated_only -- or maybe the reverse :array_can_have_spaces and default is csv-only). It's more complexity on our side, but the trade-off is that we don't expose new ways of configuring stuff to customers. Also notice this has non-local impact -- if we suddenly support spaces in some settings, then every other library is inconsistent if they don't support spaces for those settings as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point @ivoanjo

Will see what I can do

Copy link
Member Author

@GustavoCaso GustavoCaso Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This use cases where tackled on d6ab986 Thanks to the use of env_parser block

Copy link
Contributor

@lloeki lloeki Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comma_separated_only, array_can_have_spaces could become sep: ',' or something (I'm thinking something like bash IFS here, and I think some Ruby stdlib things have sep as an argument name)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should make the separator more configurable -- this way we push everyone to use commas, and they need to go out of their way if they really need to support others (but can, if needed).

Thus, the complexity is moved from the core (supporting arbitrary separators) to the setting (if you wanna do weird stuff, you pay the cost of doing weird stuff, locally).

when :bool
string_value = value
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
string_value = string_value.downcase
string_value == 'true' || string_value == '1' # rubocop:disable Style/MultipleComparison
when :string, NilClass
value
else
raise ArgumentError, "The option #{@definition.name} is using an unknown type option `#{@definition.type}`"
end
Comment on lines +130 to +132
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I wonder if this should be omitted, since validate_type will do the same work anyway.

end

def validate_type(value)
raise_error = false

valid_type = validate(@definition.type, value)

unless valid_type
raise_error = if @definition.type_options[:nilable]
!value.is_a?(NilClass)
else
true
end
end

if raise_error
error_msg = if @definition.type_options[:nilable]
"The option #{@definition.name} support this type `#{@definition.type}` "\
"and `nil` but the value provided is #{value.class}"
else
"The option #{@definition.name} support this type `#{@definition.type}` "\
"but the value provided is #{value.class}"
end
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved

raise ArgumentError, error_msg
end

value
end

def validate(type, value)
case type
when :string
value.is_a?(String)
when :int, :float
value.is_a?(Numeric)
when :array
value.is_a?(Array)
when :hash
value.is_a?(Hash)
when :bool
value.is_a?(TrueClass) || value.is_a?(FalseClass)
when :proc
value.is_a?(Proc)
when :symbol
value.is_a?(Symbol)
when NilClass
true
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +179 to +180
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Maybe worth adding a comment here stating that this is a fallback for settings that don't define a type.

else
raise ArgumentError, "The option #{@definition.name} is using an unknown type option `#{@definition.type}`"
GustavoCaso marked this conversation as resolved.
Show resolved Hide resolved
end
end

def context_exec(*args, &block)
@context.instance_exec(*args, &block)
end
Expand All @@ -102,6 +191,29 @@ def context_eval(&block)
@context.instance_eval(&block)
end

def set_value_from_env_or_default
value = nil
precedence = nil

if definition.env && ENV[definition.env]
value = coerce_env_variable(ENV[definition.env])
precedence = Precedence::PROGRAMMATIC
end

if value.nil? && definition.deprecated_env && ENV[definition.deprecated_env]
value = coerce_env_variable(ENV[definition.deprecated_env])
precedence = Precedence::PROGRAMMATIC
Comment on lines +200 to +205
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Precedence::DEFAULT? According to the comments at the top of this file, DEFAULT is

          # Configuration that comes either from environment variables,
          # or fallback values.
          DEFAULT = [0, :default].freeze

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment should be changed.

If a customer has modified the configuration value via ENV variable, it has been configured programmatically since the customer is not using the default value.

For example:

option :enabled do |o|
  o.type :bool
  o.env 'DD_APPSEC_ENABLED'
  o.default false
end

If the customer changes the value using Datadog.configuration.appsec.enabled = true or via DD_APPSEC_ENABLED, we would want the helper function using_default? to return false Datadog.configuration.appsec.using_default?(:enabled) #=> false, since the intent of DEFAULT = [0, :default].freeze is to signal the customer hasn't set the value in anyway

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Seems reasonable -- either updating the comment or perhaps introducing a new environment variable level seems OK.


Datadog::Core.log_deprecation do
"#{definition.deprecated_env} environment variable is deprecated, use #{definition.env} instead."
end
end

option_value = value.nil? ? default_value : value

set(option_value, precedence: precedence || Precedence::DEFAULT)
end

# Used for testing
attr_reader :precedence_set
private :precedence_set
Expand Down
Loading