Stricter validation for integer and float configuration #3332

marcotc · 2023-12-14T22:44:48Z

2.0 Upgrade Guide notes
Configuration options of types ':int' and ':float' now have stricter type validation:

Fractional numbers are no longer considered valid for ':int' options.
Empty values (nil or empty string) are no longer considered valid for ':int' or ':float' options.

This applies to both environment variable and programmatic configuration.
Here's the list of all affected options:
[Insert all :int and :float options at release time!!!]

The environment variable DD_EXPERIMENTAL_SKIP_CONFIGURATION_VALIDATION has been removed.

Motivation:
It was possible to configure options with empty values (e.g. DD_TRACE_SAMPLE_RATE=) and end up with a sampling rate of 0.0, because of the logic value.to_f. The same applies to integer options (value.to_i).
Non-numeric values would also be converted to zero.
This means users do not know what their effective configuration will be, and are not given feedback on how to address configuration issues.

How to test the change?
There are unit tests for all the changes.

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

codecov-commenter · 2023-12-14T22:50:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (277b90f) 98.10% compared to head (8130644) 98.10%.

Additional details and impacted files

@@           Coverage Diff           @@
##              2.0    #3332   +/-   ##
=======================================
  Coverage   98.10%   98.10%           
=======================================
  Files        1252     1252           
  Lines       72342    72348    +6     
  Branches     3387     3387           
=======================================
+ Hits        70974    70980    +6     
  Misses       1368     1368

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ivoanjo

I'm a bit short on time today so I didn't look at this very thoroughly but here's a quick pass :)

ivoanjo · 2023-12-15T11:40:12Z

lib/datadog/core/configuration/option.rb

+        rescue ArgumentError => e
+          # Try also parsing as a whole floating point numbers
+          begin
+            f = Float(value)
+
+            # Check for whole numbers
+            raise ArgumentError, "#{value} is not a whole number" unless f.truncate == f


To be honest... This seems like a bit of a surprising semantics. If I'm in a rush and see something on my config that I want to update:

DD_PROFILING_HOW_MUCH_IT_ROCKS=100.0

and then I go, oh, I want to make it rock a bit less

DD_PROFILING_HOW_MUCH_IT_ROCKS=95.5

and it blows up -- this seems weird. That .0 that someone on my team left misled me into thinking this value was supposed to be a float, and then it turns out it isn't and it doesn't work.

What do you think about allowing whole floats as integer @TonyCTHsu @ekump? (e.g. 100.0 is allowed for :int configuration options). Or should be strict and say that this is not allowed (e.g. 100.0 would be rejected, and the user should replace it with an explicit 100)

@ivoanjo, how about a warning on whole floats? I think that might be a good compromise.

how about a warning on whole floats?

I would still kinda prefer not to accept 'em, but I can live with that as a compromise ;)

We had discussed moving this to a warning, which I think is better than the current raise. If we do keep the raise, I would suggest making it a bit clear what needs to be fixed in the current error -- e.g. "Setting expects only integer settings, you've provided #{value}"?

ivoanjo · 2023-12-15T11:44:29Z

lib/datadog/core/configuration/option.rb

              "The option #{@definition.name} is using an unsupported type option for env coercion `#{@definition.type}`"
          end
        end

+        def coerce_env_var_int(value)
+          # Strict integer parsing
+          Integer(value)


I learned (depressingly) the other day that Integer(value) actually supports hex and octal literals => e.g. Integer("010") => 8 and Integer("0x10") => 16.

TL;DR the correct form is Integer(value, 10) to specifically ask it to parse only as base 10. Now I'll go back to wondering why this isn't the default >_>

I think if only 0x10 (and base 10) were allowed, we wouldn't bat an eye, but octals ruin it for everyone.

Yeah, it's hard to accidentally have hex, but so easy to accidentally have octal >_>

ivoanjo

Left a few final notes :)

ivoanjo · 2024-01-22T10:25:33Z

lib/datadog/core/configuration/option.rb

+        rescue ArgumentError => e
+          # Try also parsing as a whole floating point numbers
+          begin
+            f = Float(value)
+
+            # Check for whole numbers
+            raise ArgumentError, "#{value} is not a whole number" unless f.truncate == f


We had discussed moving this to a warning, which I think is better than the current raise. If we do keep the raise, I would suggest making it a bit clear what needs to be fixed in the current error -- e.g. "Setting expects only integer settings, you've provided #{value}"?

