Add support for Option precedence #2915

marcotc · 2023-06-19T21:37:07Z

This is part of #2848

With Remote Configuration, it will be possible to override local options remotely.

As a side effect, a decision has also been made to do not allow local options to override remote configuration, if a remote configuration is applied.

Effectively, any specific configuration option that is configured by Remote Configuration can only be reconfigured by Remote Configuration. This behavior only applies to the specific option that was changed by Remote Configuration, not the whole configuration set as a whole.

The effective configuration precedence is now:

Remote Configuration.
Custom options inside a Datadog.configure block.
Environment variables.

#2920 has been merged into this branch. It uses precedence to check if the current option is using the default value or not.

ivoanjo

Looks reasonable! I left a bunch of suggestions, but I don't think any of them are blocking -- let me know if you'd prefer to merge this as-is and I'll be happy to approve it :)

docs/GettingStarted.md

ivoanjo · 2023-06-20T08:50:44Z

docs/GettingStarted.md

-  c.tracing.test_mode.enabled = (ENV['RACK_ENV'] == 'test')
-end
-```
+**If a higher precedence value is set for an option, setting that option with a lower precedence value will not change its effective value.**


One thing I added to the AgentSettingsResolver are a few nice warnings when there are clashes between settings -- https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/core/configuration/agent_settings_resolver.rb#L268 .

Maybe a nice feature to look into for the other options as well?

ivo.anjo@rubyshade:~/datadog/dd-trace-rb$ DD_AGENT_HOST=potato bundle exec ruby -e "require 'ddtrace'; Datadog.configure { |c| c.agent.host = 'foo' }" W, [2023-06-20T09:47:56.429261 #64369] WARN -- ddtrace: [ddtrace] Configuration mismatch: values differ between 'c.agent.host' ("foo") and DD_AGENT_HOST environment variable ("potato"). Using "foo". W, [2023-06-20T09:47:56.443702 #64369] WARN -- ddtrace: [ddtrace] Configuration mismatch: values differ between 'c.agent.host' ("foo") and DD_AGENT_HOST environment variable ("potato"). Using "foo". W, [2023-06-20T09:47:56.447870 #64369] WARN -- ddtrace: [ddtrace] Configuration mismatch: values differ between 'c.agent.host' ("foo") and DD_AGENT_HOST environment variable ("potato"). Using "foo".

(The extra dupes come from things that don't use the core AgentSettingsResolver instance and recreate one instead 😭 )

I implemented it as a debug log, 🙇.

docs/GettingStarted.md

ivoanjo · 2023-06-20T09:58:10Z

lib/datadog/core/configuration/option.rb

+          REMOTE_CONFIGURATION = 2
+          PROGRAMMATIC = 1
+          DEFAULT = 0


Minor: I'm not the biggest fan of opaque numbers (since when you inspect stuff it's very opaque), but introducing a whole new struct/class, ... may be a bit overkill. I would suggest perhaps a compromise of using a hash/array, e.g.

REMOTE_CONFIGURATION = {remote_configuration: 2}.freeze # or PROGRAMMATIC = {name: programmatic, level: 1}.freeze # or DEFAULT = [:default, 0].freeze

This doesn't need more boilerplate, and the only code that needs to change is the precedence comparison to dig inside the structure instead of using the raw number.

🎶 Enum, num, num...

ivoanjo · 2023-06-20T10:01:24Z

lib/datadog/core/configuration/option.rb

+        #
+        # @param value [Object] the new value to be associated with this option
+        # @param precedence [Precedence] from what precedence order this new value comes from
+        def set(value, precedence: Precedence::PROGRAMMATIC)


Since this API is not public (...I think...?) perhaps it would make sense to not have a default for precedence:? I think anyway you've covered all callers already :)

I think the default being "the user will override anything, except Remote Configuration" is the proper default here.

I don't like that this is a public API, but hiding it also prevents users writing extensions from being able to set Precedence::DEFAULT values, which is a valid use case.

I don't think this is the perfect API, but it currently models correctly the model we are going for for remote vs local settings.

Ah, I didn't mean to say we should hide it, my suggestion is to omit Precedence::PROGRAMMATIC from being the default.

That way, we always force callers to make a decision on what precedence should be used.

lib/datadog/core/configuration/option.rb

ivoanjo · 2023-06-20T10:05:05Z

lib/datadog/core/configuration/option.rb

+        # @param value [Object] the new value to be associated with this option
+        # @param precedence [Precedence] from what precedence order this new value comes from
+        def set(value, precedence: Precedence::PROGRAMMATIC)
+          return @value if precedence < @precedence_set # Cannot override higher precedence value


On the topic of warnings when there are clashes between settings (comment above), I think we should at least print a warning when a setting gets ignored here.

E.g. it's very counter-intuitive that I would run a Datadog.configure { } block 10 minutes after I started my application and my setting would get outright ignored because in the meanwhile it got set via remote configuration.

ivoanjo · 2023-06-20T10:07:17Z

lib/datadog/core/configuration/option.rb

+        module Precedence
+          REMOTE_CONFIGURATION = 2
+          PROGRAMMATIC = 1


Minor: I wonder if there should exist an internal local override mode that is the highest priority (even if we don't document it)? Even if only for testing, it seems weird that the highest priority is remote configuration and that's the latest word on the subject.

That can make sense!
I don't think there's a use case for that yet, because we reset settings in between each test run.

But I'm not opposed to adding it if a use case arises.

Yeah, good point, no use adding more complexity for a speculative use case.

docs/GettingStarted.md

GustavoCaso · 2023-06-20T12:21:51Z

lib/datadog/core/configuration/option.rb

+        attr_reader :precedence_set
+        private :precedence_set


I believe this could be rewritten to:

private attr_reader :precedence_set

I think is more idiomatic ruby

I tried, but Rubocop won't let me, not even with (AllowModifiersOnSymbols: true, which is the default) https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/AccessModifierDeclarations

…to-configuration Add `using_default?` method to Core::Configuration

Co-authored-by: Ivo Anjo <[email protected]>

codecov-commenter · 2023-06-26T22:05:00Z

Codecov Report

Merging #2915 (c0f8a62) into master (a4b81cf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #2915    +/-   ##
========================================
  Coverage   97.96%   97.96%            
========================================
  Files        1282     1282            
  Lines       70688    70809   +121     
  Branches     3249     3252     +3     
========================================
+ Hits        69249    69368   +119     
- Misses       1439     1441     +2

Impacted Files	Coverage Δ
lib/datadog/core/configuration/option.rb	`98.00% <100.00%> (+0.85%)`	⬆️
lib/datadog/core/configuration/options.rb	`100.00% <100.00%> (ø)`
spec/datadog/core/configuration/base_spec.rb	`100.00% <100.00%> (ø)`
spec/datadog/core/configuration/option_spec.rb	`99.48% <100.00%> (+0.18%)`	⬆️
spec/datadog/core/configuration/options_spec.rb	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ivoanjo

I've left a few more notes for your consideration, but I think it's in good enough shape that I'll press the magic button and let you make the decisions 👍

docs/GettingStarted.md

ivoanjo · 2023-06-27T08:55:09Z

lib/datadog/core/configuration/option.rb

+        #
+        # @param value [Object] the new value to be associated with this option
+        # @param precedence [Precedence] from what precedence order this new value comes from
+        def set(value, precedence: Precedence::PROGRAMMATIC)


Ah, I didn't mean to say we should hide it, my suggestion is to omit Precedence::PROGRAMMATIC from being the default.

That way, we always force callers to make a decision on what precedence should be used.

ivoanjo · 2023-06-27T08:58:02Z

lib/datadog/core/configuration/option.rb

+          if precedence[0] < @precedence_set[0]
+            Datadog.logger.debug do
+              "Option '#{definition.name}' not changed to '#{value}' (precedence: #{precedence[1]}) because the higher " \
+                "precedence value '#{@value}' (precedence: #{@precedence_set[1]}) was already set."
+            end
+
+            return @value


I'll leave the final decision up to you, but I think this should be higher than debug (at least info, possibly warn).

Setting an option is an explicit action being taken by code that the user wrote, so it seems reasonable to tell them when we're ignoring what they asked for.

I was concerned with our own internal usage of Datadog.configure {}, which the user has no control over.
These, in my mind, could trigger this log line.

But inspecting them closer, none of the internal use cases change any of the options that Remote Configuration change today, and they don't seem likely to be supported through Remote Configuration in the future.

I've bumped it up to INFO: I'm not confident WARN is the best option because Remote Configuration overriding your local configuration leaves the application in a stable state, so the WARN might not actually be of concern. I don't want to have to introduce an introspection API for users to avoid triggering the WARN ("let me check if this option was set by remote configuration before setting it locally, otherwise ddtrace will WARN").

Seems reasonable!

lib/datadog/core/configuration/option.rb

ivoanjo · 2023-06-27T09:11:24Z

spec/datadog/core/configuration/base_spec.rb

+        context 'without default option' do
+          context 'when not set' do
+            it 'returns false' do
+              expect(configuration.fake_test.using_default?(:without_default)).to be(false)
+            end
+          end
+
+          context 'when not set but accessed' do
+            it 'returns true' do
+              configuration.fake_test.without_default
+              expect(configuration.fake_test.using_default?(:without_default)).to be(true)
+            end
+          end


I believe this came from @GustavoCaso's PR, but... it's weird to me to adopt these semantics. E.g. when would it be useful to have "no default" change from false to true depending on it being accessed or not?

This seems like a potential foot-gun being armed and primed to go.

That is an excellent point @ivoanjo

Maybe we should always return false when the option hasn't been accessed or configured once. That way we always rely on the precedence value for returning the result.

def using_default?(option) return options[option].default_precedence? if options[option] false end

@ivoanjo @marcotc any thoughts?

It's somewhat interesting to me that you propose that the default be false.

So, part of my confusion on what to suggest here is that up until now I've only seen the implementation of this API, and not really what we're going to do with it.

From some our chats, I thought the intention is for using_default? was to be used first as a way for remote configuration to decide "has the customer provided their own config directly or not?".

And in that situation, it kinda looks to me that we'd want using_default? to be true as a fallback when not accessed. That's because, in practice there's no default but the intention seems to be (?) that we're using this check as a proxy for did the customer set something via code or not?, and the answer to me is that they did not, and thus, it should count as true (= using default).

But again I may be misunderstanding all of the above :)

@ivoanjo, thanks for the feedback.

You are right! We should default to true since the values haven't been yet configured or accessed, meaning the user or remote configuration hasn't been configured yet.

I thought the intention is for using_default? was to be used first as a way for remote configuration to decide "has the customer provided their own config directly or not?".

That use case, while valid, is not why I introduced the code. In AppSec we need to check if the user has configured a certain setting.

dd-trace-rb/lib/datadog/appsec/remote.rb

Lines 107 to 109 in cbbd749

def remote_features_enabled?

Datadog::AppSec.send(:default_setting?, :ruleset)

end

And in that situation, it kinda looks to me that we'd want using_default? to be true as a fallback when not accessed.

Right!

Let me update the code to reflect the conversation above

Done bb39eca

ivoanjo · 2023-06-27T09:17:26Z

spec/datadog/core/configuration/options_spec.rb

+          context 'with precedence' do
+            subject(:set_option) { options_object.set_option(name, value, precedence: precedence) }
+            let(:precedence) { 123 }
+
+            it 'sets the option precedence' do
+              set_option
+              expect(options_object.options[name].send(:precedence_set)).to eq(precedence)
+            end
+          end


I wonder if we should enforce that precedence is one of the defined ones? Just to make sure nobody thinks to provide their own custom level and wreck havoc.

I agree with that. We should probably validate that the precedence is part of the ones defined by us 😄

I changed the test to use a real precedence value, as to not lead people into thinking it can be anything.

…r accessed

GustavoCaso · 2023-06-27T11:49:49Z

@marcotc makes sure to pull before doing any more work. I added a new commit changing some behaviour from using_default?

Co-authored-by: Ivo Anjo <[email protected]>

Add support for Option precedence

6c6b52a

marcotc requested a review from a team June 19, 2023 21:37

marcotc self-assigned this Jun 19, 2023

github-actions bot added the core Involves Datadog core libraries label Jun 19, 2023

marcotc mentioned this pull request Jun 19, 2023

Tracing Dynamic Configuration #2848

Merged

ivoanjo reviewed Jun 20, 2023

View reviewed changes

GustavoCaso reviewed Jun 20, 2023

View reviewed changes

GustavoCaso and others added 4 commits June 21, 2023 10:29

Add using_default? method to Core::Configuration

dbda20b

Merge pull request #2920 from DataDog/core-add-default_option-method-…

0587a15

…to-configuration Add `using_default?` method to Core::Configuration

Apply suggestions from code review

5b990c2

Co-authored-by: Ivo Anjo <[email protected]>

Merge branch 'master' into precedence

c27fa60

marcotc changed the base branch from tracing-remote-configuration to master June 26, 2023 21:49

Less lingo, more language

456518b

marcotc added 2 commits June 26, 2023 15:17

Emit debug log on ignored setting override

ac20f03

Use object instead of number for precedence enum

78b930a

marcotc requested review from ivoanjo and GustavoCaso June 26, 2023 22:51

ivoanjo approved these changes Jun 27, 2023

View reviewed changes

using_default? defaults to true when option has not been configured o…

bb39eca

…r accessed

marcotc and others added 5 commits June 27, 2023 10:57

Document precedence levels

8eb127f

Update remote configuration doc link

04ca0fc

Update docs/GettingStarted.md

3d2d5fa

Co-authored-by: Ivo Anjo <[email protected]>

Increase log level to info

138ccae

Use real precedence in test

c0f8a62

marcotc merged commit 7b9014b into master Jun 27, 2023

marcotc deleted the precedence branch June 27, 2023 22:03

github-actions bot added this to the 1.13.0 milestone Jun 27, 2023

marcotc mentioned this pull request Jul 14, 2023

Add support for unsetting options #2972

Merged

TonyCTHsu mentioned this pull request Jul 30, 2023

Bump to version 1.13.0 #3009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Option precedence #2915

Add support for Option precedence #2915

marcotc commented Jun 19, 2023 •

edited

Loading

ivoanjo left a comment

ivoanjo Jun 20, 2023

marcotc Jun 26, 2023

ivoanjo Jun 20, 2023

marcotc Jun 26, 2023

ivoanjo Jun 20, 2023

marcotc Jun 26, 2023

ivoanjo Jun 27, 2023

ivoanjo Jun 20, 2023

marcotc Jun 26, 2023

ivoanjo Jun 20, 2023

marcotc Jun 26, 2023

ivoanjo Jun 27, 2023

GustavoCaso Jun 20, 2023 •

edited

Loading

marcotc Jun 26, 2023

codecov-commenter commented Jun 26, 2023 •

edited

Loading

ivoanjo left a comment

ivoanjo Jun 27, 2023

ivoanjo Jun 27, 2023

marcotc Jun 27, 2023

ivoanjo Jun 28, 2023

ivoanjo Jun 27, 2023

GustavoCaso Jun 27, 2023

ivoanjo Jun 27, 2023 •

edited

Loading

GustavoCaso Jun 27, 2023

GustavoCaso Jun 27, 2023

ivoanjo Jun 27, 2023

GustavoCaso Jun 27, 2023

marcotc Jun 27, 2023

GustavoCaso commented Jun 27, 2023

	def remote_features_enabled?
	Datadog::AppSec.send(:default_setting?, :ruleset)
	end

Add support for Option precedence #2915

Add support for Option precedence #2915

Conversation

marcotc commented Jun 19, 2023 • edited Loading

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GustavoCaso Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 26, 2023 • edited Loading

Codecov Report

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivoanjo Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GustavoCaso commented Jun 27, 2023

marcotc commented Jun 19, 2023 •

edited

Loading

GustavoCaso Jun 20, 2023 •

edited

Loading

codecov-commenter commented Jun 26, 2023 •

edited

Loading

ivoanjo Jun 27, 2023 •

edited

Loading