-
Notifications
You must be signed in to change notification settings - Fork 369
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
[NO-TICKET] Replace profiling.advanced.experimental_allocation_enabled
with profiling.allocation_enabled
and remove experimental warning
#3520
Conversation
…ofiling.allocation_enabled` As we are no longer considering this option experimental, let's rename it to reflect that. Furthermore, enabling allocation profiling is not considered an "advanced" option. In the process, let's deprecate the old `profiling.advanced.experimental_allocation_enabled` and leave a note asking customers to update their `Datadog.configuration` block. I did spend some time trying to get the value from `experimental_allocation_enabled` into the new setting automatically, but because they are in separate options groups, I wasn't able to find a way to access the new setting from the old one. I was able to still support the old environment variable, so at least we have that.
It's no longer considered experimental.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3520 +/- ##
==========================================
- Coverage 98.24% 98.23% -0.01%
==========================================
Files 1275 1275
Lines 75273 75295 +22
Branches 3554 3556 +2
==========================================
+ Hits 73949 73968 +19
- Misses 1324 1327 +3 ☔ View full report in Codecov by Sentry. |
'The profiling.advanced.experimental_allocation_enabled setting has been deprecated for removal and ' \ | ||
'no longer does anything. Please remove it from your Datadog.configure block. ' \ | ||
'Allocation profiling is now controlled by the profiling.allocation_enabled setting instead.' | ||
) |
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.
Related to your PR description, is the problem that set_option(:allocation_enabled, value)
would act on the advanced
settings, not on the parent profiling
settings?
I'm still not fully acquainted with the option code but it could be interesting if you could do allocation_enabled_option = option :allocation_enabled
and then call allocation_enabled_option.set(value)
from here. Although yeah if not heavily restricted I can see how this can create a mess of setting value discoverability over time 😂
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.
Related to your PR description, is the problem that set_option(:allocation_enabled, value) would act on the advanced settings, not on the parent profiling settings?
Yes, exactly -- unless I saw it completely wrong, the option groups objects form a tree and are linked only to their children.
I'm still not fully acquainted with the option code but it could be interesting if you could do allocation_enabled_option = option :allocation_enabled and then call allocation_enabled_option.set(value) from here. Although yeah if not heavily restricted I can see how this can create a mess of setting value discoverability over time 😂
Hmm... I'll admit I didn't try something like this. Let me give a final try to see if I can make it work.
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.
Ok so after thinking about it some more, here's how far I got:
-
allocation_enabled_option = option :allocation_enabled
was a good try, but doesn't quite work because we get anOptionDefinition
instance. The actualOption
instance that keeps the data is created separately (inresolve_option
) and so we don't actually have the object we need at this point. -
It's possible to add a link to the parent group when defining the options -- here but I'm somewhat reluctant to add a feature to the options system (which is already quite complex) just for this
-
One thing that occurred to me would be inverting the order of the options. That is,
allocation_enabled
, since it's higher in the tree, is able to access theexperimental_allocation_enabled
, so one option would be forallocation_enabled
to write its result intoexperimental_allocation_enabled
and then have the code read the result fromexperimental_allocation_enabled
until we deprecated it. That... would somewhat work, although it would interfere with the deprecation warning messages (e.g. not sure if we could still have them).
Overall... Given the current number of users we have using the experimental feature and that we're going to remove the option soon (e.g. for 2.0), I'm not sure it's worth trying really hard to support reading from the old config. (But let me know if you disagree!)
And overall our settings system is too damn complex >_>
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.
Given you approved, I'm going to go ahead and merge this, but happy to keep iterating on it as a separate PR if you're not convinced by my attempts above ;)
What does this PR do?
In the next dd-trace-rb release, allocation profiling is no longer experimental. To reflect this, this PR includes two changes:
profiling.advanced.experimental_allocation_enabled
with the newprofiling.allocation_enabled
optionMotivation:
Update code to match non-experimental status of feature.
Additional Notes:
I've decided to move the new setting outside the
advanced
configuration group, as enabling allocation profiling is not considered an "advanced" option.To allow for smooth upgrades, I've left
profiling.advanced.experimental_allocation_enabled
in place with a deprecation warning, but unfortunately I was not able to find a nice way to automatically set the newprofiling.allocation_enabled
when the old one was set.This means that users that had previously-enabled allocation profiling via code will need to update their configuration after upgrading, otherwise they won't get allocation profiling.
On the bright side, I was still able to support the old environment variable, so any customers that enabled the feature previously using that path will still get allocation profiling, plus a deprecation warning suggesting moving to the new environment variable.
How to test the change?
Change includes test coverage.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.