Merge pull request #3169 from DataDog/ivoanjo/prof-8289-libdatadog5-u…

…pgrade

[PROF-8289] Upgrade to libdatadog 5

ddtrace.gemspec

@@ -66,7 +66,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', '~> 4.0.0.1.0'
+  spec.add_dependency 'libdatadog', '~> 5.0.0.1.0'
 
   # used for CI visibility product until the next major version
   spec.add_dependency 'datadog-ci', '~> 0.1.0'

ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time_worker.c

@@ -260,6 +260,9 @@ static const rb_data_type_t cpu_and_wall_time_worker_typed_data = {
 static VALUE _native_new(VALUE klass) {
   struct cpu_and_wall_time_worker_state *state = ruby_xcalloc(1, sizeof(struct cpu_and_wall_time_worker_state));
 
+  // Note: Any exceptions raised from this note until the TypedData_Wrap_Struct call will lead to the state memory
+  // being leaked.
+
   state->gc_profiling_enabled = false;
   state->allocation_counting_enabled = false;
   state->no_signals_workaround_enabled = false;

ext/ddtrace_profiling_native_extension/collectors_idle_sampling_helper.c

@@ -78,6 +78,9 @@ static const rb_data_type_t idle_sampling_helper_typed_data = {
 static VALUE _native_new(VALUE klass) {
   struct idle_sampling_loop_state *state = ruby_xcalloc(1, sizeof(struct idle_sampling_loop_state));
 
+  // Note: Any exceptions raised from this note until the TypedData_Wrap_Struct call will lead to the state memory
+  // being leaked.
+
   reset_state(state);
 
   return TypedData_Wrap_Struct(klass, &idle_sampling_helper_typed_data, state);

ext/ddtrace_profiling_native_extension/collectors_thread_context.c

@@ -304,6 +304,9 @@ static int hash_map_per_thread_context_free_values(DDTRACE_UNUSED st_data_t _thr
 static VALUE _native_new(VALUE klass) {
   struct thread_context_collector_state *state = ruby_xcalloc(1, sizeof(struct thread_context_collector_state));
 
+  // Note: Any exceptions raised from this note until the TypedData_Wrap_Struct call will lead to the state memory
+  // being leaked.
+
   // Update this when modifying state struct
   state->sampling_buffer = NULL;
   state->hash_map_per_thread_context =
@@ -660,7 +663,6 @@ static void trigger_sample_for_thread(
     1 + // thread id
     1 + // thread name
     1 + // profiler overhead
-    1 + // end_timestamp_ns
     2 + // ruby vm type and allocation class
     1 + // state (only set for cpu/wall-time samples)
     2;  // local root span id and span id
@@ -725,13 +727,6 @@ static void trigger_sample_for_thread(
     };
   }
 
-  if (state->timeline_enabled && current_monotonic_wall_time_ns != INVALID_TIME) {
-    labels[label_pos++] = (ddog_prof_Label) {
-      .key = DDOG_CHARSLICE_C("end_timestamp_ns"),
-      .num = monotonic_to_system_epoch_ns(&state->time_converter_state, current_monotonic_wall_time_ns)
-    };
-  }
-
   if (ruby_vm_type != NULL) {
     labels[label_pos++] = (ddog_prof_Label) {
       .key = DDOG_CHARSLICE_C("ruby vm type"),
@@ -770,12 +765,18 @@ static void trigger_sample_for_thread(
 
   ddog_prof_Slice_Label slice_labels = {.ptr = labels, .len = label_pos};
 
+  // The end_timestamp_ns is treated specially by libdatadog and that's why it's not added as a ddog_prof_Label
+  int64_t end_timestamp_ns = 0;
+  if (state->timeline_enabled && current_monotonic_wall_time_ns != INVALID_TIME) {
+    end_timestamp_ns = monotonic_to_system_epoch_ns(&state->time_converter_state, current_monotonic_wall_time_ns);
+  }
+
   sample_thread(
     stack_from_thread,
     state->sampling_buffer,
     state->recorder_instance,
     values,
-    (sample_labels) {.labels = slice_labels, .state_label = state_label},
+    (sample_labels) {.labels = slice_labels, .state_label = state_label, .end_timestamp_ns = end_timestamp_ns},
     type
   );
 }

ext/ddtrace_profiling_native_extension/http_transport.c

@@ -201,7 +201,8 @@ static VALUE perform_export(
   ddog_prof_Exporter *exporter,
   ddog_Timespec start,
   ddog_Timespec finish,
-  ddog_prof_Exporter_Slice_File slice_files,
+  ddog_prof_Exporter_Slice_File files_to_compress_and_export,
+  ddog_prof_Exporter_Slice_File files_to_export_unmodified,
   ddog_Vec_Tag *additional_tags,
   ddog_CharSlice internal_metadata,
   uint64_t timeout_milliseconds
@@ -211,7 +212,8 @@ static VALUE perform_export(
     exporter,
     start,
     finish,
-    slice_files,
+    files_to_compress_and_export,
+    files_to_export_unmodified,
     additional_tags,
     endpoints_stats,
     &internal_metadata,
@@ -308,18 +310,23 @@ static VALUE _native_do_export(
   ddog_Timespec finish =
     {.seconds = NUM2LONG(finish_timespec_seconds), .nanoseconds = NUM2UINT(finish_timespec_nanoseconds)};
 
-  int files_to_report = 1 + (have_code_provenance ? 1 : 0);
-  ddog_prof_Exporter_File files[files_to_report];
-  ddog_prof_Exporter_Slice_File slice_files = {.ptr = files, .len = files_to_report};
+  int to_compress_length = have_code_provenance ? 1 : 0;
+  ddog_prof_Exporter_File to_compress[to_compress_length];
+  int already_compressed_length = 1; // pprof
+  ddog_prof_Exporter_File already_compressed[already_compressed_length];
 
-  files[0] = (ddog_prof_Exporter_File) {
+  ddog_prof_Exporter_Slice_File files_to_compress_and_export = {.ptr = to_compress, .len = to_compress_length};
+  ddog_prof_Exporter_Slice_File files_to_export_unmodified = {.ptr = already_compressed, .len = already_compressed_length};
+
+  already_compressed[0] = (ddog_prof_Exporter_File) {
     .name = char_slice_from_ruby_string(pprof_file_name),
-    .file = byte_slice_from_ruby_string(pprof_data)
+    .file = byte_slice_from_ruby_string(pprof_data),
   };
+
   if (have_code_provenance) {
-    files[1] = (ddog_prof_Exporter_File) {
+    to_compress[0] = (ddog_prof_Exporter_File) {
       .name = char_slice_from_ruby_string(code_provenance_file_name),
-      .file = byte_slice_from_ruby_string(code_provenance_data)
+      .file = byte_slice_from_ruby_string(code_provenance_data),
     };
   }
 
@@ -332,7 +339,16 @@ 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.ok, start, finish, slice_files, null_additional_tags, internal_metadata, timeout_milliseconds);
+  return perform_export(
+    exporter_result.ok,
+    start,
+    finish,
+    files_to_compress_and_export,
+    files_to_export_unmodified,
+    null_additional_tags,
+    internal_metadata,
+    timeout_milliseconds
+  );
 }
 
 static void *call_exporter_without_gvl(void *call_args) {

ext/ddtrace_profiling_native_extension/native_extension_helpers.rb

@@ -15,7 +15,7 @@ module NativeExtensionHelpers
       # The MJIT header was introduced on 2.6 and removed on 3.3; for other Rubies we rely on debase-ruby_core_source
       CAN_USE_MJIT_HEADER = RUBY_VERSION.start_with?('2.6', '2.7', '3.0.', '3.1.', '3.2.')
 
-      LIBDATADOG_VERSION = '~> 4.0.0.1.0'
+      LIBDATADOG_VERSION = '~> 5.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

@@ -197,6 +197,7 @@ struct call_serialize_without_gvl_arguments {
 
 static VALUE _native_new(VALUE klass);
 static void initialize_slot_concurrency_control(struct stack_recorder_state *state);
+static void initialize_profiles(struct stack_recorder_state *state, ddog_prof_Slice_ValueType sample_types);
 static void stack_recorder_typed_data_free(void *data);
 static VALUE _native_initialize(DDTRACE_UNUSED VALUE _self, VALUE recorder_instance, VALUE cpu_time_enabled, VALUE alloc_samples_enabled);
 static VALUE _native_serialize(VALUE self, VALUE recorder_instance);
@@ -257,18 +258,25 @@ static const rb_data_type_t stack_recorder_typed_data = {
 static VALUE _native_new(VALUE klass) {
   struct stack_recorder_state *state = ruby_xcalloc(1, sizeof(struct stack_recorder_state));
 
+  // Note: Any exceptions raised from this note until the TypedData_Wrap_Struct call will lead to the state memory
+  // being leaked.
+
   ddog_prof_Slice_ValueType sample_types = {.ptr = all_value_types, .len = ALL_VALUE_TYPES_COUNT};
 
   initialize_slot_concurrency_control(state);
   for (uint8_t i = 0; i < ALL_VALUE_TYPES_COUNT; i++) { state->position_for[i] = all_value_types_positions[i]; }
   state->enabled_values_count = ALL_VALUE_TYPES_COUNT;
 
+  // Note: At this point, slot_one_profile and slot_two_profile contain null pointers. Libdatadog validates pointers
+  // before using them so it's ok for us to go ahead and create the StackRecorder object.
+
+  VALUE stack_recorder = TypedData_Wrap_Struct(klass, &stack_recorder_typed_data, state);
+
   // Note: Don't raise exceptions after this point, since it'll lead to libdatadog memory leaking!
 
-  state->slot_one_profile = ddog_prof_Profile_new(sample_types, NULL /* period is optional */, NULL /* start_time is optional */);
-  state->slot_two_profile = ddog_prof_Profile_new(sample_types, NULL /* period is optional */, NULL /* start_time is optional */);
+  initialize_profiles(state, sample_types);
 
-  return TypedData_Wrap_Struct(klass, &stack_recorder_typed_data, state);
+  return stack_recorder;
 }
 
 static void initialize_slot_concurrency_control(struct stack_recorder_state *state) {
@@ -281,6 +289,28 @@ static void initialize_slot_concurrency_control(struct stack_recorder_state *sta
   state->active_slot = 1;
 }
 
+static void initialize_profiles(struct stack_recorder_state *state, ddog_prof_Slice_ValueType sample_types) {
+  ddog_prof_Profile_NewResult slot_one_profile_result =
+    ddog_prof_Profile_new(sample_types, NULL /* period is optional */, NULL /* start_time is optional */);
+
+  if (slot_one_profile_result.tag == DDOG_PROF_PROFILE_NEW_RESULT_ERR) {
+    rb_raise(rb_eRuntimeError, "Failed to initialize slot one profile: %"PRIsVALUE, get_error_details_and_drop(&slot_one_profile_result.err));
+  }
+
+  ddog_prof_Profile_NewResult slot_two_profile_result =
+    ddog_prof_Profile_new(sample_types, NULL /* period is optional */, NULL /* start_time is optional */);
+
+  if (slot_two_profile_result.tag == DDOG_PROF_PROFILE_NEW_RESULT_ERR) {
+    // Uff! Though spot. We need to make sure to properly clean up the other profile as well first
+    ddog_prof_Profile_drop(&slot_one_profile_result.ok);
+    // And now we can raise...
+    rb_raise(rb_eRuntimeError, "Failed to initialize slot two profile: %"PRIsVALUE, get_error_details_and_drop(&slot_two_profile_result.err));
+  }
+
+  state->slot_one_profile = slot_one_profile_result.ok;
+  state->slot_two_profile = slot_two_profile_result.ok;
+}
+
 static void stack_recorder_typed_data_free(void *state_ptr) {
   struct stack_recorder_state *state = (struct stack_recorder_state *) state_ptr;
 
@@ -334,13 +364,11 @@ static VALUE _native_initialize(DDTRACE_UNUSED VALUE _self, VALUE recorder_insta
     state->position_for[ALLOC_SAMPLES_VALUE_ID] = next_disabled_pos++;
   }
 
-  ddog_prof_Slice_ValueType sample_types = {.ptr = enabled_value_types, .len = state->enabled_values_count};
-
   ddog_prof_Profile_drop(&state->slot_one_profile);
   ddog_prof_Profile_drop(&state->slot_two_profile);
 
-  state->slot_one_profile = ddog_prof_Profile_new(sample_types, NULL /* period is optional */, NULL /* start_time is optional */);
-  state->slot_two_profile = ddog_prof_Profile_new(sample_types, NULL /* period is optional */, NULL /* start_time is optional */);
+  ddog_prof_Slice_ValueType sample_types = {.ptr = enabled_value_types, .len = state->enabled_values_count};
+  initialize_profiles(state, sample_types);
 
   return Qtrue;
 }
@@ -387,9 +415,6 @@ static VALUE _native_serialize(DDTRACE_UNUSED VALUE _self, VALUE recorder_instan
   VALUE start = ruby_time_from(ddprof_start);
   VALUE finish = ruby_time_from(ddprof_finish);
 
-  // This will raise on error
-  reset_profile(args.profile, /* start_time: */ NULL);
-
   return rb_ary_new_from_args(2, ok_symbol, rb_ary_new_from_args(3, start, finish, encoded_pprof));
 }
 
@@ -423,7 +448,8 @@ void record_sample(VALUE recorder_instance, ddog_prof_Slice_Location locations,
       .locations = locations,
       .values = (ddog_Slice_I64) {.ptr = metric_values, .len = state->enabled_values_count},
       .labels = labels.labels
-    }
+    },
+    labels.end_timestamp_ns
   );
 
   sampler_unlock_active_profile(active_slot);
@@ -452,7 +478,8 @@ static void *call_serialize_without_gvl(void *call_args) {
   struct call_serialize_without_gvl_arguments *args = (struct call_serialize_without_gvl_arguments *) call_args;
 
   args->profile = serializer_flip_active_and_inactive_slots(args->state);
-  args->result = ddog_prof_Profile_serialize(args->profile, &args->finish_timestamp, NULL /* duration_nanos is optional */);
+  // Note: The profile gets reset by the serialize call
+  args->result = ddog_prof_Profile_serialize(args->profile, &args->finish_timestamp, NULL /* duration_nanos is optional */, NULL /* start_time is optional */);
   args->serialize_ran = true;
 
   return NULL; // Unused

ext/ddtrace_profiling_native_extension/stack_recorder.h

@@ -15,6 +15,8 @@ typedef struct sample_labels {
   // This is used to allow the `Collectors::Stack` to modify the existing label, if any. This MUST be NULL or point
   // somewhere inside the labels slice above.
   ddog_prof_Label *state_label;
+
+  int64_t end_timestamp_ns;
 } sample_labels;
 
 void record_sample(VALUE recorder_instance, ddog_prof_Slice_Location locations, sample_values values, sample_labels labels);