Update core configuration to use env and type options #2988

GustavoCaso · 2023-07-21T14:30:40Z

What does this PR do?

This code has been extracted from #2970 to facilitate the review process.

This PR focus on updating our current settings options to use env and type new functionality #2983

The only addition outside of updating the settings is making sure the default value gets duplicated before setting it a118dbd

There has been a discussion rather than duplicating. We should make sure that it is frozen to avoid unwanted changes to the value. #2970 (comment)
That change would be done on a separate PR

Motivation

Additional Notes

How to test the change?

GustavoCaso · 2023-07-24T11:09:41Z

lib/datadog/tracing/contrib/http/configuration/settings.rb

@@ -35,10 +41,26 @@ class Settings < Contrib::Configuration::Settings
            end

            option :error_status_codes do |o|


I haven't add the type to the error_status_code option, because in the tests the values used are Array and Range. I didn't wanted to change the backend logic for :array on this PR. So I will leave the decision to adding the type to the original author of this integration

GustavoCaso · 2023-07-24T11:09:54Z

lib/datadog/tracing/contrib/httpclient/configuration/settings.rb

@@ -35,10 +41,26 @@ class Settings < Contrib::Configuration::Settings
            end

            option :error_status_codes do |o|


Same as https://github.com/DataDog/dd-trace-rb/pull/2988/files#r1272116069

GustavoCaso · 2023-07-24T11:10:05Z

lib/datadog/tracing/contrib/httprb/configuration/settings.rb

@@ -35,10 +41,26 @@ class Settings < Contrib::Configuration::Settings
            end

            option :error_status_codes do |o|


Same as https://github.com/DataDog/dd-trace-rb/pull/2988/files#r1272116069

ivoanjo

It's in mega good shape! I left a few small notes, and I think after looking into them/discussing them, this is good to go.

One additional note: when running the specs manually I'm seeing a bunch of

/lib/datadog/core/configuration/options.rb:27: warning: instance variable @settings_name not initialized

I think this happens for top-level groups (e.g. c.agent) that don't get to have a custom class. I think the fix is as simple as settings_name = defined?(@settings_name) && @settings_name, as we do in a few other places in the codebase.

lib/datadog/core/configuration/option.rb

lib/datadog/core/diagnostics/environment_logger.rb

ivoanjo · 2023-07-25T11:01:43Z

lib/datadog/tracing/contrib/delayed_job/configuration/settings.rb

-            option :error_handler, experimental_default_proc: Tracing::SpanOperation::Events::DEFAULT_ON_ERROR
+            option :error_handler do |o|
+              o.type :proc
+              o.experimental_default_proc(&Tracing::SpanOperation::Events::DEFAULT_ON_ERROR)


Not for this PR -- but are we're ready to make a decision on the experimental_default_proc?

E.g. do we want to keep it as-is for now (probably rename it), or do you still have plans to change/remove it?

For now, no plans to remove it. We should rename it to default_proc 😄

Ack! Created https://github.com/DataDog/ruby-guild/issues/29 to record it (Datadog-only link)

ivoanjo · 2023-07-25T11:05:09Z

lib/datadog/tracing/contrib/opensearch/configuration/settings.rb

-              o.lazy
+              o.type :float
+              o.env Ext::ENV_ANALYTICS_SAMPLE_RATE
+              o.default 1.0
            end

            option :quantize, default: {}


@sarahchen6 Maybe worth adding a type here as a follow-up PR to Gustavo's?

spec/datadog/ci/configuration/settings_spec.rb

spec/datadog/core/configuration/settings_spec.rb

spec/datadog/tracing/configuration/settings_spec.rb

lib/datadog/tracing/configuration/settings.rb

ivoanjo · 2023-07-25T11:37:15Z

lib/datadog/tracing/contrib/rails/configuration/settings.rb

              o.on_set do |value|
                # Update ActionPack analytics too
-                Datadog.configuration.tracing[:action_pack][:analytics_enabled] = value
+                Datadog.configuration.tracing[:action_pack][:analytics_enabled] = value unless value.nil?
              end


It's subtle, but since both this setting, as well as the action_pack setting allow nil, I think perhaps that unless value.nil? is perhaps not the best idea (because in a way, nil previously meant "disable everything" and now only means "disable only rails".

In practice in most cases it'll be equivalent, but my thinking is, why change it, if we can keep the exact same previous behavior?

Done 00b1168

On a private Zoom call, we also discussed that rails and action_pack supporting nil for analytics_enabled is weird and that we should use a boolean value instead.

That change should be done on a separate PR

lib/datadog/core/configuration/settings.rb

marcotc · 2023-07-25T17:54:46Z

lib/datadog/tracing/configuration/settings.rb

-                                Datadog.logger.warn { "#{Tracing::Configuration::Ext::ClientIp::ENV_DISABLED} environment variable is deprecated, found set to #{disabled}, use #{Tracing::Configuration::Ext::ClientIp::ENV_ENABLED}=#{!disabled}" }
+                                Datadog::Core.log_deprecation do
+                                  "#{Tracing::Configuration::Ext::ClientIp::ENV_DISABLED} environment variable is deprecated, use #{Tracing::Configuration::Ext::ClientIp::ENV_ENABLED} instead."
+                                end


