From 9b95598bb8fabd5963709a19d23ffe4e541be3fd Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Tue, 4 Apr 2023 12:37:41 +0100 Subject: [PATCH 1/2] Add helper to run Ruby block with sigprof disabled 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. --- .../collectors_cpu_and_wall_time_worker.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time_worker.c b/ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time_worker.c index a17ec80d08f..f9ca1283a15 100644 --- a/ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time_worker.c +++ b/ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time_worker.c @@ -170,6 +170,7 @@ static void sleep_for(uint64_t time_ns); static VALUE _native_allocation_count(DDTRACE_UNUSED VALUE self); static void on_newobj_event(DDTRACE_UNUSED VALUE tracepoint_data, DDTRACE_UNUSED void *unused); static void disable_tracepoints(struct cpu_and_wall_time_worker_state *state); +static VALUE _native_with_blocked_sigprof(DDTRACE_UNUSED VALUE self); // Note on sampler global state safety: // @@ -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); } // This structure is used to define a Ruby object that stores a pointer to a struct cpu_and_wall_time_worker_state @@ -869,3 +871,16 @@ static void disable_tracepoints(struct cpu_and_wall_time_worker_state *state) { rb_tracepoint_disable(state->gc_tracepoint); rb_tracepoint_disable(state->object_allocation_tracepoint); } + +static VALUE _native_with_blocked_sigprof(DDTRACE_UNUSED VALUE self) { + block_sigprof_signal_handler_from_running_in_current_thread(); + int exception_state; + VALUE result = rb_protect(rb_yield, Qundef, &exception_state); + unblock_sigprof_signal_handler_from_running_in_current_thread(); + + if (exception_state) { + rb_jump_tag(exception_state); + } else { + return result; + } +} From aff94bce0a0c6c0b96b71969b8b153ba61c6112a Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Tue, 4 Apr 2023 14:40:35 +0100 Subject: [PATCH 2/2] [PROF-7409] Do not auto-enable new profiler when rugged gem is detected **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 reproduces the issue for me every time. With this change, as expected, the issue no longer appears. --- lib/datadog/profiling/component.rb | 15 +++++++++++++-- spec/datadog/profiling/component_spec.rb | 24 ++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/lib/datadog/profiling/component.rb b/lib/datadog/profiling/component.rb index 611ac66019f..40cc2de225a 100644 --- a/lib/datadog/profiling/component.rb +++ b/lib/datadog/profiling/component.rb @@ -172,8 +172,19 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:) 'library used by the mysql2 gem) have a bug in their signal handling code that the new profiler can trigger. ' \ 'This bug (https://bugs.mysql.com/bug.php?id=83109) is fixed in libmysqlclient versions 8.0.0 and above. ' \ 'If your Linux distribution provides a modern libmysqlclient, you can force-enable the new CPU Profiling 2.0 ' \ - 'profiler by using the `DD_PROFILING_FORCE_ENABLE_NEW` or `c.profiling.advanced.force_enable_new_profiler` ' \ - 'settings.' + 'profiler by using the `DD_PROFILING_FORCE_ENABLE_NEW` environment variable or the ' \ + '`c.profiling.advanced.force_enable_new_profiler` setting.' \ + ) + return false + end + + 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 ' \ + '. If you still want to try out the new profiler, you ' \ + 'can force-enable it by using the `DD_PROFILING_FORCE_ENABLE_NEW` environment variable or the ' \ + '`c.profiling.advanced.force_enable_new_profiler` setting.' ) return false end diff --git a/spec/datadog/profiling/component_spec.rb b/spec/datadog/profiling/component_spec.rb index 2fd52d47123..c39a3de3b1b 100644 --- a/spec/datadog/profiling/component_spec.rb +++ b/spec/datadog/profiling/component_spec.rb @@ -402,8 +402,28 @@ end end - context 'when mysql2 gem is not available' do - include_context 'loaded gems', :mysql2 => nil + context 'when rugged gem is available' do + include_context 'loaded gems', :rugged => Gem::Version.new('1.6.3') + + before { allow(Datadog.logger).to receive(:warn) } + + it { is_expected.to be false } + + it 'logs a warning message mentioning that the legacy profiler is going to be used' do + expect(Datadog.logger).to receive(:warn).with(/Falling back to legacy profiler/) + + enable_new_profiler? + end + + context 'when force_enable_new_profiler is enabled' do + before { settings.profiling.advanced.force_enable_new_profiler = true } + + it { is_expected.to be true } + end + end + + context 'when mysql2 / rugged gem are not available' do + include_context('loaded gems', mysql2: nil, rugged: nil) it { is_expected.to be true } end