From 5735eba9dfa9050cb045b4d52dc24761cc54985d Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Fri, 1 Dec 2023 09:35:17 +0000 Subject: [PATCH] [NO-TICKET] Minor: Clean up a few classes kept in global variables **What does this PR do?** This PR gets rid of two references to classes that were being kept as global variables in the profiling C extension. * The one in `stack_recorder.c` wasn't actually needed * The one in `http_transport.c` was only needed for an upcall to Ruby code in a rare error handling case, so I've replaced it with an actual lookup, so we don't need to keep the reference **Motivation:** Having as little global state as possible seems like a good idea, and we don't need to care about telling or not telling the Ruby GC about these. **Additional Notes:** N/A **How to test the change?** The change in `http_transport.c` already has code coverage -- I checked this by quickly writing the wrong code and validating that indeed this code was exercised and some of the tests failed as expected. --- ext/ddtrace_profiling_native_extension/http_transport.c | 7 +++++-- ext/ddtrace_profiling_native_extension/stack_recorder.c | 4 +--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/ext/ddtrace_profiling_native_extension/http_transport.c b/ext/ddtrace_profiling_native_extension/http_transport.c index d1b795f6e9e..704690174c5 100644 --- a/ext/ddtrace_profiling_native_extension/http_transport.c +++ b/ext/ddtrace_profiling_native_extension/http_transport.c @@ -16,7 +16,6 @@ static ID agent_id; // id of :agent in Ruby static ID log_failure_to_process_tag_id; // id of :log_failure_to_process_tag in Ruby -static VALUE http_transport_class = Qnil; static VALUE library_version_string = Qnil; struct call_exporter_without_gvl_arguments { @@ -54,7 +53,7 @@ static void interrupt_exporter_call(void *cancel_token); static VALUE ddtrace_version(void); void http_transport_init(VALUE profiling_module) { - http_transport_class = rb_define_class_under(profiling_module, "HttpTransport", rb_cObject); + VALUE http_transport_class = rb_define_class_under(profiling_module, "HttpTransport", rb_cObject); rb_define_singleton_method(http_transport_class, "_native_validate_exporter", _native_validate_exporter, 1); rb_define_singleton_method(http_transport_class, "_native_do_export", _native_do_export, 12); @@ -180,6 +179,10 @@ static ddog_Vec_Tag convert_tags(VALUE tags_as_array) { } static VALUE log_failure_to_process_tag(VALUE err_details) { + VALUE datadog_module = rb_const_get(rb_cObject, rb_intern("Datadog")); + VALUE profiling_module = rb_const_get(datadog_module, rb_intern("Profiling")); + VALUE http_transport_class = rb_const_get(profiling_module, rb_intern("HttpTransport")); + return rb_funcall(http_transport_class, log_failure_to_process_tag_id, 1, err_details); } diff --git a/ext/ddtrace_profiling_native_extension/stack_recorder.c b/ext/ddtrace_profiling_native_extension/stack_recorder.c index 41729b06c0d..a192a8d61ca 100644 --- a/ext/ddtrace_profiling_native_extension/stack_recorder.c +++ b/ext/ddtrace_profiling_native_extension/stack_recorder.c @@ -129,8 +129,6 @@ static VALUE ok_symbol = Qnil; // :ok in Ruby static VALUE error_symbol = Qnil; // :error in Ruby -static VALUE stack_recorder_class = Qnil; - // Note: Please DO NOT use `VALUE_STRING` anywhere else, instead use `DDOG_CHARSLICE_C`. // `VALUE_STRING` is only needed because older versions of gcc (4.9.2, used in our Ruby 2.2 CI test images) // tripped when compiling `enabled_value_types` using `-std=gnu99` due to the extra cast that is included in @@ -217,7 +215,7 @@ static VALUE _native_record_endpoint(DDTRACE_UNUSED VALUE _self, VALUE recorder_ static void reset_profile(ddog_prof_Profile *profile, ddog_Timespec *start_time /* Can be null */); void stack_recorder_init(VALUE profiling_module) { - stack_recorder_class = rb_define_class_under(profiling_module, "StackRecorder", rb_cObject); + VALUE stack_recorder_class = rb_define_class_under(profiling_module, "StackRecorder", rb_cObject); // Hosts methods used for testing the native code using RSpec VALUE testing_module = rb_define_module_under(stack_recorder_class, "Testing");