[PROF-8942] Enable Ruby timeline by default #3428
Conversation
github-actions
bot
added
core
**What does this PR do?** This PR enables the timeline feature for the Continuous Profiler by default. It introduces a new setting, `timeline_enabled`, which can also be controlled by setting the `DD_PROFILING_TIMELINE_ENABLED` environment variable. It also deprecates the old `experimental_timeline_enabled` + it's environment variable counterpart. **Motivation:** With the latest round of improvements to libdatadog, we're confident about enabling this feature by default. **Additional Notes:** I'm opening this PR as a draft as it still depends on DataDog/libdatadog#293 being released + a few more rounds of internal validation. **How to test the change?** Try to profile any application, validate that you get timeline data without further configuration.
ivoanjo
force-pushed
the
ivoanjo/prof-8942-enable-ruby-timeline-default
branch
from
6d2de9a
to
e3210c5
Compare
|
Marking this as good to go. For Datadog folks, search google drive for "Review of Ruby profiler timeline feature before enabling by default (2024-02)" for a review of benchmarks with the timeline feature. Note to self: after this gets merged, we need to do a pass on our benchmarking configurations, due to the new default/new environment variable. |
| @@ -424,9 +424,29 @@ def initialize(*_) | |||
| # | |||
| # @default `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED` environment variable as a boolean, otherwise `false` | |||
| option :experimental_timeline_enabled do |o| | |||
| o.after_set do |_, _, precedence| | |||
There was a problem hiding this comment.
Slightly off-topic: I've noticed we don't do this in the other deprecated settings but wonder if we should:
Keep the o.env setting so that we warn if deprecated env variables are left lying around?
There was a problem hiding this comment.
It's an interesting idea...
Although at this point, I was thinking that for 2.0 we'd remove all these settings 🤔. Do you think we should keep them around, potentially with the o.env as well?
There was a problem hiding this comment.
Nah good time to clean up house but I think it'd make sense to take this into account during the lifetime of 2.x and the deprecations that happen therein
| # If you needed to disable this, please tell us why on <https://github.com/DataDog/dd-trace-rb/issues/new>, | ||
| # so we can fix it! | ||
| # | ||
| # TODO: In the future this setting may be deprecated and the timeline may no longer be optional. |
There was a problem hiding this comment.
Minor: Is this TODO useful? It's a given that in the future anything may happen 😂
There was a problem hiding this comment.
You're right, it's a bit too much futurology. Removed in c862874 .
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3428 +/- ##
==========================================
- Coverage 98.23% 98.23% -0.01%
==========================================
Files 1277 1277
Lines 75141 75149 +8
Branches 3544 3546 +2
==========================================
+ Hits 73817 73824 +7
- Misses 1324 1325 +1 ☔ View full report in Codecov by Sentry. |
… timeline **What does this PR do?** As soon as we merge #3428 the profiler will default to timeline being enabled. This means that the "profiler" configurations will all have timeline, so having separate configurations for timeline does not make any sense. **Motivation:** Remove timeline testing configurations that no longer make sense. **Additional Notes:** For the configuration with GC profiling, I've removed the `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED` environment variable, since, again, timeline is on by default. We could potentially keep a configuration without timeline around, but we already have a lot of configurations, so for now I've chosen not to do so. **How to test the change?** This can be tested by manually triggering a deploy to the benchmarking platform.
What does this PR do?
This PR enables the timeline feature for the Continuous Profiler by default.
It introduces a new setting,
timeline_enabled, which can also be controlled by setting theDD_PROFILING_TIMELINE_ENABLEDenvironment variable.It also deprecates the old
experimental_timeline_enabled+ it's environment variable counterpart.Motivation:
With the latest round of improvements to libdatadog, we're confident about enabling this feature by default.
Additional Notes:
I'm opening this PR as a draft as it still depends on DataDog/libdatadog#293 being released + a few more rounds of internal validation.✅ FixedThis PR also will need to be updated with the changes from #3357 OR the reverse will need to happen -- whichever one is merged first.✅ FixedHow to test the change?
Try to profile any application, validate that you get timeline data without further configuration.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.