From 00b954641b03a6cc259076d261bf7cf2703f44dc Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Fri, 6 Jan 2023 12:23:23 +0000 Subject: [PATCH 1/5] Attach local root span id to profiling samples as a number not as a string This is a newly-added feature to both the profiling backend, as well as libdatadog. Instead of sending the local root span ids as strings (that take up space in the string table, etc), we can now send them as numbers. We already did a similar migration for span ids in #2476, but at the time we did not change the local root span ids because the libdatadog `ddog_prof_Profile_set_endpoint` API could not properly handle the numeric ids (this has been changed in https://github.com/DataDog/libdatadog/pull/80 ). As far as testing goes, I could have chosen to abstract away this change from our specs BUT I chose to make it explicit and changed the specs to match. I like having visibility in the specs of the values being encoded and passed around as numbers and not strings. --- .../collectors_cpu_and_wall_time.c | 15 +++--------- .../collectors_stack.c | 17 ++++++++++--- .../stack_recorder.c | 5 ++-- .../stack_recorder.h | 2 +- .../collectors/cpu_and_wall_time_spec.rb | 2 +- .../profiling/collectors/stack_spec.rb | 3 ++- spec/datadog/profiling/stack_recorder_spec.rb | 24 ++++++++++--------- 7 files changed, 37 insertions(+), 31 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time.c b/ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time.c index 2f95f1fecf6..26debd3d2fc 100644 --- a/ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time.c +++ b/ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time.c @@ -124,12 +124,9 @@ struct per_thread_context { // Used to correlate profiles with traces struct trace_identifiers { - #define MAXIMUM_LENGTH_64_BIT_IDENTIFIER 21 // Why 21? 2^64 => 20 digits + 1 for \0 - bool valid; - ddog_CharSlice local_root_span_id; + uint64_t local_root_span_id; uint64_t span_id; - char local_root_span_id_buffer[MAXIMUM_LENGTH_64_BIT_IDENTIFIER]; VALUE trace_endpoint; }; @@ -576,7 +573,7 @@ static void trigger_sample_for_thread( trace_identifiers_for(state, thread, &trace_identifiers_result); if (trace_identifiers_result.valid) { - labels[label_pos++] = (ddog_prof_Label) {.key = DDOG_CHARSLICE_C("local root span id"), .str = trace_identifiers_result.local_root_span_id}; + labels[label_pos++] = (ddog_prof_Label) {.key = DDOG_CHARSLICE_C("local root span id"), .num = trace_identifiers_result.local_root_span_id}; labels[label_pos++] = (ddog_prof_Label) {.key = DDOG_CHARSLICE_C("span id"), .num = trace_identifiers_result.span_id}; if (trace_identifiers_result.trace_endpoint != Qnil) { @@ -851,13 +848,7 @@ static void trace_identifiers_for(struct cpu_and_wall_time_collector_state *stat VALUE numeric_span_id = rb_ivar_get(active_span, at_id_id /* @id */); if (numeric_local_root_span_id == Qnil || numeric_span_id == Qnil) return; - unsigned long long local_root_span_id = NUM2ULL(numeric_local_root_span_id); - snprintf(trace_identifiers_result->local_root_span_id_buffer, MAXIMUM_LENGTH_64_BIT_IDENTIFIER, "%llu", local_root_span_id); - - trace_identifiers_result->local_root_span_id = (ddog_CharSlice) { - .ptr = trace_identifiers_result->local_root_span_id_buffer, - .len = strlen(trace_identifiers_result->local_root_span_id_buffer) - }; + trace_identifiers_result->local_root_span_id = NUM2ULL(numeric_local_root_span_id); trace_identifiers_result->span_id = NUM2ULL(numeric_span_id); trace_identifiers_result->valid = true; diff --git a/ext/ddtrace_profiling_native_extension/collectors_stack.c b/ext/ddtrace_profiling_native_extension/collectors_stack.c index 505442d6fc5..7bf9f0f7726 100644 --- a/ext/ddtrace_profiling_native_extension/collectors_stack.c +++ b/ext/ddtrace_profiling_native_extension/collectors_stack.c @@ -32,6 +32,7 @@ static VALUE _native_sample( VALUE recorder_instance, VALUE metric_values_hash, VALUE labels_array, + VALUE numeric_labels_array, VALUE max_frames, VALUE in_gc ); @@ -60,7 +61,7 @@ void collectors_stack_init(VALUE profiling_module) { // Hosts methods used for testing the native code using RSpec VALUE testing_module = rb_define_module_under(collectors_stack_class, "Testing"); - rb_define_singleton_method(testing_module, "_native_sample", _native_sample, 6); + rb_define_singleton_method(testing_module, "_native_sample", _native_sample, 7); missing_string = rb_str_new2(""); rb_global_variable(&missing_string); @@ -74,11 +75,13 @@ static VALUE _native_sample( VALUE recorder_instance, VALUE metric_values_hash, VALUE labels_array, + VALUE numeric_labels_array, VALUE max_frames, VALUE in_gc ) { ENFORCE_TYPE(metric_values_hash, T_HASH); ENFORCE_TYPE(labels_array, T_ARRAY); + ENFORCE_TYPE(numeric_labels_array, T_ARRAY); if (RHASH_SIZE(metric_values_hash) != ENABLED_VALUE_TYPES_COUNT) { rb_raise( @@ -95,10 +98,10 @@ static VALUE _native_sample( metric_values[i] = NUM2LONG(metric_value); } - long labels_count = RARRAY_LEN(labels_array); + long labels_count = RARRAY_LEN(labels_array) + RARRAY_LEN(numeric_labels_array); ddog_prof_Label labels[labels_count]; - for (int i = 0; i < labels_count; i++) { + for (int i = 0; i < RARRAY_LEN(labels_array); i++) { VALUE key_str_pair = rb_ary_entry(labels_array, i); labels[i] = (ddog_prof_Label) { @@ -106,6 +109,14 @@ static VALUE _native_sample( .str = char_slice_from_ruby_string(rb_ary_entry(key_str_pair, 1)) }; } + for (int i = 0; i < RARRAY_LEN(numeric_labels_array); i++) { + VALUE key_str_pair = rb_ary_entry(numeric_labels_array, i); + + labels[i + RARRAY_LEN(labels_array)] = (ddog_prof_Label) { + .key = char_slice_from_ruby_string(rb_ary_entry(key_str_pair, 0)), + .num = NUM2ULL(rb_ary_entry(key_str_pair, 1)) + }; + } int max_frames_requested = NUM2INT(max_frames); if (max_frames_requested < 0) rb_raise(rb_eArgError, "Invalid max_frames: value must not be negative"); diff --git a/ext/ddtrace_profiling_native_extension/stack_recorder.c b/ext/ddtrace_profiling_native_extension/stack_recorder.c index 4e9adbabebc..a9271b96677 100644 --- a/ext/ddtrace_profiling_native_extension/stack_recorder.c +++ b/ext/ddtrace_profiling_native_extension/stack_recorder.c @@ -322,7 +322,7 @@ void record_sample(VALUE recorder_instance, ddog_prof_Sample sample) { sampler_unlock_active_profile(active_slot); } -void record_endpoint(VALUE recorder_instance, ddog_CharSlice local_root_span_id, ddog_CharSlice endpoint) { +void record_endpoint(VALUE recorder_instance, uint64_t local_root_span_id, ddog_CharSlice endpoint) { struct stack_recorder_state *state; TypedData_Get_Struct(recorder_instance, struct stack_recorder_state, &stack_recorder_typed_data, state); @@ -475,6 +475,7 @@ static void serializer_set_start_timestamp_for_next_profile(struct stack_recorde } static VALUE _native_record_endpoint(DDTRACE_UNUSED VALUE _self, VALUE recorder_instance, VALUE local_root_span_id, VALUE endpoint) { - record_endpoint(recorder_instance, char_slice_from_ruby_string(local_root_span_id), char_slice_from_ruby_string(endpoint)); + ENFORCE_TYPE(local_root_span_id, T_FIXNUM); + record_endpoint(recorder_instance, NUM2ULL(local_root_span_id), char_slice_from_ruby_string(endpoint)); return Qtrue; } diff --git a/ext/ddtrace_profiling_native_extension/stack_recorder.h b/ext/ddtrace_profiling_native_extension/stack_recorder.h index 6b18ae70b7d..b514afea7cf 100644 --- a/ext/ddtrace_profiling_native_extension/stack_recorder.h +++ b/ext/ddtrace_profiling_native_extension/stack_recorder.h @@ -35,5 +35,5 @@ static const ddog_prof_ValueType enabled_value_types[] = { #define ENABLED_VALUE_TYPES_COUNT (sizeof(enabled_value_types) / sizeof(ddog_prof_ValueType)) void record_sample(VALUE recorder_instance, ddog_prof_Sample sample); -void record_endpoint(VALUE recorder_instance, ddog_CharSlice local_root_span_id, ddog_CharSlice endpoint); +void record_endpoint(VALUE recorder_instance, uint64_t local_root_span_id, ddog_CharSlice endpoint); VALUE enforce_recorder_instance(VALUE object); diff --git a/spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb b/spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb index 4c489647dab..2211fb0c59b 100644 --- a/spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb +++ b/spec/datadog/profiling/collectors/cpu_and_wall_time_spec.rb @@ -348,7 +348,7 @@ def stats sample expect(t1_sample.fetch(:labels)).to include( - :'local root span id' => @t1_local_root_span_id.to_s, + :'local root span id' => @t1_local_root_span_id.to_i, :'span id' => @t1_span_id.to_i, ) end diff --git a/spec/datadog/profiling/collectors/stack_spec.rb b/spec/datadog/profiling/collectors/stack_spec.rb index aed64a1b0cc..024ae7128ab 100644 --- a/spec/datadog/profiling/collectors/stack_spec.rb +++ b/spec/datadog/profiling/collectors/stack_spec.rb @@ -20,7 +20,8 @@ let(:gathered_stack) { stacks.fetch(:gathered) } def sample(thread, recorder_instance, metric_values_hash, labels_array, max_frames: 400, in_gc: false) - described_class::Testing._native_sample(thread, recorder_instance, metric_values_hash, labels_array, max_frames, in_gc) + numeric_labels_array = [] + described_class::Testing._native_sample(thread, recorder_instance, metric_values_hash, labels_array, numeric_labels_array, max_frames, in_gc) end # This spec explicitly tests the main thread because an unpatched rb_profile_frames returns one more frame in the diff --git a/spec/datadog/profiling/stack_recorder_spec.rb b/spec/datadog/profiling/stack_recorder_spec.rb index 131840f5c4d..4caecfc0212 100644 --- a/spec/datadog/profiling/stack_recorder_spec.rb +++ b/spec/datadog/profiling/stack_recorder_spec.rb @@ -6,6 +6,8 @@ RSpec.describe Datadog::Profiling::StackRecorder do before { skip_if_profiling_not_supported(self) } + let(:numeric_labels) { [] } + subject(:stack_recorder) { described_class.new } # NOTE: A lot of libdatadog integration behaviors are tested in the Collectors::Stack specs, since we need actual @@ -144,7 +146,7 @@ def sample_types_from(decoded_profile) before do Datadog::Profiling::Collectors::Stack::Testing - ._native_sample(Thread.current, stack_recorder, metric_values, labels, 400, false) + ._native_sample(Thread.current, stack_recorder, metric_values, labels, numeric_labels, 400, false) expect(samples.size).to be 1 end @@ -175,23 +177,23 @@ def sample_types_from(decoded_profile) end describe 'trace endpoint behavior' do - let(:metric_values) { { 'cpu-time' => 123, 'cpu-samples' => 1, 'wall-time' => 789 } } + let(:metric_values) { { 'cpu-time' => 101, 'cpu-samples' => 1, 'wall-time' => 789 } } let(:samples) { samples_from_pprof(encoded_pprof) } it 'includes the endpoint for all matching samples taken before and after recording the endpoint' do - local_root_span_id_with_endpoint = { 'local root span id' => '123' } - local_root_span_id_without_endpoint = { 'local root span id' => '456' } + local_root_span_id_with_endpoint = { 'local root span id' => 123 } + local_root_span_id_without_endpoint = { 'local root span id' => 456 } - sample = proc do |labels = {}| + sample = proc do |numeric_labels = {}| Datadog::Profiling::Collectors::Stack::Testing - ._native_sample(Thread.current, stack_recorder, metric_values, labels.to_a, 400, false) + ._native_sample(Thread.current, stack_recorder, metric_values, [], numeric_labels.to_a, 400, false) end sample.call sample.call(local_root_span_id_without_endpoint) sample.call(local_root_span_id_with_endpoint) - described_class::Testing._native_record_endpoint(stack_recorder, '123', 'recorded-endpoint') + described_class::Testing._native_record_endpoint(stack_recorder, 123, 'recorded-endpoint') sample.call sample.call(local_root_span_id_without_endpoint) @@ -201,12 +203,12 @@ def sample_types_from(decoded_profile) # Other samples have not been changed expect(samples.select { |it| it[:labels].empty? }).to have(2).items - expect(samples.select { |it| it[:labels] == { :'local root span id' => '456' } }).to have(2).items + expect(samples.select { |it| it[:labels] == { :'local root span id' => 456 } }).to have(2).items # Matching samples taken before and after recording the endpoint have been changed expect( samples.select do |it| - it[:labels] == { :'local root span id' => '123', :'trace endpoint' => 'recorded-endpoint' } + it[:labels] == { :'local root span id' => 123, :'trace endpoint' => 'recorded-endpoint' } end ).to have(2).items end @@ -314,7 +316,7 @@ def sample_types_from(decoded_profile) it 'makes the next calls to serialize return no data' do # Add some data Datadog::Profiling::Collectors::Stack::Testing - ._native_sample(Thread.current, stack_recorder, metric_values, labels, 400, false) + ._native_sample(Thread.current, stack_recorder, metric_values, labels, numeric_labels, 400, false) # Sanity check: validate that data is there, to avoid the test passing because of other issues sanity_check_samples = samples_from_pprof(stack_recorder.serialize.last) @@ -322,7 +324,7 @@ def sample_types_from(decoded_profile) # Add some data, again Datadog::Profiling::Collectors::Stack::Testing - ._native_sample(Thread.current, stack_recorder, metric_values, labels, 400, false) + ._native_sample(Thread.current, stack_recorder, metric_values, labels, numeric_labels, 400, false) reset_after_fork From 0a7f10af7c50351b5b1fb693663d57f8533cac94 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Fri, 13 Jan 2023 18:49:29 +0000 Subject: [PATCH 2/5] Raise ArgumentError when ddog_prof_Profile_add returns a failure This will allow us to have early warning for when we're using the API incorrectly. See also https://github.com/DataDog/libdatadog/pull/87 . --- .../stack_recorder.c | 4 +++- spec/datadog/profiling/stack_recorder_spec.rb | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/ext/ddtrace_profiling_native_extension/stack_recorder.c b/ext/ddtrace_profiling_native_extension/stack_recorder.c index a9271b96677..ee2a1a378ba 100644 --- a/ext/ddtrace_profiling_native_extension/stack_recorder.c +++ b/ext/ddtrace_profiling_native_extension/stack_recorder.c @@ -317,9 +317,11 @@ void record_sample(VALUE recorder_instance, ddog_prof_Sample sample) { struct active_slot_pair active_slot = sampler_lock_active_profile(state); - ddog_prof_Profile_add(active_slot.profile, sample); + uint64_t success = ddog_prof_Profile_add(active_slot.profile, sample); sampler_unlock_active_profile(active_slot); + + if (!success) rb_raise(rb_eArgError, "Failed to record sample: ddog_prof_Profile_add returned failure status code"); } void record_endpoint(VALUE recorder_instance, uint64_t local_root_span_id, ddog_CharSlice endpoint) { diff --git a/spec/datadog/profiling/stack_recorder_spec.rb b/spec/datadog/profiling/stack_recorder_spec.rb index 4caecfc0212..a7db92da3ba 100644 --- a/spec/datadog/profiling/stack_recorder_spec.rb +++ b/spec/datadog/profiling/stack_recorder_spec.rb @@ -176,6 +176,21 @@ def sample_types_from(decoded_profile) end end + context 'when sample is invalid' do + context 'because the local root span id is being defined using a string instead of as a number' do + let(:metric_values) { { 'cpu-time' => 123, 'cpu-samples' => 456, 'wall-time' => 789 } } + + it do + # We're using `_native_sample` here to test the behavior of `record_sample` in `stack_recorder.c` + expect do + Datadog::Profiling::Collectors::Stack::Testing._native_sample( + Thread.current, stack_recorder, metric_values, { 'local root span id' => 'incorrect' }.to_a, [], 400, false + ) + end.to raise_error(ArgumentError) + end + end + end + describe 'trace endpoint behavior' do let(:metric_values) { { 'cpu-time' => 101, 'cpu-samples' => 1, 'wall-time' => 789 } } let(:samples) { samples_from_pprof(encoded_pprof) } From 9a42eafccad1259b17cda63338a1115498d16fb6 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Thu, 2 Feb 2023 12:13:26 +0000 Subject: [PATCH 3/5] Adopt to new error reporting API, as well as to new drop methods One of the drop methods had to be commented out due to a libdatadog bug, I'll re-enable it once libdatadog gets fixed. --- .../http_transport.c | 57 ++++++++----------- .../libdatadog_helpers.h | 11 +++- .../stack_recorder.c | 16 +++--- 3 files changed, 42 insertions(+), 42 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/http_transport.c b/ext/ddtrace_profiling_native_extension/http_transport.c index d99d0e96126..fd056c17c9f 100644 --- a/ext/ddtrace_profiling_native_extension/http_transport.c +++ b/ext/ddtrace_profiling_native_extension/http_transport.c @@ -21,7 +21,7 @@ static VALUE library_version_string = Qnil; struct call_exporter_without_gvl_arguments { ddog_prof_Exporter *exporter; - ddog_prof_Exporter_Request *request; + ddog_prof_Exporter_Request_BuildResult *build_result; ddog_CancellationToken *cancel_token; ddog_prof_Exporter_SendResult result; bool send_ran; @@ -83,7 +83,7 @@ static VALUE _native_validate_exporter(DDTRACE_UNUSED VALUE _self, VALUE exporte // We don't actually need the exporter for now -- we just wanted to validate that we could create it with the // settings we were given - ddog_prof_Exporter_NewResult_drop(exporter_result); + ddog_prof_Exporter_drop(exporter_result.ok); return rb_ary_new_from_args(2, ok_symbol, Qnil); } @@ -111,13 +111,9 @@ static ddog_prof_Exporter_NewResult create_exporter(VALUE exporter_configuration } static VALUE handle_exporter_failure(ddog_prof_Exporter_NewResult exporter_result) { - if (exporter_result.tag == DDOG_PROF_EXPORTER_NEW_RESULT_OK) return Qnil; - - VALUE err_details = ruby_string_from_prof_vec_u8(exporter_result.err); - - ddog_prof_Exporter_NewResult_drop(exporter_result); - - return rb_ary_new_from_args(2, error_symbol, err_details); + return exporter_result.tag == DDOG_PROF_EXPORTER_NEW_RESULT_OK ? + Qnil : + rb_ary_new_from_args(2, error_symbol, get_error_details_and_drop(&exporter_result.err)); } static ddog_Endpoint endpoint_from(VALUE exporter_configuration) { @@ -173,14 +169,9 @@ static ddog_Vec_Tag convert_tags(VALUE tags_as_array) { ddog_Vec_Tag_push(&tags, char_slice_from_ruby_string(tag_name), char_slice_from_ruby_string(tag_value)); if (push_result.tag == DDOG_VEC_TAG_PUSH_RESULT_ERR) { - VALUE err_details = ruby_string_from_vec_u8(push_result.err); - ddog_Vec_Tag_PushResult_drop(push_result); - // libdatadog validates tags and may catch invalid tags that ddtrace didn't actually catch. // We warn users about such tags, and then just ignore them. - safely_log_failure_to_process_tag(tags, err_details); - } else { - ddog_Vec_Tag_PushResult_drop(push_result); + safely_log_failure_to_process_tag(tags, get_error_details_and_drop(&push_result.err)); } } @@ -206,23 +197,28 @@ static void safely_log_failure_to_process_tag(ddog_Vec_Tag tags, VALUE err_detai // Note: This function handles a bunch of libdatadog dynamically-allocated objects, so it MUST not use any Ruby APIs // which can raise exceptions, otherwise the objects will be leaked. static VALUE perform_export( - ddog_prof_Exporter_NewResult valid_exporter_result, // Must be called with a valid exporter result + ddog_prof_Exporter *exporter, ddog_Timespec start, ddog_Timespec finish, ddog_prof_Exporter_Slice_File slice_files, ddog_Vec_Tag *additional_tags, uint64_t timeout_milliseconds ) { - ddog_prof_Exporter *exporter = valid_exporter_result.ok; - ddog_CancellationToken *cancel_token = ddog_CancellationToken_new(); ddog_prof_ProfiledEndpointsStats *endpoints_stats = NULL; // Not in use yet - ddog_prof_Exporter_Request *request = + ddog_prof_Exporter_Request_BuildResult build_result = ddog_prof_Exporter_Request_build(exporter, start, finish, slice_files, additional_tags, endpoints_stats, timeout_milliseconds); + if (build_result.tag == DDOG_PROF_EXPORTER_REQUEST_BUILD_RESULT_ERR) { + ddog_prof_Exporter_drop(exporter); + return rb_ary_new_from_args(2, error_symbol, get_error_details_and_drop(&build_result.err)); + } + + ddog_CancellationToken *cancel_token = ddog_CancellationToken_new(); + // We'll release the Global VM Lock while we're calling send, so that the Ruby VM can continue to work while this // is pending struct call_exporter_without_gvl_arguments args = - {.exporter = exporter, .request = request, .cancel_token = cancel_token, .send_ran = false}; + {.exporter = exporter, .build_result = &build_result, .cancel_token = cancel_token, .send_ran = false}; // We use rb_thread_call_without_gvl2 instead of rb_thread_call_without_gvl as the gvl2 variant never raises any // exceptions. @@ -247,26 +243,23 @@ static VALUE perform_export( // Cleanup exporter and token, no longer needed ddog_CancellationToken_drop(cancel_token); - ddog_prof_Exporter_NewResult_drop(valid_exporter_result); + ddog_prof_Exporter_drop(exporter); if (pending_exception) { // If we got here send did not run, so we need to explicitly dispose of the request - ddog_prof_Exporter_Request_drop(request); + ddog_prof_Exporter_Request_drop(&build_result.ok); // Let Ruby propagate the exception. This will not return. rb_jump_tag(pending_exception); } - ddog_prof_Exporter_SendResult result = args.result; - bool success = result.tag == DDOG_PROF_EXPORTER_SEND_RESULT_HTTP_RESPONSE; - - VALUE ruby_status = success ? ok_symbol : error_symbol; - VALUE ruby_result = success ? UINT2NUM(result.http_response.code) : ruby_string_from_prof_vec_u8(result.err); + // The request itself does not need to be freed as libdatadog takes ownership of it as part of sending. - ddog_prof_Exporter_SendResult_drop(args.result); - // The request itself does not need to be freed as libdatadog takes care of it as part of sending. + ddog_prof_Exporter_SendResult result = args.result; - return rb_ary_new_from_args(2, ruby_status, ruby_result); + return result.tag == DDOG_PROF_EXPORTER_SEND_RESULT_HTTP_RESPONSE ? + rb_ary_new_from_args(2, ok_symbol, UINT2NUM(result.http_response.code)) : + rb_ary_new_from_args(2, error_symbol, get_error_details_and_drop(&result.err)); } static VALUE _native_do_export( @@ -326,13 +319,13 @@ static VALUE _native_do_export( VALUE failure_tuple = handle_exporter_failure(exporter_result); if (!NIL_P(failure_tuple)) return failure_tuple; - return perform_export(exporter_result, start, finish, slice_files, null_additional_tags, timeout_milliseconds); + return perform_export(exporter_result.ok, start, finish, slice_files, null_additional_tags, timeout_milliseconds); } static void *call_exporter_without_gvl(void *call_args) { struct call_exporter_without_gvl_arguments *args = (struct call_exporter_without_gvl_arguments*) call_args; - args->result = ddog_prof_Exporter_send(args->exporter, args->request, args->cancel_token); + args->result = ddog_prof_Exporter_send(args->exporter, &args->build_result->ok, args->cancel_token); args->send_ran = true; return NULL; // Unused diff --git a/ext/ddtrace_profiling_native_extension/libdatadog_helpers.h b/ext/ddtrace_profiling_native_extension/libdatadog_helpers.h index 9044631fcef..23874282250 100644 --- a/ext/ddtrace_profiling_native_extension/libdatadog_helpers.h +++ b/ext/ddtrace_profiling_native_extension/libdatadog_helpers.h @@ -13,6 +13,13 @@ inline static VALUE ruby_string_from_vec_u8(ddog_Vec_U8 string) { return rb_str_new((char *) string.ptr, string.len); } -inline static VALUE ruby_string_from_prof_vec_u8(ddog_prof_Vec_U8 string) { - return rb_str_new((char *) string.ptr, string.len); +inline static VALUE ruby_string_from_error(const ddog_Error *error) { + ddog_CharSlice char_slice = ddog_Error_message(error); + return rb_str_new(char_slice.ptr, char_slice.len); +} + +inline static VALUE get_error_details_and_drop(ddog_Error *error) { + VALUE result = ruby_string_from_error(error); + ddog_Error_drop(error); + return result; } diff --git a/ext/ddtrace_profiling_native_extension/stack_recorder.c b/ext/ddtrace_profiling_native_extension/stack_recorder.c index ee2a1a378ba..aca12e416ff 100644 --- a/ext/ddtrace_profiling_native_extension/stack_recorder.c +++ b/ext/ddtrace_profiling_native_extension/stack_recorder.c @@ -282,18 +282,16 @@ static VALUE _native_serialize(DDTRACE_UNUSED VALUE _self, VALUE recorder_instan ddog_prof_Profile_SerializeResult serialized_profile = args.result; if (serialized_profile.tag == DDOG_PROF_PROFILE_SERIALIZE_RESULT_ERR) { - VALUE err_details = ruby_string_from_prof_vec_u8(serialized_profile.err); - ddog_prof_Profile_SerializeResult_drop(serialized_profile); - return rb_ary_new_from_args(2, error_symbol, err_details); + return rb_ary_new_from_args(2, error_symbol, get_error_details_and_drop(&serialized_profile.err)); } - VALUE encoded_pprof = ruby_string_from_prof_vec_u8(serialized_profile.ok.buffer); + VALUE encoded_pprof = ruby_string_from_vec_u8(serialized_profile.ok.buffer); ddog_Timespec ddprof_start = serialized_profile.ok.start; ddog_Timespec ddprof_finish = serialized_profile.ok.end; - // Clean up libdatadog object to avoid leaking in case ruby_time_from raises an exception - ddog_prof_Profile_SerializeResult_drop(serialized_profile); + // FIXME: Temporarily disabled until a libdatadog crash gets fixed + // ddog_prof_EncodedProfile_drop(&serialized_profile.ok); VALUE start = ruby_time_from(ddprof_start); VALUE finish = ruby_time_from(ddprof_finish); @@ -317,11 +315,13 @@ void record_sample(VALUE recorder_instance, ddog_prof_Sample sample) { struct active_slot_pair active_slot = sampler_lock_active_profile(state); - uint64_t success = ddog_prof_Profile_add(active_slot.profile, sample); + ddog_prof_Profile_AddResult result = ddog_prof_Profile_add(active_slot.profile, sample); sampler_unlock_active_profile(active_slot); - if (!success) rb_raise(rb_eArgError, "Failed to record sample: ddog_prof_Profile_add returned failure status code"); + if (result.tag == DDOG_PROF_PROFILE_ADD_RESULT_ERR) { + rb_raise(rb_eArgError, "Failed to record sample: %"PRIsVALUE, get_error_details_and_drop(&result.err)); + } } void record_endpoint(VALUE recorder_instance, uint64_t local_root_span_id, ddog_CharSlice endpoint) { From 2a52553ee9c799b1c4aa36d55c5fce5881c063c1 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Thu, 2 Feb 2023 17:01:13 +0000 Subject: [PATCH 4/5] Remove FIXME now that libdatadog has been fixed --- ext/ddtrace_profiling_native_extension/stack_recorder.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/stack_recorder.c b/ext/ddtrace_profiling_native_extension/stack_recorder.c index aca12e416ff..3a0e2a765b4 100644 --- a/ext/ddtrace_profiling_native_extension/stack_recorder.c +++ b/ext/ddtrace_profiling_native_extension/stack_recorder.c @@ -290,8 +290,7 @@ static VALUE _native_serialize(DDTRACE_UNUSED VALUE _self, VALUE recorder_instan ddog_Timespec ddprof_start = serialized_profile.ok.start; ddog_Timespec ddprof_finish = serialized_profile.ok.end; - // FIXME: Temporarily disabled until a libdatadog crash gets fixed - // ddog_prof_EncodedProfile_drop(&serialized_profile.ok); + ddog_prof_EncodedProfile_drop(&serialized_profile.ok); VALUE start = ruby_time_from(ddprof_start); VALUE finish = ruby_time_from(ddprof_finish); From f4abd1d8bc61fa2df9feb301f3ecc9ff5afb147b Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Fri, 3 Feb 2023 14:50:47 +0000 Subject: [PATCH 5/5] Bump dependency to pull in libdatadog 2.0 --- ddtrace.gemspec | 2 +- .../native_extension_helpers.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ddtrace.gemspec b/ddtrace.gemspec index c61bb3f5ec2..18bd0b8e0b7 100644 --- a/ddtrace.gemspec +++ b/ddtrace.gemspec @@ -69,7 +69,7 @@ Gem::Specification.new do |spec| # Used by profiling (and possibly others in the future) # When updating the version here, please also update the version in `native_extension_helpers.rb` (and yes we have a test for it) - spec.add_dependency 'libdatadog', '~> 1.0.1.1.0' + spec.add_dependency 'libdatadog', '~> 2.0.0.1.0' spec.extensions = ['ext/ddtrace_profiling_native_extension/extconf.rb', 'ext/ddtrace_profiling_loader/extconf.rb'] end diff --git a/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb b/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb index b699cee1bb8..e1d47b1198b 100644 --- a/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb +++ b/ext/ddtrace_profiling_native_extension/native_extension_helpers.rb @@ -17,7 +17,7 @@ module NativeExtensionHelpers # Older Rubies don't have the MJIT header, used by the JIT compiler, so we need to use a different approach CAN_USE_MJIT_HEADER = RUBY_VERSION >= '2.6' - LIBDATADOG_VERSION = '~> 1.0.1.1.0' + LIBDATADOG_VERSION = '~> 2.0.0.1.0' def self.fail_install_if_missing_extension? ENV[ENV_FAIL_INSTALL_IF_MISSING_EXTENSION].to_s.strip.downcase == 'true'