Skip to content

Commit

Permalink
Merge pull request #3987 from DataDog/ivoanjo/prof-10679-feedback
Browse files Browse the repository at this point in the history 
[PROF-10679] Fixes for comments from PR 3984
  • Loading branch information
@ivoanjo
ivoanjo authored Oct 10, 2024
2 parents 8db9e44 + 222b4d4 commit 58b17f0
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 100 deletions.
71 changes: 33 additions & 38 deletions ext/datadog_profiling_native_extension/collectors_thread_context.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -1660,62 +1661,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;

Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions lib/datadog/core/configuration/settings.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
12 changes: 1 addition & 11 deletions lib/datadog/profiling/component.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
71 changes: 26 additions & 45 deletions spec/datadog/core/configuration/settings_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions spec/datadog/profiling/collectors/thread_context_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 58b17f0

Please sign in to comment.