-
Notifications
You must be signed in to change notification settings - Fork 399
[APMAPI-1594] (DO-NOT-MERGE) Add support for config inversion on datadog-ci-rb #5256
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.
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
base: master
Are you sure you want to change the base?
Conversation
| LOG_DEPRECATIONS_ONLY_ONCE[deprecations] ||= Datadog::Core::Utils::OnlyOnce.new | ||
| LOG_DEPRECATIONS_ONLY_ONCE[deprecations].run do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The OnlyOnce is designed to be thread safe; when used in this manner it's not going to be thread-safe. I think in practice that's fine (I don't think we need to rely on its thread-safety properties here), but perhaps it's worth leaving a comment along the lines of
# This way of initializing the `OnlyOnce` is not thread-safe but that's OK here
LOG_DEPRECATIONS_ONLY_ONCE[deprecations] ||= Datadog::Core::Utils::OnlyOnce.new
|
BenchmarksBenchmark execution time: 2026-01-21 14:11:50 Comparing candidate commit 789eda4 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 43 metrics, 2 unstable metrics. scenario:profiling - gvl benchmark samples
|
| alias_method :member?, :key? | ||
|
|
||
| # This method will be used by datadog-ci-rb once it will bump its minimum dependancy of dd-trace-rb to 2.27.0. | ||
| # In the meantime, it uses ConfigHelper#instance_variable_get(:@source_env) to set environment variables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this step and not just update to 2.27.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious myself -- I think that's a question for @anmarchenko -- would it be worth doing that bump first there and then coming back to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that some customers uses an old version of dd-trace-rb with latest version of datadog-ci-rb and we want to maintain compatibility ? If not then we can indeed skip this step and this should make it easier than trying to maintain compatibility without introducing breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datadog-ci-rb once it will bump its minimum dependancy of dd-trace-rb to 2.27.0
I wasn't planning to bump the minimum dependency because I will need then to go other tools that do sanity checks for customer setups (test-visibility-instrumentation-script, ddtest, etc) and adapt the checks to correctly tell the customer which versions we support. This isn't something we plan to invest right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general as much as possible I think we should "push" users to use the latest versions because there's lots of bugs on the old ones AND it creates a big matrix of combinations we don't really test with (any dd-trace-rb x any datadog-ci-rb is a lot of versions...).
But I do think the ultimate owner of this is @anmarchenko so do think it makes sense to leave the decision to him ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with @ivoanjo, this is what I tried to do as much as possible, but unfortunately life always gets more complicated over time, so this becomes harder when we have more tools that we need to keep in sync.
I would be happy to bump this minimum version for a customer-facing feature that unlocks real value for our customers - then it would a worth endeavour to upgrade for us and the customers. This PR is for internal purposes and we should not make any changes here that might affect customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify: This PR does not introduce any breaking change to the exisiting codebase of datadog-ci-rb and should not change the behaviour of previously released datadog-ci-rb gems (If someone uses datadog-ci-rb 1.26.0 with dd-trace-rb release that contains these changes, there will be no change in behaviour)
And that is also true for the other way around, if someone uses the version of datadog-ci-rb that contains this PR, it will work with any version of dd-trace-rb that is supported (I've tested with 2.20.0, which doesn't even include config inversion/ConfigHelper code, 2.21.0 which contains the v1 code of config inversion and skip any checks if it detects datadog-ci-rb, and with this PR)
We do not need to bump the dependency of dd-trace-rb on datadog-ci-rb
| alias_method :include?, :key? | ||
| alias_method :member?, :key? | ||
|
|
||
| # This method will be used by datadog-ci-rb once it will bump its minimum dependancy of dd-trace-rb to 2.27.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # This method will be used by datadog-ci-rb once it will bump its minimum dependancy of dd-trace-rb to 2.27.0. | |
| # This method will be used by datadog-ci-rb after it increases minimum required version of dd-trace-rb to 2.27.0. |
| # datadog-ci-rb is using dd-trace-rb config DSL, which uses this method. | ||
| # Until we've correctly implemented support for datadog-ci-rb, we disable config inversion if ci is enabled. | ||
| if !defined?(::Datadog::CI) && | ||
| if (!defined?(::Datadog::CI) || Gem::Version.new(::Datadog::CI::VERSION::STRING) >= Gem::Version.new('1.27.0')) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.27.0 is not released yet, it's a bit sketchy to merge this to our master because if we were to release tomorrow I assume this code will break datadog-ci.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this PR should not be merged until @anmarchenko gives the green light to merge both this PR and DataDog/datadog-ci-rb#459 at the same time. I will change the title of the PR to add "DO-NOT-MERGE"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not check for version ever, we must check for capabilities (like specific class present)
because otherwise we will not be able to release datadog-ci gem because CI will be broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI will not be broken but the checks will be skipped. But checking for capabilities is much better because it will not wait for the version to be bumped by fast_castle to actually do the checks, thanks for the feedback!
| module Deprecations | ||
| LOG_DEPRECATIONS_ONLY_ONCE = Datadog::Core::Utils::OnlyOnce.new | ||
| # Hash of OnlyOnce, as we may call log_deprecations_from_all_sources from datadog-ci-rb too with different deprecations set | ||
| # This way of initializing the `OnlyOnce` is not thread-safe but that's OK here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but this comment goes with the write on line 16, not with the assignment on line 13.
| module Configuration | ||
| module Deprecations | ||
| LOG_DEPRECATIONS_ONLY_ONCE = Datadog::Core::Utils::OnlyOnce.new | ||
| # Hash of OnlyOnce, as we may call log_deprecations_from_all_sources from datadog-ci-rb too with different deprecations set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Hash of OnlyOnce, as we may call log_deprecations_from_all_sources from datadog-ci-rb too with different deprecations set | |
| # Hash of OnlyOnce instances, as we may call log_deprecations_from_all_sources from datadog-ci-rb too with different deprecations set |
The current comment reads as a hash (digest) of a single OnlyOnce instance which is confusing and not what is actually happening.
|
|
||
| # This method will be used by datadog-ci-rb once it will bump its minimum dependancy of dd-trace-rb to 2.27.0. | ||
| # In the meantime, it uses ConfigHelper#instance_variable_get(:@source_env) to set environment variables. | ||
| def []=(name, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused why this method is being added and immediately stated to be unused in the upcoming datadog-ci release, can a comment be added explaining this in more detail please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datadog-ci-rb is compatible with dd-trace-rb 2.4.0. To make config inversion work on datadog-ci-rb, we monkey-patch DATADOG_ENV, which was introduced in dd-trace-rb 2.21.0 IIRC. For version lower that 2.21.0, DATADOG_ENV will be equal to ENV, but versions above that is a ConfigHelper instance. It is not planned to bump dd-trace-rb dependency in datadog-ci-rb yet, but I added this so it is future proof, and can be used to simplify the code once datadog-ci-rb bumps dd-trace-rb dependency
What does this PR do?
This PR adds support for datadog-ci-rb on config inversion.
Motivation:
We miss all of datadog-ci-rb configs on the config registry
Change log entry
None.
Additional Notes:
DataDog/datadog-ci-rb#459 depends on this PR
How to test the change?