lib/datadog/core/configuration/option.rb

marcotc · 2024-01-22T22:39:41Z

@ivoanjo I talked to @TonyCTHsu and I was convinced to have stricter Integer validation, which aligns with your original suggestion.

ivoanjo

I think we should still allow users to provide Integer to float settings when doing it via the configure block, but otherwise this looks good to me 👍

ivoanjo · 2024-01-23T08:28:17Z

lib/datadog/core/configuration/option.rb

+          when :float
+            value.is_a?(Float) || value.is_a?(Rational)


I think allowing users to provide integer values for floats is a nice UX touch -- e.g. if you're providing a percentage, often you may want to say 50 and not 50.0 or something like that.

So I don't think we should lose that.

TonyCTHsu

Should we mention the removal of ENV['DD_EXPERIMENTAL_SKIP_CONFIGURATION_VALIDATION'] in the upgrade guide?

ivoanjo

👍 LGTM

marcotc requested a review from a team as a code owner December 14, 2023 22:44

marcotc self-assigned this Dec 14, 2023

github-actions bot added the core Involves Datadog core libraries label Dec 14, 2023

ivoanjo reviewed Dec 15, 2023

View reviewed changes

marcotc requested a review from a team December 15, 2023 18:04

marcotc requested a review from ivoanjo January 19, 2024 23:58

ivoanjo reviewed Jan 22, 2024

View reviewed changes

marcotc added 3 commits January 22, 2024 14:24

Stricter validation for integer and float configuration

c02fcd0

Only allow base 10 numbers

79fa20c

Simplify integer accepted types

90a7f10

marcotc force-pushed the better-type-validation branch from b3e8f7c to 90a7f10 Compare January 22, 2024 22:37

marcotc requested a review from a team as a code owner January 22, 2024 22:37

marcotc requested a review from TonyCTHsu January 22, 2024 22:39

Fix non-tracing tests

8130644

marcotc requested a review from a team as a code owner January 23, 2024 00:30

ivoanjo approved these changes Jan 23, 2024

View reviewed changes

TonyCTHsu approved these changes Jan 24, 2024

View reviewed changes

Allow programmatic ints for type float

76c83a4

ivoanjo approved these changes Jan 25, 2024

View reviewed changes

marcotc merged commit 79870d4 into 2.0 Jan 25, 2024
149 of 150 checks passed

marcotc deleted the better-type-validation branch January 25, 2024 20:44

TonyCTHsu added this to the 2.0 milestone Feb 20, 2024

ivoanjo added the 2.0 label Mar 14, 2024

TonyCTHsu modified the milestones: 2.0, 2.0.0.beta1 Mar 22, 2024

This was referenced Apr 12, 2024

Bump to version 2.0.0 TonyCTHsu/dd-trace-rb#1

Closed

Bump to version 2.0.0 TonyCTHsu/dd-trace-rb#2

Closed

Update changelog with release 2.0.0.beta2 #3599

Merged

TonyCTHsu mentioned this pull request Apr 18, 2024

Bump to version 2.0.0.beta2 #3600

Merged

TonyCTHsu mentioned this pull request Jun 6, 2024

Bump to version 2.0.0 #3687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stricter validation for integer and float configuration #3332

Stricter validation for integer and float configuration #3332

marcotc commented Dec 14, 2023 •

edited

codecov-commenter commented Dec 14, 2023 •

edited

ivoanjo left a comment

ivoanjo Dec 15, 2023

marcotc Dec 15, 2023

marcotc Dec 15, 2023

ivoanjo Dec 15, 2023

ivoanjo Jan 22, 2024

ivoanjo Dec 15, 2023

marcotc Dec 15, 2023

marcotc Dec 15, 2023

ivoanjo Dec 15, 2023

ivoanjo left a comment

ivoanjo Jan 22, 2024

marcotc commented Jan 22, 2024

ivoanjo left a comment

ivoanjo Jan 23, 2024

TonyCTHsu left a comment

ivoanjo left a comment

Stricter validation for integer and float configuration #3332

Stricter validation for integer and float configuration #3332

Conversation

marcotc commented Dec 14, 2023 • edited

codecov-commenter commented Dec 14, 2023 • edited

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

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

marcotc commented Jan 22, 2024

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyCTHsu left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

marcotc commented Dec 14, 2023 •

edited

codecov-commenter commented Dec 14, 2023 •

edited