Skip to content
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

[PROF-7409] Do not auto-enable new profiler when rugged gem is detected #2741

Merged
merged 2 commits into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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:
//
Expand Down Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this as a useful helper to test in issues like this (see individual commits for details)

}

// This structure is used to define a Ruby object that stores a pointer to a struct cpu_and_wall_time_worker_state
Expand Down Expand Up @@ -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;
}
}
15 changes: 13 additions & 2 deletions lib/datadog/profiling/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 ' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'<https://github.com/datadog/dd-trace-rb/issues/2721>. 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
Expand Down
24 changes: 22 additions & 2 deletions spec/datadog/profiling/component_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down