[PROF-7409] Do not auto-enable new profiler when rugged gem is detected #2741
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is useful when testing if a specific bit of code is affected by sigprof signal delivery. I'm using it to test the issue reported in #2721. I think this helper is small enough/useful enough to keep around for later.
**What does this PR do?**: This PR adds the `rugged` gem to the list of gems that prevent the new CPU Profiling 2.0 profiler from being auto-enabled. This is needed because the `rugged` gem (more specifically, its C-level dependencies) do not correctly handle the unix signals that the new profiler uses. We plan to do more than just add this workaround, but until we do so, this change will prevent customers from being bitten by the issue. **Motivation**: Make sure that customers being moved to the new CPU Profiling 2.0 profiler have a smooth experience. **Additional Notes**: N/A **How to test the change?**: The snippet shared by the customer in <#2721> reproduces the issue for me every time. With this change, as expected, the issue no longer appears.
ivoanjo
commented
Apr 4, 2023
| @@ -222,6 +223,7 @@ void collectors_cpu_and_wall_time_worker_init(VALUE profiling_module) { | |||
| rb_define_singleton_method(testing_module, "_native_simulate_handle_sampling_signal", _native_simulate_handle_sampling_signal, 0); | |||
| rb_define_singleton_method(testing_module, "_native_simulate_sample_from_postponed_job", _native_simulate_sample_from_postponed_job, 0); | |||
| rb_define_singleton_method(testing_module, "_native_is_sigprof_blocked_in_current_thread", _native_is_sigprof_blocked_in_current_thread, 0); | |||
| rb_define_singleton_method(testing_module, "_native_with_blocked_sigprof", _native_with_blocked_sigprof, 0); | |||
There was a problem hiding this comment.
I've added this as a useful helper to test in issues like this (see individual commits for details)
TonyCTHsu
approved these changes
Apr 5, 2023
marcotc
reviewed
Apr 5, 2023
| if Gem.loaded_specs['rugged'] | ||
| Datadog.logger.warn( | ||
| 'Falling back to legacy profiler because rugged gem is installed. Some operations on this gem are ' \ | ||
| 'currently incompatible with the new CPU Profiling 2.0 profiler, as detailed in ' \ |
There was a problem hiding this comment.
Is there a subset of the rugged gem that's safe, like an specific version? I'm asking so users know when it makes sense to try to enable profiling 2.0 anyway.
There was a problem hiding this comment.
Currently, there isn't. It's on my next-things-to-do list to report this upstream and even seeing if I can submit a PR with a fix or workaround, which would allow us to indeed make this finer-grained than what this PR does.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
This PR adds the
ruggedgem to the list of gems that prevent the new CPU Profiling 2.0 profiler from being auto-enabled.This is needed because the
ruggedgem (more specifically, its C-level dependencies) do not correctly handle the unix signals that the new profiler uses.We plan to do more than just add this workaround, but until we do so, this change will prevent customers from being bitten by the issue.
Motivation:
Make sure that customers being moved to the new CPU Profiling 2.0 profiler have a smooth experience.
Additional Notes:
N/A
How to test the change?:
The snippet shared by the customer in #2721 reproduces the issue for me every time.
With this change, as expected, the issue no longer appears.