[PROF-10679] Fixes for comments from PR 3984

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;
 
@@ -1525,15 +1528,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;
@@ -1559,7 +1560,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
@@ -1645,62 +1646,48 @@ 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;
 
   trace_identifiers_result->valid = true;
 
   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;
 
@@ -1711,7 +1698,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

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

lib/datadog/profiling/component.rb

@@ -84,24 +84,14 @@ 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,
           tracer: 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
 

spec/datadog/core/configuration/settings_spec.rb

@@ -947,24 +947,30 @@
           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
 
           ['true', '1'].each do |value|
             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

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