marcotc · 2023-07-25T17:58:36Z

lib/datadog/tracing/configuration/settings.rb

-              option :writer_options, default: ->(_i) { {} }
+              # @return [Hash]
+              option :writer_options do |o|
+                o.type :hash


This was changed to not nillable, but after inspecting our codebase, this change has no impact as the default {} is accepted everywhere nil would.

marcotc · 2023-07-25T17:59:29Z

lib/datadog/ci/configuration/settings.rb

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


When nothing is configured like this, is the type nillable?

When there's no type, the validation is disabled completely (https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/core/configuration/option.rb#L237) so it's a bit different from nillable (because nillable is a modifier after you select a specific type)

marcotc

Nothing really blocking this, awesome job!

ivoanjo

👍 LGTM!

For future PRs, I'd suggest not force-pushing after the first review is posted, as it makes it hard to see what has changed between revisions.

I'm the "trust, but verify" kinda person so I always like to give a look to the overall diff to try to spot new accidental issues, and this makes it hard to know if the older commits have changed :)

lib/datadog/core/configuration/settings.rb

github-actions bot added ci-app CI product for test suite instrumentation core Involves Datadog core libraries integrations Involves tracing integrations tracing labels Jul 21, 2023

GustavoCaso force-pushed the update-configurationto-use-env-and-type-options branch from 68b041f to 2ee5597 Compare July 21, 2023 14:34

delner assigned GustavoCaso Jul 21, 2023

GustavoCaso force-pushed the update-configurationto-use-env-and-type-options branch 4 times, most recently from 526ebfd to a118dbd Compare July 24, 2023 10:47

GustavoCaso marked this pull request as ready for review July 24, 2023 10:53

GustavoCaso requested a review from a team July 24, 2023 10:53

GustavoCaso mentioned this pull request Jul 24, 2023

Core configuration add env var and casting functionality #2970

Closed

GustavoCaso marked this pull request as draft July 24, 2023 10:55

GustavoCaso marked this pull request as ready for review July 24, 2023 11:01

GustavoCaso force-pushed the update-configurationto-use-env-and-type-options branch from a118dbd to ce03d19 Compare July 24, 2023 11:05

GustavoCaso commented Jul 24, 2023

View reviewed changes

ivoanjo reviewed Jul 25, 2023

View reviewed changes

GustavoCaso added 5 commits July 25, 2023 16:09

Update core configuration to use env and type options

5230f06

fix warning: instance variable @settings_name not initialized

5ba76f6

fix comment

6ce89de

revert startup_logs.enabled changes

165138d

remove unncesary settings.reset! calls on the specs

1e31049

GustavoCaso force-pushed the update-configurationto-use-env-and-type-options branch from ce03d19 to 1e31049 Compare July 25, 2023 14:10

revert modifying rails & action_pack analytics_enabled code

00b1168

marcotc reviewed Jul 25, 2023

View reviewed changes

marcotc approved these changes Jul 25, 2023

View reviewed changes

ivoanjo approved these changes Jul 26, 2023

View reviewed changes

lib/datadog/core/configuration/settings.rb Show resolved Hide resolved

revert removing comment for startup_logs.enabled

26de29d

GustavoCaso merged commit 242c247 into master Jul 26, 2023
162 checks passed

GustavoCaso deleted the update-configurationto-use-env-and-type-options branch July 26, 2023 13:14

github-actions bot added this to the 1.13.0 milestone Jul 26, 2023

TonyCTHsu mentioned this pull request Jul 30, 2023

Bump to version 1.13.0 #3009

Merged

anmarchenko mentioned this pull request Aug 17, 2023

Update core configs for datadog-ci DataDog/datadog-ci-rb#5

Merged

GustavoCaso mentioned this pull request Aug 30, 2023

rename experimental_default_proc to default_proc #3091

Merged

2 tasks

GustavoCaso mentioned this pull request Sep 25, 2023

remove variable helpers module from our configuration DSL #3152

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update core configuration to use env and type options #2988

Update core configuration to use env and type options #2988

GustavoCaso commented Jul 21, 2023 •

edited

Loading

GustavoCaso Jul 24, 2023

GustavoCaso Jul 24, 2023

GustavoCaso Jul 24, 2023

ivoanjo left a comment

ivoanjo Jul 25, 2023

GustavoCaso Jul 25, 2023

ivoanjo Jul 26, 2023

ivoanjo Jul 25, 2023

ivoanjo Jul 25, 2023

GustavoCaso Jul 25, 2023

marcotc Jul 25, 2023

marcotc Jul 25, 2023

marcotc Jul 25, 2023

ivoanjo Jul 26, 2023 •

edited

Loading

marcotc left a comment

ivoanjo left a comment

		@@ -35,10 +41,26 @@ class Settings < Contrib::Configuration::Settings
		end

		option :error_status_codes do \|o\|

Update core configuration to use env and type options #2988

Update core configuration to use env and type options #2988

Conversation

GustavoCaso commented Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

ivoanjo Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

marcotc left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

GustavoCaso commented Jul 21, 2023 •

edited

Loading

ivoanjo Jul 26, 2023 •

edited

Loading