Skip to content

Commit

Permalink
[NO-TICKET] Minor: Clean up a few classes kept in global variables
Browse files Browse the repository at this point in the history
**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.
  • Loading branch information
ivoanjo committed Dec 1, 2023
1 parent 96d4457 commit 5735eba
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 5 deletions.
7 changes: 5 additions & 2 deletions ext/ddtrace_profiling_native_extension/http_transport.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down
4 changes: 1 addition & 3 deletions ext/ddtrace_profiling_native_extension/stack_recorder.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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");

Expand Down

0 comments on commit 5735eba

Please sign in to comment.