diff --git a/ext/datadog_profiling_native_extension/collectors_thread_context.c b/ext/datadog_profiling_native_extension/collectors_thread_context.c index 5edec57ce1d..52b2e8df049 100644 --- a/ext/datadog_profiling_native_extension/collectors_thread_context.c +++ b/ext/datadog_profiling_native_extension/collectors_thread_context.c @@ -146,6 +146,8 @@ struct thread_context_collector_state { // Used to identify the main thread, to give it a fallback name VALUE main_thread; // Used when extracting trace identifiers from otel spans. Lazily initialized. + // Qtrue serves as a marker we've not yet extracted it; when we try to extract it, we set it to an object if + // successful and Qnil if not. VALUE otel_current_span_key; struct stats { @@ -193,7 +195,8 @@ struct trace_identifiers { struct otel_span { VALUE span; - VALUE span_context; + VALUE span_id; + VALUE trace_id; }; static void thread_context_collector_typed_data_mark(void *state_ptr); @@ -290,7 +293,7 @@ static void otel_without_ddtrace_trace_identifiers_for( VALUE thread, struct trace_identifiers *trace_identifiers_result ); -static struct otel_span otel_span_and_context_from(VALUE otel_context, VALUE otel_current_span_key); +static struct otel_span otel_span_from(VALUE otel_context, VALUE otel_current_span_key); static uint64_t otel_span_id_to_uint(VALUE otel_span_id); void collectors_thread_context_init(VALUE profiling_module) { @@ -437,7 +440,7 @@ static VALUE _native_new(VALUE klass) { state->time_converter_state = (monotonic_to_system_epoch_state) MONOTONIC_TO_SYSTEM_EPOCH_INITIALIZER; VALUE main_thread = rb_thread_main(); state->main_thread = main_thread; - state->otel_current_span_key = Qnil; + state->otel_current_span_key = Qtrue; state->gc_tracking.wall_time_at_previous_gc_ns = INVALID_TIME; state->gc_tracking.wall_time_at_last_flushed_gc_event_ns = 0; @@ -1540,15 +1543,13 @@ static VALUE read_otel_current_span_key_const(DDTRACE_UNUSED VALUE _unused) { } static VALUE get_otel_current_span_key(struct thread_context_collector_state *state) { - if (state->otel_current_span_key == Qnil) { + if (state->otel_current_span_key == Qtrue) { // Qtrue means we haven't tried to extract it yet // If this fails, we want to fail gracefully, rather than raise an exception (e.g. if the opentelemetry gem // gets refactored, we should not fall on our face) VALUE span_key = rb_protect(read_otel_current_span_key_const, Qnil, NULL); - // Marks when we failed to get the value, so we don't wasting resources trying - VALUE not_found_marker = Qfalse; - - state->otel_current_span_key = span_key != Qnil ? span_key : not_found_marker; + // Note that this gets set to Qnil if we failed to extract the correct value, and thus we won't try to extract it again + state->otel_current_span_key = span_key; } return state->otel_current_span_key; @@ -1574,7 +1575,7 @@ static void ddtrace_otel_trace_identifiers_for( if (resolved_numeric_span_id == Qnil) return; VALUE otel_current_span_key = get_otel_current_span_key(state); - if (otel_current_span_key == Qfalse) return; + if (otel_current_span_key == Qnil) return; VALUE current_trace = *active_trace; // ddtrace uses a different structure when spans are created from otel, where each otel span will have a unique ddtrace @@ -1660,43 +1661,29 @@ static void otel_without_ddtrace_trace_identifiers_for( if (context_storage == Qnil || !RB_TYPE_P(context_storage, T_ARRAY)) return; VALUE otel_current_span_key = get_otel_current_span_key(state); - if (otel_current_span_key == Qfalse) return; + if (otel_current_span_key == Qnil) return; int active_context_index = RARRAY_LEN(context_storage) - 1; if (active_context_index < 0) return; - struct otel_span active_span_and_context = otel_span_and_context_from(rb_ary_entry(context_storage, active_context_index), otel_current_span_key); - // If it exists, active_span_context is expected to be a OpenTelemetry::Trace::SpanContext (don't confuse it with OpenTelemetry::Context) - VALUE active_span_context = active_span_and_context.span_context; - if (active_span_context == Qnil) return; - - // Get the span id and trace id from the active span... - VALUE active_span_id = rb_ivar_get(active_span_context, at_span_id_id /* @span_id */); - VALUE active_span_trace_id = rb_ivar_get(active_span_context, at_trace_id_id /* @trace_id */); - if (active_span_id == Qnil || active_span_trace_id == Qnil || !RB_TYPE_P(active_span_id, T_STRING) || !RB_TYPE_P(active_span_trace_id, T_STRING)) return; + struct otel_span active_span = otel_span_from(rb_ary_entry(context_storage, active_context_index), otel_current_span_key); + if (active_span.span == Qnil) return; - VALUE local_root_span_id = active_span_id; - VALUE local_root_span = active_span_and_context.span; + struct otel_span local_root_span = active_span; // Now find the oldest span starting from the active span that still has the same trace id as the active span for (int i = active_context_index - 1; i >= 0; i--) { - struct otel_span span_and_context = otel_span_and_context_from(rb_ary_entry(context_storage, i), otel_current_span_key); - VALUE span_context = span_and_context.span_context; - if (span_context == Qnil) return; - - VALUE span_id = rb_ivar_get(span_context, at_span_id_id /* @span_id */); - VALUE span_trace_id = rb_ivar_get(span_context, at_trace_id_id /* @trace_id */); - if (span_id == Qnil || span_trace_id == Qnil || !RB_TYPE_P(span_id, T_STRING) || !RB_TYPE_P(span_trace_id, T_STRING)) return; + struct otel_span checking_span = otel_span_from(rb_ary_entry(context_storage, i), otel_current_span_key); + if (checking_span.span == Qnil) return; - if (rb_str_equal(active_span_trace_id, span_trace_id) == Qfalse) break; + if (rb_str_equal(active_span.trace_id, checking_span.trace_id) == Qfalse) break; - local_root_span_id = span_id; - local_root_span = span_and_context.span; + local_root_span = checking_span; } // Convert the span ids into uint64_t to match what the Datadog tracer does - trace_identifiers_result->span_id = otel_span_id_to_uint(active_span_id); - trace_identifiers_result->local_root_span_id = otel_span_id_to_uint(local_root_span_id); + trace_identifiers_result->span_id = otel_span_id_to_uint(active_span.span_id); + trace_identifiers_result->local_root_span_id = otel_span_id_to_uint(local_root_span.span_id); if (trace_identifiers_result->span_id == 0 || trace_identifiers_result->local_root_span_id == 0) return; @@ -1704,18 +1691,18 @@ static void otel_without_ddtrace_trace_identifiers_for( if (!state->endpoint_collection_enabled) return; - VALUE root_span_type = rb_ivar_get(local_root_span, at_kind_id /* @kind */); + VALUE root_span_type = rb_ivar_get(local_root_span.span, at_kind_id /* @kind */); // We filter out spans that don't have `kind: :server` if (root_span_type == Qnil || !RB_TYPE_P(root_span_type, T_SYMBOL) || SYM2ID(root_span_type) != server_id) return; - VALUE trace_resource = rb_ivar_get(local_root_span, at_name_id /* @name */); + VALUE trace_resource = rb_ivar_get(local_root_span.span, at_name_id /* @name */); if (!RB_TYPE_P(trace_resource, T_STRING)) return; trace_identifiers_result->trace_endpoint = trace_resource; } -static struct otel_span otel_span_and_context_from(VALUE otel_context, VALUE otel_current_span_key) { - struct otel_span failed = {.span = Qnil, .span_context = Qnil}; +static struct otel_span otel_span_from(VALUE otel_context, VALUE otel_current_span_key) { + struct otel_span failed = {.span = Qnil, .span_id = Qnil, .trace_id = Qnil}; if (otel_context == Qnil) return failed; @@ -1726,7 +1713,15 @@ static struct otel_span otel_span_and_context_from(VALUE otel_context, VALUE ote VALUE span = rb_hash_lookup(context_entries, otel_current_span_key); if (span == Qnil) return failed; - return (struct otel_span) {.span = span, .span_context = rb_ivar_get(span, at_context_id /* @context */)}; + // If it exists, span_context is expected to be a OpenTelemetry::Trace::SpanContext (don't confuse it with OpenTelemetry::Context) + VALUE span_context = rb_ivar_get(span, at_context_id /* @context */); + if (span_context == Qnil) return failed; + + VALUE span_id = rb_ivar_get(span_context, at_span_id_id /* @span_id */); + VALUE trace_id = rb_ivar_get(span_context, at_trace_id_id /* @trace_id */); + if (span_id == Qnil || trace_id == Qnil || !RB_TYPE_P(span_id, T_STRING) || !RB_TYPE_P(trace_id, T_STRING)) return failed; + + return (struct otel_span) {.span = span, .span_id = span_id, .trace_id = trace_id}; } // Otel span ids are represented as a big-endian 8-byte string diff --git a/lib/datadog/core/configuration/settings.rb b/lib/datadog/core/configuration/settings.rb index 46cf1209ab1..52ed9e7e222 100644 --- a/lib/datadog/core/configuration/settings.rb +++ b/lib/datadog/core/configuration/settings.rb @@ -491,7 +491,7 @@ def initialize(*_) # @default false option :preview_otel_context_enabled do |o| o.env 'DD_PROFILING_PREVIEW_OTEL_CONTEXT_ENABLED' - o.default 'false' + o.default false o.env_parser do |value| if value value = value.strip.downcase @@ -506,11 +506,11 @@ def initialize(*_) end o.setter do |value| if value == true - 'both' + :both elsif ['only', 'both', :only, :both].include?(value) - value.to_s + value.to_sym else - 'false' + false end end end diff --git a/lib/datadog/profiling/component.rb b/lib/datadog/profiling/component.rb index f7f7d8dc74a..5debd5ea3d2 100644 --- a/lib/datadog/profiling/component.rb +++ b/lib/datadog/profiling/component.rb @@ -84,16 +84,6 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:) end private_class_method def self.build_thread_context_collector(settings, recorder, optional_tracer, timeline_enabled) - otel_context_enabled = - case settings.profiling.advanced.preview_otel_context_enabled - when 'both' - :both - when 'only' - :only - else - false - end - Datadog::Profiling::Collectors::ThreadContext.new( recorder: recorder, max_frames: settings.profiling.advanced.max_frames, @@ -101,7 +91,7 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:) endpoint_collection_enabled: settings.profiling.advanced.endpoint.collection.enabled, timeline_enabled: timeline_enabled, waiting_for_gvl_threshold_ns: settings.profiling.advanced.waiting_for_gvl_threshold_ns, - otel_context_enabled: otel_context_enabled, + otel_context_enabled: settings.profiling.advanced.preview_otel_context_enabled, ) end diff --git a/spec/datadog/core/configuration/settings_spec.rb b/spec/datadog/core/configuration/settings_spec.rb index 1dbcbfd7eb5..5cc9974c69c 100644 --- a/spec/datadog/core/configuration/settings_spec.rb +++ b/spec/datadog/core/configuration/settings_spec.rb @@ -947,14 +947,14 @@ context 'is not defined' do let(:environment) { nil } - it { is_expected.to eq 'false' } + it { is_expected.to eq false } end - ['only', 'both', 'false'].each do |value| + ['only', 'both'].each do |value| context "is defined as #{value}" do - let(:environment) { value.to_s } + let(:environment) { value } - it { is_expected.to eq value } + it { is_expected.to eq value.to_sym } end end @@ -962,9 +962,15 @@ context "is defined as #{value}" do let(:environment) { value.to_s } - it { is_expected.to eq 'both' } + it { is_expected.to eq :both } end end + + context 'is defined as false' do + let(:environment) { 'false' } + + it { is_expected.to eq false } + end end end @@ -973,55 +979,30 @@ it 'updates the #preview_otel_context_enabled setting' do expect { settings.profiling.advanced.preview_otel_context_enabled = true } .to change { settings.profiling.advanced.preview_otel_context_enabled } - .from('false') - .to('both') - end - end - - context 'with false' do - it 'updates the #preview_otel_context_enabled setting' do - settings.profiling.advanced.preview_otel_context_enabled = true - - expect { settings.profiling.advanced.preview_otel_context_enabled = false } - .to change { settings.profiling.advanced.preview_otel_context_enabled } - .from('both') - .to('false') - end - end - - context 'with "only"' do - it 'updates the #preview_otel_context_enabled setting' do - expect { settings.profiling.advanced.preview_otel_context_enabled = 'only' } - .to change { settings.profiling.advanced.preview_otel_context_enabled } - .from('false') - .to('only') + .from(false) + .to(:both) end end - context 'with "both"' do - it 'updates the #preview_otel_context_enabled setting' do - expect { settings.profiling.advanced.preview_otel_context_enabled = 'both' } - .to change { settings.profiling.advanced.preview_otel_context_enabled } - .from('false') - .to('both') + ['only', 'both', :only, :both].each do |value| + context "with #{value.inspect}" do + it 'updates the #preview_otel_context_enabled setting' do + expect { settings.profiling.advanced.preview_otel_context_enabled = value } + .to change { settings.profiling.advanced.preview_otel_context_enabled } + .from(false) + .to(value.to_sym) + end end end - context 'with :only' do + context 'with false' do it 'updates the #preview_otel_context_enabled setting' do - expect { settings.profiling.advanced.preview_otel_context_enabled = :only } - .to change { settings.profiling.advanced.preview_otel_context_enabled } - .from('false') - .to('only') - end - end + settings.profiling.advanced.preview_otel_context_enabled = true - context 'with :both' do - it 'updates the #preview_otel_context_enabled setting' do - expect { settings.profiling.advanced.preview_otel_context_enabled = :both } + expect { settings.profiling.advanced.preview_otel_context_enabled = false } .to change { settings.profiling.advanced.preview_otel_context_enabled } - .from('false') - .to('both') + .from(:both) + .to(false) end end end diff --git a/spec/datadog/profiling/collectors/thread_context_spec.rb b/spec/datadog/profiling/collectors/thread_context_spec.rb index de8e890f2e9..718aa58818e 100644 --- a/spec/datadog/profiling/collectors/thread_context_spec.rb +++ b/spec/datadog/profiling/collectors/thread_context_spec.rb @@ -930,10 +930,10 @@ def otel_span_id_to_i(span_id) end context "when trace comes from otel sdk (warning)", if: otel_sdk_available? do - not_being_tested = otel_otlp_exporter_available? ? "otel sdk with ddtrace" : "otel sdk without ddtrace" + not_being_tested = otel_otlp_exporter_available? ? "otel sdk WITH ddtrace" : "otel sdk WITHOUT ddtrace" it "#{not_being_tested} is not being tested" do - skip "The tests for otel sdk with and without ddtrace are mutually exclusive, because ddtrace monkey " \ + skip "The tests for otel sdk WITH and WITHOUT ddtrace are mutually exclusive, because ddtrace monkey " \ "patches the otel sdk in a way that makes it hard to remove. To test both configurations, run this " \ "spec with and without `opentelemetry-exporter-otlp` on your Gemfile (hint: can be done using appraisals)." end