Merge pull request #2599 from DataDog/ivoanjo/wip-libdatadog-2

Upgrade profiler to use libdatadog v2.0.0

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

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;

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,17 +98,25 @@ 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) {
       .key = char_slice_from_ruby_string(rb_ary_entry(key_str_pair, 0)),
       .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");

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

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;
 }

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'

ext/ddtrace_profiling_native_extension/stack_recorder.c

@@ -282,18 +282,15 @@ 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);
+  ddog_prof_EncodedProfile_drop(&serialized_profile.ok);
 
   VALUE start = ruby_time_from(ddprof_start);
   VALUE finish = ruby_time_from(ddprof_finish);
@@ -317,12 +314,16 @@ 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);
+  ddog_prof_Profile_AddResult result = ddog_prof_Profile_add(active_slot.profile, sample);
 
   sampler_unlock_active_profile(active_slot);
+
+  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, 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 +476,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;
 }

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);

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

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