fix: ensure uds always takes precedence over http if both configurations are defined #4024
Conversation
BenchmarksBenchmark execution time: 2024-11-01 14:15:42 Comparing candidate commit 09b6274 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 29 metrics, 2 unstable metrics. |
| can_use_uds? && !mixed_http_and_uds? | ||
| # When we have mixed settings for http/https and uds, we print a warning | ||
| # and use the uds settings. | ||
| unless defined?(@mixed_http_and_uds) | ||
| @mixed_http_and_uds = (configured_hostname || configured_port) && can_use_uds? | ||
| if @mixed_http_and_uds | ||
| warn_if_configuration_mismatch( | ||
| [ | ||
| DetectedConfiguration.new( | ||
| friendly_name: 'configuration for unix domain socket', | ||
| value: parsed_url.to_s, | ||
| ), | ||
| DetectedConfiguration.new( | ||
| friendly_name: 'configuration of hostname/port for http/https use', | ||
| value: "hostname: '#{hostname}', port: '#{port}'", | ||
| ), | ||
| ] | ||
| ) | ||
| end | ||
| end | ||
| can_use_uds? |
There was a problem hiding this comment.
With these changes we violate SRP. Question-mark methods doesn't suppose to set any variables, but provide only true/false checks.
Solution: Rename existing method mixed_http_and_uds? into mixed_http_and_uds and adjust should_use_uds? accordingly.
There was a problem hiding this comment.
The variable being set is just a cache for the value being returned or am I missing something?
There was a problem hiding this comment.
Memoizing values plays an important part when combined with the warnings: it means we don't repeat warnings just because some helper function queried the same info again.
|
Thanks for picking this up! I assume from
That other dd libraries behave in the way we're changing Ruby to adhere to. Do you mind if I ask for some references? Is there perhaps a system-test for this that we can use to ensure we don't regress across libraries? |
We have this system-test: https://github.com/DataDog/system-tests/blob/5e90616079fc11656348026f5f771aea8bafa748/tests/parametric/test_config_consistency.py#L145. |
spec/datadog/core/configuration/agent_settings_resolver_spec.rb
Outdated
Show resolved
Hide resolved
spec/datadog/core/configuration/agent_settings_resolver_spec.rb
Outdated
Show resolved
Hide resolved
spec/datadog/core/configuration/agent_settings_resolver_spec.rb
Outdated
Show resolved
Hide resolved
spec/datadog/core/configuration/agent_settings_resolver_spec.rb
Outdated
Show resolved
Hide resolved
Amazing, thanks for sharing ❤️ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4024 +/- ##
=======================================
Coverage 97.71% 97.71%
=======================================
Files 1338 1338
Lines 80248 80249 +1
Branches 4016 4016
=======================================
+ Hits 78418 78419 +1
Misses 1830 1830 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Sergey Fedorov <oni.strech@gmail.com>
TonyCTHsu
force-pushed
the
munir/uds-should-take-precedence-over-http-if-both-defined
branch
from
14480fe
to
ebaa0bf
Compare
TonyCTHsu
deleted the
munir/uds-should-take-precedence-over-http-if-both-defined
branch
What does this PR do?
Ensures uds configurations are parsed in a manner that is consistent with other libraries.
Motivation:
DD_TRACE_AGENT_URLis applied in a consistent manner.Change log entry
Additional Notes:
How to test the change?