From 1856678787dc4cfa29c70a06f0fdc86406652dbc Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Thu, 10 Oct 2024 16:58:52 +0100 Subject: [PATCH 1/5] Minor: Cleanup enum to make it consistent with other enums --- .../collectors_thread_context.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ext/datadog_profiling_native_extension/collectors_thread_context.c b/ext/datadog_profiling_native_extension/collectors_thread_context.c index 52b2e8df04..ad1c489ad7 100644 --- a/ext/datadog_profiling_native_extension/collectors_thread_context.c +++ b/ext/datadog_profiling_native_extension/collectors_thread_context.c @@ -109,7 +109,7 @@ static ID otel_context_storage_id; // id of :__opentelemetry_context_storage__ i // and that'll be the one that last wrote this setting. static uint32_t global_waiting_for_gvl_threshold_ns = MILLIS_AS_NS(10); -enum otel_context_enabled {otel_context_enabled_false, otel_context_enabled_only, otel_context_enabled_both}; +typedef enum { OTEL_CONTEXT_ENABLED_FALSE, OTEL_CONTEXT_ENABLED_ONLY, OTEL_CONTEXT_ENABLED_BOTH } otel_context_enabled; // Contains state for a single ThreadContext instance struct thread_context_collector_state { @@ -138,7 +138,7 @@ struct thread_context_collector_state { // Used to omit timestamps / timeline events from collected data bool timeline_enabled; // Used to control context collection - enum otel_context_enabled otel_context_enabled; + otel_context_enabled otel_context_enabled; // Used to omit class information from collected allocation data bool allocation_type_enabled; // Used when calling monotonic_to_system_epoch_ns @@ -435,7 +435,7 @@ static VALUE _native_new(VALUE klass) { state->thread_list_buffer = thread_list_buffer; state->endpoint_collection_enabled = true; state->timeline_enabled = true; - state->otel_context_enabled = otel_context_enabled_false; + state->otel_context_enabled = OTEL_CONTEXT_ENABLED_FALSE; state->allocation_type_enabled = true; state->time_converter_state = (monotonic_to_system_epoch_state) MONOTONIC_TO_SYSTEM_EPOCH_INITIALIZER; VALUE main_thread = rb_thread_main(); @@ -485,11 +485,11 @@ static VALUE _native_initialize( state->endpoint_collection_enabled = (endpoint_collection_enabled == Qtrue); state->timeline_enabled = (timeline_enabled == Qtrue); if (otel_context_enabled == Qfalse || otel_context_enabled == Qnil) { - state->otel_context_enabled = otel_context_enabled_false; + state->otel_context_enabled = OTEL_CONTEXT_ENABLED_FALSE; } else if (otel_context_enabled == ID2SYM(rb_intern("only"))) { - state->otel_context_enabled = otel_context_enabled_only; + state->otel_context_enabled = OTEL_CONTEXT_ENABLED_ONLY; } else if (otel_context_enabled == ID2SYM(rb_intern("both"))) { - state->otel_context_enabled = otel_context_enabled_both; + state->otel_context_enabled = OTEL_CONTEXT_ENABLED_BOTH; } else { rb_raise(rb_eArgError, "Unexpected value for otel_context_enabled: %+" PRIsVALUE, otel_context_enabled); } @@ -885,7 +885,7 @@ static void trigger_sample_for_thread( struct trace_identifiers trace_identifiers_result = {.valid = false, .trace_endpoint = Qnil}; trace_identifiers_for(state, thread, &trace_identifiers_result); - if (!trace_identifiers_result.valid && state->otel_context_enabled != otel_context_enabled_false) { + if (!trace_identifiers_result.valid && state->otel_context_enabled != OTEL_CONTEXT_ENABLED_FALSE) { // If we couldn't get something with ddtrace, let's see if we can get some trace identifiers from opentelemetry directly otel_without_ddtrace_trace_identifiers_for(state, thread, &trace_identifiers_result); } @@ -1302,7 +1302,7 @@ static VALUE _native_gc_tracking(DDTRACE_UNUSED VALUE _self, VALUE collector_ins // Assumption 1: This function is called in a thread that is holding the Global VM Lock. Caller is responsible for enforcing this. static void trace_identifiers_for(struct thread_context_collector_state *state, VALUE thread, struct trace_identifiers *trace_identifiers_result) { - if (state->otel_context_enabled == otel_context_enabled_only) return; + if (state->otel_context_enabled == OTEL_CONTEXT_ENABLED_ONLY) return; if (state->tracer_context_key == MISSING_TRACER_CONTEXT_KEY) return; VALUE current_context = rb_thread_local_aref(thread, state->tracer_context_key); From 7e5f948ed425292a515def9702070a04cc5d9a42 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Thu, 10 Oct 2024 17:09:05 +0100 Subject: [PATCH 2/5] Convert ThreadContext collector initialization to use options hash I'm pretty happy with the same refactoring that was applied to the CpuAndWallTimeWorker, so I went ahead and did it for the ThreadContext. --- .../collectors_thread_context.c | 46 ++++++++----------- .../profiling/collectors/thread_context.rb | 18 ++++---- .../profiling/collectors/thread_context.rbs | 18 ++++---- 3 files changed, 37 insertions(+), 45 deletions(-) diff --git a/ext/datadog_profiling_native_extension/collectors_thread_context.c b/ext/datadog_profiling_native_extension/collectors_thread_context.c index ad1c489ad7..06049d247e 100644 --- a/ext/datadog_profiling_native_extension/collectors_thread_context.c +++ b/ext/datadog_profiling_native_extension/collectors_thread_context.c @@ -204,18 +204,7 @@ static void thread_context_collector_typed_data_free(void *state_ptr); static int hash_map_per_thread_context_mark(st_data_t key_thread, st_data_t _value, st_data_t _argument); static int hash_map_per_thread_context_free_values(st_data_t _thread, st_data_t value_per_thread_context, st_data_t _argument); static VALUE _native_new(VALUE klass); -static VALUE _native_initialize( - VALUE self, - VALUE collector_instance, - VALUE recorder_instance, - VALUE max_frames, - VALUE tracer_context_key, - VALUE endpoint_collection_enabled, - VALUE timeline_enabled, - VALUE waiting_for_gvl_threshold_ns, - VALUE otel_context_enabled, - VALUE allocation_type_enabled -); +static VALUE _native_initialize(int argc, VALUE *argv, DDTRACE_UNUSED VALUE _self); static VALUE _native_sample(VALUE self, VALUE collector_instance, VALUE profiler_overhead_stack_thread); static VALUE _native_on_gc_start(VALUE self, VALUE collector_instance); static VALUE _native_on_gc_finish(VALUE self, VALUE collector_instance); @@ -312,7 +301,7 @@ void collectors_thread_context_init(VALUE profiling_module) { // https://bugs.ruby-lang.org/issues/18007 for a discussion around this. rb_define_alloc_func(collectors_thread_context_class, _native_new); - rb_define_singleton_method(collectors_thread_context_class, "_native_initialize", _native_initialize, 9); + rb_define_singleton_method(collectors_thread_context_class, "_native_initialize", _native_initialize, -1); rb_define_singleton_method(collectors_thread_context_class, "_native_inspect", _native_inspect, 1); rb_define_singleton_method(collectors_thread_context_class, "_native_reset_after_fork", _native_reset_after_fork, 1); rb_define_singleton_method(testing_module, "_native_sample", _native_sample, 2); @@ -456,26 +445,29 @@ static VALUE _native_new(VALUE klass) { return instance; } -// TODO: Convert this to use options like CpuAndWallTimeWorker -static VALUE _native_initialize( - DDTRACE_UNUSED VALUE _self, - VALUE collector_instance, - VALUE recorder_instance, - VALUE max_frames, - VALUE tracer_context_key, - VALUE endpoint_collection_enabled, - VALUE timeline_enabled, - VALUE waiting_for_gvl_threshold_ns, - VALUE otel_context_enabled, - VALUE allocation_type_enabled -) { +static VALUE _native_initialize(int argc, VALUE *argv, DDTRACE_UNUSED VALUE _self) { + VALUE options; + rb_scan_args(argc, argv, "0:", &options); + if (options == Qnil) options = rb_hash_new(); + + VALUE self_instance = rb_hash_fetch(options, ID2SYM(rb_intern("self_instance"))); + VALUE recorder_instance = rb_hash_fetch(options, ID2SYM(rb_intern("recorder"))); + VALUE max_frames = rb_hash_fetch(options, ID2SYM(rb_intern("max_frames"))); + VALUE tracer_context_key = rb_hash_fetch(options, ID2SYM(rb_intern("tracer_context_key"))); + VALUE endpoint_collection_enabled = rb_hash_fetch(options, ID2SYM(rb_intern("endpoint_collection_enabled"))); + VALUE timeline_enabled = rb_hash_fetch(options, ID2SYM(rb_intern("timeline_enabled"))); + VALUE waiting_for_gvl_threshold_ns = rb_hash_fetch(options, ID2SYM(rb_intern("waiting_for_gvl_threshold_ns"))); + VALUE otel_context_enabled = rb_hash_fetch(options, ID2SYM(rb_intern("otel_context_enabled"))); + VALUE allocation_type_enabled = rb_hash_fetch(options, ID2SYM(rb_intern("allocation_type_enabled"))); + + ENFORCE_TYPE(max_frames, T_FIXNUM); ENFORCE_BOOLEAN(endpoint_collection_enabled); ENFORCE_BOOLEAN(timeline_enabled); ENFORCE_TYPE(waiting_for_gvl_threshold_ns, T_FIXNUM); ENFORCE_BOOLEAN(allocation_type_enabled); struct thread_context_collector_state *state; - TypedData_Get_Struct(collector_instance, struct thread_context_collector_state, &thread_context_collector_typed_data, state); + TypedData_Get_Struct(self_instance, struct thread_context_collector_state, &thread_context_collector_typed_data, state); // Update this when modifying state struct state->max_frames = sampling_buffer_check_max_frames(NUM2INT(max_frames)); diff --git a/lib/datadog/profiling/collectors/thread_context.rb b/lib/datadog/profiling/collectors/thread_context.rb index 14fbb31dcd..68b9b0ed20 100644 --- a/lib/datadog/profiling/collectors/thread_context.rb +++ b/lib/datadog/profiling/collectors/thread_context.rb @@ -26,15 +26,15 @@ def initialize( ) tracer_context_key = safely_extract_context_key_from(tracer) self.class._native_initialize( - self, - recorder, - max_frames, - tracer_context_key, - endpoint_collection_enabled, - timeline_enabled, - waiting_for_gvl_threshold_ns, - otel_context_enabled, - allocation_type_enabled, + self_instance: self, + recorder: recorder, + max_frames: max_frames, + tracer_context_key: tracer_context_key, + endpoint_collection_enabled: endpoint_collection_enabled, + timeline_enabled: timeline_enabled, + waiting_for_gvl_threshold_ns: waiting_for_gvl_threshold_ns, + otel_context_enabled: otel_context_enabled, + allocation_type_enabled: allocation_type_enabled, ) end diff --git a/sig/datadog/profiling/collectors/thread_context.rbs b/sig/datadog/profiling/collectors/thread_context.rbs index aba3cecda3..3a389231dd 100644 --- a/sig/datadog/profiling/collectors/thread_context.rbs +++ b/sig/datadog/profiling/collectors/thread_context.rbs @@ -14,15 +14,15 @@ module Datadog ) -> void def self._native_initialize: ( - Datadog::Profiling::Collectors::ThreadContext collector_instance, - Datadog::Profiling::StackRecorder recorder_instance, - ::Integer max_frames, - ::Symbol? tracer_context_key, - bool endpoint_collection_enabled, - bool timeline_enabled, - ::Integer waiting_for_gvl_threshold_ns, - (::Symbol? | bool) otel_context_enabled, - bool allocation_type_enabled, + self_instance: Datadog::Profiling::Collectors::ThreadContext, + recorder: Datadog::Profiling::StackRecorder, + max_frames: ::Integer, + tracer_context_key: ::Symbol?, + endpoint_collection_enabled: bool, + timeline_enabled: bool, + waiting_for_gvl_threshold_ns: ::Integer, + otel_context_enabled: (::Symbol? | bool), + allocation_type_enabled: bool, ) -> void def inspect: () -> ::String From 6b3d33dc71f45bc74c0fd968ad6d084f600c297e Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Thu, 10 Oct 2024 17:14:19 +0100 Subject: [PATCH 3/5] Remove `allocation_type_enabled` setting At some point we thought we may expose this as a user setting, but we never did, and we've been running with this setting enabled all the time, so let's just get rid of it. --- .../collectors_thread_context.c | 107 ++++++++---------- .../profiling/collectors/thread_context.rb | 4 +- .../profiling/collectors/thread_context.rbs | 2 - .../collectors/thread_context_spec.rb | 34 ------ 4 files changed, 50 insertions(+), 97 deletions(-) diff --git a/ext/datadog_profiling_native_extension/collectors_thread_context.c b/ext/datadog_profiling_native_extension/collectors_thread_context.c index 06049d247e..5cfb07a9a4 100644 --- a/ext/datadog_profiling_native_extension/collectors_thread_context.c +++ b/ext/datadog_profiling_native_extension/collectors_thread_context.c @@ -139,8 +139,6 @@ struct thread_context_collector_state { bool timeline_enabled; // Used to control context collection otel_context_enabled otel_context_enabled; - // Used to omit class information from collected allocation data - bool allocation_type_enabled; // Used when calling monotonic_to_system_epoch_ns monotonic_to_system_epoch_state time_converter_state; // Used to identify the main thread, to give it a fallback name @@ -425,7 +423,6 @@ static VALUE _native_new(VALUE klass) { state->endpoint_collection_enabled = true; state->timeline_enabled = true; state->otel_context_enabled = OTEL_CONTEXT_ENABLED_FALSE; - state->allocation_type_enabled = true; 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; @@ -458,13 +455,11 @@ static VALUE _native_initialize(int argc, VALUE *argv, DDTRACE_UNUSED VALUE _sel VALUE timeline_enabled = rb_hash_fetch(options, ID2SYM(rb_intern("timeline_enabled"))); VALUE waiting_for_gvl_threshold_ns = rb_hash_fetch(options, ID2SYM(rb_intern("waiting_for_gvl_threshold_ns"))); VALUE otel_context_enabled = rb_hash_fetch(options, ID2SYM(rb_intern("otel_context_enabled"))); - VALUE allocation_type_enabled = rb_hash_fetch(options, ID2SYM(rb_intern("allocation_type_enabled"))); ENFORCE_TYPE(max_frames, T_FIXNUM); ENFORCE_BOOLEAN(endpoint_collection_enabled); ENFORCE_BOOLEAN(timeline_enabled); ENFORCE_TYPE(waiting_for_gvl_threshold_ns, T_FIXNUM); - ENFORCE_BOOLEAN(allocation_type_enabled); struct thread_context_collector_state *state; TypedData_Get_Struct(self_instance, struct thread_context_collector_state, &thread_context_collector_typed_data, state); @@ -485,7 +480,6 @@ static VALUE _native_initialize(int argc, VALUE *argv, DDTRACE_UNUSED VALUE _sel } else { rb_raise(rb_eArgError, "Unexpected value for otel_context_enabled: %+" PRIsVALUE, otel_context_enabled); } - state->allocation_type_enabled = (allocation_type_enabled == Qtrue); global_waiting_for_gvl_threshold_ns = NUM2UINT(waiting_for_gvl_threshold_ns); @@ -1103,7 +1097,6 @@ static VALUE _native_inspect(DDTRACE_UNUSED VALUE _self, VALUE collector_instanc rb_str_concat(result, rb_sprintf(" endpoint_collection_enabled=%"PRIsVALUE, state->endpoint_collection_enabled ? Qtrue : Qfalse)); rb_str_concat(result, rb_sprintf(" timeline_enabled=%"PRIsVALUE, state->timeline_enabled ? Qtrue : Qfalse)); rb_str_concat(result, rb_sprintf(" otel_context_enabled=%d", state->otel_context_enabled)); - rb_str_concat(result, rb_sprintf(" allocation_type_enabled=%"PRIsVALUE, state->allocation_type_enabled ? Qtrue : Qfalse)); rb_str_concat(result, rb_sprintf( " time_converter_state={.system_epoch_ns_reference=%ld, .delta_to_epoch_ns=%ld}", state->time_converter_state.system_epoch_ns_reference, @@ -1407,62 +1400,60 @@ void thread_context_collector_sample_allocation(VALUE self_instance, unsigned in ddog_CharSlice *optional_class_name = NULL; char imemo_type[100]; - if (state->allocation_type_enabled) { - optional_class_name = &class_name; - - if ( - type == RUBY_T_OBJECT || - type == RUBY_T_CLASS || - type == RUBY_T_MODULE || - type == RUBY_T_FLOAT || - type == RUBY_T_STRING || - type == RUBY_T_REGEXP || - type == RUBY_T_ARRAY || - type == RUBY_T_HASH || - type == RUBY_T_STRUCT || - type == RUBY_T_BIGNUM || - type == RUBY_T_FILE || - type == RUBY_T_DATA || - type == RUBY_T_MATCH || - type == RUBY_T_COMPLEX || - type == RUBY_T_RATIONAL || - type == RUBY_T_NIL || - type == RUBY_T_TRUE || - type == RUBY_T_FALSE || - type == RUBY_T_SYMBOL || - type == RUBY_T_FIXNUM - ) { - VALUE klass = rb_class_of(new_object); - - // Ruby sometimes plays a bit fast and loose with some of its internal objects, e.g. - // `rb_str_tmp_frozen_acquire` allocates a string with no class (klass=0). - // Thus, we need to make sure there's actually a class before getting its name. - - if (klass != 0) { - const char *name = rb_class2name(klass); - size_t name_length = name != NULL ? strlen(name) : 0; - - if (name_length > 0) { - class_name = (ddog_CharSlice) {.ptr = name, .len = name_length}; - } else { - // @ivoanjo: I'm not sure this can ever happen, but just-in-case - class_name = ruby_value_type_to_class_name(type); - } + optional_class_name = &class_name; + + if ( + type == RUBY_T_OBJECT || + type == RUBY_T_CLASS || + type == RUBY_T_MODULE || + type == RUBY_T_FLOAT || + type == RUBY_T_STRING || + type == RUBY_T_REGEXP || + type == RUBY_T_ARRAY || + type == RUBY_T_HASH || + type == RUBY_T_STRUCT || + type == RUBY_T_BIGNUM || + type == RUBY_T_FILE || + type == RUBY_T_DATA || + type == RUBY_T_MATCH || + type == RUBY_T_COMPLEX || + type == RUBY_T_RATIONAL || + type == RUBY_T_NIL || + type == RUBY_T_TRUE || + type == RUBY_T_FALSE || + type == RUBY_T_SYMBOL || + type == RUBY_T_FIXNUM + ) { + VALUE klass = rb_class_of(new_object); + + // Ruby sometimes plays a bit fast and loose with some of its internal objects, e.g. + // `rb_str_tmp_frozen_acquire` allocates a string with no class (klass=0). + // Thus, we need to make sure there's actually a class before getting its name. + + if (klass != 0) { + const char *name = rb_class2name(klass); + size_t name_length = name != NULL ? strlen(name) : 0; + + if (name_length > 0) { + class_name = (ddog_CharSlice) {.ptr = name, .len = name_length}; } else { - // Fallback for objects with no class + // @ivoanjo: I'm not sure this can ever happen, but just-in-case class_name = ruby_value_type_to_class_name(type); } - } else if (type == RUBY_T_IMEMO) { - const char *imemo_string = imemo_kind(new_object); - if (imemo_string != NULL) { - snprintf(imemo_type, 100, "(VM Internal, T_IMEMO, %s)", imemo_string); - class_name = (ddog_CharSlice) {.ptr = imemo_type, .len = strlen(imemo_type)}; - } else { // Ruby < 3 - class_name = DDOG_CHARSLICE_C("(VM Internal, T_IMEMO)"); - } } else { - class_name = ruby_vm_type; // For other weird internal things we just use the VM type + // Fallback for objects with no class + class_name = ruby_value_type_to_class_name(type); + } + } else if (type == RUBY_T_IMEMO) { + const char *imemo_string = imemo_kind(new_object); + if (imemo_string != NULL) { + snprintf(imemo_type, 100, "(VM Internal, T_IMEMO, %s)", imemo_string); + class_name = (ddog_CharSlice) {.ptr = imemo_type, .len = strlen(imemo_type)}; + } else { // Ruby < 3 + class_name = DDOG_CHARSLICE_C("(VM Internal, T_IMEMO)"); } + } else { + class_name = ruby_vm_type; // For other weird internal things we just use the VM type } track_object(state->recorder_instance, new_object, sample_weight, optional_class_name); diff --git a/lib/datadog/profiling/collectors/thread_context.rb b/lib/datadog/profiling/collectors/thread_context.rb index 68b9b0ed20..ec4491b5cd 100644 --- a/lib/datadog/profiling/collectors/thread_context.rb +++ b/lib/datadog/profiling/collectors/thread_context.rb @@ -21,8 +21,7 @@ def initialize( endpoint_collection_enabled:, timeline_enabled:, waiting_for_gvl_threshold_ns:, - otel_context_enabled:, - allocation_type_enabled: true + otel_context_enabled: ) tracer_context_key = safely_extract_context_key_from(tracer) self.class._native_initialize( @@ -34,7 +33,6 @@ def initialize( timeline_enabled: timeline_enabled, waiting_for_gvl_threshold_ns: waiting_for_gvl_threshold_ns, otel_context_enabled: otel_context_enabled, - allocation_type_enabled: allocation_type_enabled, ) end diff --git a/sig/datadog/profiling/collectors/thread_context.rbs b/sig/datadog/profiling/collectors/thread_context.rbs index 3a389231dd..76f649c6ee 100644 --- a/sig/datadog/profiling/collectors/thread_context.rbs +++ b/sig/datadog/profiling/collectors/thread_context.rbs @@ -10,7 +10,6 @@ module Datadog timeline_enabled: bool, waiting_for_gvl_threshold_ns: ::Integer, otel_context_enabled: (::Symbol? | bool), - ?allocation_type_enabled: bool, ) -> void def self._native_initialize: ( @@ -22,7 +21,6 @@ module Datadog timeline_enabled: bool, waiting_for_gvl_threshold_ns: ::Integer, otel_context_enabled: (::Symbol? | bool), - allocation_type_enabled: bool, ) -> void def inspect: () -> ::String diff --git a/spec/datadog/profiling/collectors/thread_context_spec.rb b/spec/datadog/profiling/collectors/thread_context_spec.rb index 718aa58818..297fdcd2b9 100644 --- a/spec/datadog/profiling/collectors/thread_context_spec.rb +++ b/spec/datadog/profiling/collectors/thread_context_spec.rb @@ -39,7 +39,6 @@ let(:tracer) { nil } let(:endpoint_collection_enabled) { true } let(:timeline_enabled) { false } - let(:allocation_type_enabled) { true } # This mirrors the use of RUBY_FIXNUM_MAX for GVL_WAITING_ENABLED_EMPTY in the native code; it may need adjusting if we # ever want to support more platforms let(:gvl_waiting_enabled_empty_magic_value) { 2**62 - 1 } @@ -55,7 +54,6 @@ timeline_enabled: timeline_enabled, waiting_for_gvl_threshold_ns: waiting_for_gvl_threshold_ns, otel_context_enabled: otel_context_enabled, - allocation_type_enabled: allocation_type_enabled, ) end @@ -1595,16 +1593,6 @@ def sample_and_check(expected_state:) expect(single_sample.labels.fetch(:"allocation class")).to eq klass end - - context "when allocation_type_enabled is false" do - let(:allocation_type_enabled) { false } - - it "does not record the correct class for the passed object" do - sample_allocation(weight: 123, new_object: object) - - expect(single_sample.labels).to_not include("allocation class": anything) - end - end end end @@ -1624,18 +1612,6 @@ def sample_and_check(expected_state:) expect(single_sample.labels.fetch(:"allocation class")).to eq "File" end - - context "when allocation_type_enabled is false" do - let(:allocation_type_enabled) { false } - - it "does not record the correct class for the passed object" do - File.open(__FILE__) do |file| - sample_allocation(weight: 123, new_object: file) - end - - expect(single_sample.labels).to_not include("allocation class": anything) - end - end end context "when sampling a Struct" do @@ -1654,16 +1630,6 @@ def sample_and_check(expected_state:) expect(single_sample.labels.fetch(:"allocation class")).to eq "ThreadContextSpec::TestStruct" end - - context "when allocation_type_enabled is false" do - let(:allocation_type_enabled) { false } - - it "does not record the correct class for the passed object" do - sample_allocation(weight: 123, new_object: ThreadContextSpec::TestStruct.new) - - expect(single_sample.labels).to_not include("allocation class": anything) - end - end end end From 86c39073f4cf398f37aca56d7571fb4ad18ec275 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Thu, 10 Oct 2024 17:16:59 +0100 Subject: [PATCH 4/5] Minor: Move class docs to top of file --- .../collectors_cpu_and_wall_time_worker.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c b/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c index c1b872bf7e..aaf131cbe1 100644 --- a/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c +++ b/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c @@ -17,13 +17,6 @@ #include "setup_signal_handler.h" #include "time_helpers.h" -#define ERR_CLOCK_FAIL "failed to get clock time" - -// Maximum allowed value for an allocation weight. Attempts to use higher values will result in clamping. -// See https://docs.google.com/document/d/1lWLB714wlLBBq6T4xZyAc4a5wtWhSmr4-hgiPKeErlA/edit#heading=h.ugp0zxcj5iqh -// (Datadog-only link) for research backing the choice of this value. -unsigned int MAX_ALLOC_WEIGHT = 10000; - // Used to trigger the execution of Collectors::ThreadState, which implements all of the sampling logic // itself; this class only implements the "when to do it" part. // @@ -83,6 +76,13 @@ unsigned int MAX_ALLOC_WEIGHT = 10000; // // --- +#define ERR_CLOCK_FAIL "failed to get clock time" + +// Maximum allowed value for an allocation weight. Attempts to use higher values will result in clamping. +// See https://docs.google.com/document/d/1lWLB714wlLBBq6T4xZyAc4a5wtWhSmr4-hgiPKeErlA/edit#heading=h.ugp0zxcj5iqh +// (Datadog-only link) for research backing the choice of this value. +unsigned int MAX_ALLOC_WEIGHT = 10000; + #ifndef NO_POSTPONED_TRIGGER // Used to call the rb_postponed_job_trigger from Ruby 3.3+. These get initialized in // `collectors_cpu_and_wall_time_worker_init` below and always get reused after that. From e84e4005fbdab13d1985c6d9d5af5863e66c7a83 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Thu, 10 Oct 2024 17:18:18 +0100 Subject: [PATCH 5/5] Resolve old TODO Prof-correctness has been updated to stop using `_native_is_running?` in https://github.com/DataDog/prof-correctness/pull/42 . --- .../collectors_cpu_and_wall_time_worker.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c b/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c index aaf131cbe1..905db0c29d 100644 --- a/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c +++ b/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c @@ -315,8 +315,6 @@ void collectors_cpu_and_wall_time_worker_init(VALUE profiling_module) { rb_define_singleton_method(testing_module, "_native_current_sigprof_signal_handler", _native_current_sigprof_signal_handler, 0); rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_hold_signals", _native_hold_signals, 0); rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_resume_signals", _native_resume_signals, 0); - // TODO: Remove `_native_is_running` from `testing_module` (should be in class) once `prof-correctness` has been updated to not need it - rb_define_singleton_method(testing_module, "_native_is_running?", _native_is_running, 1); rb_define_singleton_method(testing_module, "_native_install_testing_signal_handler", _native_install_testing_signal_handler, 0); rb_define_singleton_method(testing_module, "_native_remove_testing_signal_handler", _native_remove_testing_signal_handler, 0); rb_define_singleton_method(testing_module, "_native_trigger_sample", _native_trigger_sample, 0);