Respond to code cleanliness comments; still need to fix logic/impl

gyuheon0h · gyuheon0h · commit ef62258386ff · 2025-11-25T20:55:44.000Z
diff --git a/Rakefile b/Rakefile
@@ -229,6 +229,8 @@ namespace :spec do
     t.pattern = CORE_WITH_LIBDATADOG_API.join(', ')
     t.rspec_opts = args.to_a.join(' ')
   end.tap do |t|
+    # Core with libdatadog is dependent on profiling because crashtracking runtime stacks logic
+    # lives in the profiling extension (they share same logic accessing Ruby internals)
     Rake::Task[t.name].enhance(
       [
         "compile:libdatadog_api.#{RUBY_VERSION[/\d+.\d+/]}_#{RUBY_PLATFORM}",
diff --git a/ext/datadog_profiling_native_extension/crashtracking_runtime_stacks.c b/ext/datadog_profiling_native_extension/crashtracking_runtime_stacks.c
@@ -1,3 +1,9 @@
+// NOTE: This file is a part of the profiling native extension even though the
+// runtime stacks feature is consumed by the crashtracker. The profiling
+// extension already carries all the Ruby VM private header access and build
+// plumbing required to safely poke at internal structures. Sharing that setup
+// avoids duplicating another native extension with the same (fragile) access
+// patterns, and keeps the overall install/build surface area smaller.
 #include "extconf.h"
 
 #ifdef RUBY_MJIT_HEADER
@@ -32,7 +38,7 @@
   #endif
 #endif
 
-#include "runtime_stacks.h"
+#include "crashtracking_runtime_stacks.h"
 #include <datadog/crashtracker.h>
 #include "datadog_ruby_common.h"
 #include "private_vm_api_access.h"
@@ -41,13 +47,9 @@
 #include <errno.h>
 #include <string.h>
 
-// This was renamed in Ruby 3.2
-#if !defined(ccan_list_for_each) && defined(list_for_each)
-  #define ccan_list_for_each list_for_each
-#endif
-
 static VALUE _native_register_runtime_stack_callback(VALUE _self);
 static VALUE _native_is_runtime_callback_registered(DDTRACE_UNUSED VALUE _self);
+static const rb_data_type_t *crashtracker_thread_data_type = NULL;
 
 static void ruby_runtime_stack_callback(
   void (*emit_frame)(const ddog_crasht_RuntimeStackFrame*)
@@ -202,13 +204,9 @@ static void ruby_runtime_stack_callback(
   VALUE current_thread = rb_thread_current();
   if (current_thread == Qnil) return;
 
-  static const rb_data_type_t *thread_data_type = NULL;
-  if (thread_data_type == NULL) {
-    thread_data_type = RTYPEDDATA_TYPE(current_thread);
-    if (!thread_data_type) return;
-  }
+  if (crashtracker_thread_data_type == NULL) return;
 
-  rb_thread_t *th = (rb_thread_t *) rb_check_typeddata(current_thread, thread_data_type);
+  rb_thread_t *th = (rb_thread_t *) rb_check_typeddata(current_thread, crashtracker_thread_data_type);
   if (!th) return;
 
   const rb_execution_context_t *ec = th->ec;
@@ -374,6 +372,16 @@ static void ruby_runtime_stack_callback(
 }
 
 static VALUE _native_register_runtime_stack_callback(DDTRACE_UNUSED VALUE _self) {
+  if (crashtracker_thread_data_type == NULL) {
+    VALUE current_thread = rb_thread_current();
+    if (current_thread == Qnil) return Qfalse;
+
+    const rb_data_type_t *thread_data_type = RTYPEDDATA_TYPE(current_thread);
+    if (!thread_data_type) return Qfalse;
+
+    crashtracker_thread_data_type = thread_data_type;
+  }
+
   enum ddog_crasht_CallbackResult result = ddog_crasht_register_runtime_frame_callback(
     ruby_runtime_stack_callback
   );
@@ -392,8 +400,10 @@ static VALUE _native_is_runtime_callback_registered(DDTRACE_UNUSED VALUE _self)
   return ddog_crasht_is_runtime_callback_registered() ? Qtrue : Qfalse;
 }
 
-void runtime_stacks_init(VALUE datadog_module) {
-  VALUE runtime_stacks_class = rb_define_class_under(datadog_module, "RuntimeStacks", rb_cObject);
+void crashtracking_runtime_stacks_init(VALUE datadog_module) {
+  VALUE core_module = rb_define_module_under(datadog_module, "Core");
+  VALUE crashtracking_module = rb_define_module_under(core_module, "Crashtracking");
+  VALUE runtime_stacks_class = rb_define_class_under(crashtracking_module, "RuntimeStacks", rb_cObject);
 
   rb_define_singleton_method(
     runtime_stacks_class,
diff --git a/ext/datadog_profiling_native_extension/crashtracking_runtime_stacks.h b/ext/datadog_profiling_native_extension/crashtracking_runtime_stacks.h
@@ -0,0 +1,4 @@
+#pragma once
+
+void crashtracking_runtime_stacks_init(VALUE datadog_module);
+
diff --git a/ext/datadog_profiling_native_extension/profiling.c b/ext/datadog_profiling_native_extension/profiling.c
@@ -9,7 +9,7 @@
 #include "helpers.h"
 #include "private_vm_api_access.h"
 #include "ruby_helpers.h"
-#include "runtime_stacks.h"
+#include "crashtracking_runtime_stacks.h"
 #include "setup_signal_handler.h"
 #include "time_helpers.h"
 #include "unsafe_api_calls_check.h"
@@ -66,7 +66,7 @@ void DDTRACE_EXPORT Init_datadog_profiling_native_extension(void) {
   encoded_profile_init(profiling_module);
   http_transport_init(profiling_module);
   stack_recorder_init(profiling_module);
-  runtime_stacks_init(datadog_module);
+  crashtracking_runtime_stacks_init(datadog_module);
   unsafe_api_calls_check_init();
 
   // Hosts methods used for testing the native code using RSpec
diff --git a/ext/datadog_profiling_native_extension/runtime_stacks.h b/ext/datadog_profiling_native_extension/runtime_stacks.h
diff --git a/lib/datadog/core.rb b/lib/datadog/core.rb
@@ -20,14 +20,6 @@ module Core
       rescue LoadError => e
         e.message
       end
-
-    RUNTIME_STACKS_FAILURE =
-      begin
-        require 'datadog/runtime_stacks'
-        nil
-      rescue LoadError => e
-        e.message
-      end
   end
 
   DATADOG_ENV = Core::Configuration::ConfigHelper.new
diff --git a/lib/datadog/core/configuration/supported_configurations.rb b/lib/datadog/core/configuration/supported_configurations.rb
@@ -32,7 +32,7 @@ module Configuration
          "DD_APPSEC_TRACE_RATE_LIMIT" => {version: ["A"]},
          "DD_APPSEC_WAF_DEBUG" => {version: ["A"]},
          "DD_APPSEC_WAF_TIMEOUT" => {version: ["A"]},
-         "DD_CRASHTRACKER_EMIT_RUNTIME_STACKS" => {version: ["A"]},
+         "DD_CRASHTRACKING_EMIT_RUNTIME_STACKS" => {version: ["A"]},
          "DD_CRASHTRACKING_ENABLED" => {version: ["A"]},
          "DD_DATA_STREAMS_ENABLED" => {version: ["A"]},
          "DD_DBM_PROPAGATION_MODE" => {version: ["A"]},
diff --git a/lib/datadog/core/crashtracking/component.rb b/lib/datadog/core/crashtracking/component.rb
@@ -9,6 +9,14 @@
 module Datadog
   module Core
     module Crashtracking
+      RUNTIME_STACKS_FAILURE =
+        begin
+          require_relative 'crashtracking_runtime_stacks'
+          nil
+        rescue LoadError => e
+          e.message
+        end
+
       # Used to report Ruby VM crashes.
       #
       # NOTE: The crashtracker native state is a singleton;
@@ -57,9 +65,7 @@ def start
           Utils::AtForkMonkeyPatch.apply!
 
           start_or_update_on_fork(action: :start, tags: tags)
-          if DATADOG_ENV['DD_CRASHTRACKER_EMIT_RUNTIME_STACKS'] == 'true'
-            register_runtime_stack_callback
-          end
+          register_runtime_stack_callback if DATADOG_ENV['DD_CRASHTRACKING_EMIT_RUNTIME_STACKS'] == 'true'
 
           ONLY_ONCE.run do
             Utils::AtForkMonkeyPatch.at_fork(:child) do
@@ -84,39 +90,25 @@ def stop
           logger.error("Failed to stop crash tracking: #{e.message}")
         end
 
-        def runtime_callback_registered?
-          return false if Datadog::Core::RUNTIME_STACKS_FAILURE
-          return false unless Datadog.const_defined?(:RuntimeStacks, false)
-
-          Datadog::RuntimeStacks._native_is_runtime_callback_registered
-        rescue => e
-          logger.debug("Runtime stack callback status check not available: #{e.message}")
-          false
-        end
-
         private
 
         def register_runtime_stack_callback
           # Check if runtime_stacks extension loaded successfully
-          if Datadog::Core::RUNTIME_STACKS_FAILURE
-            logger.debug("Skipping runtime stack callback registration: #{Datadog::Core::RUNTIME_STACKS_FAILURE}")
+          if Crashtracking::RUNTIME_STACKS_FAILURE
+            logger.debug("Skipping runtime stack callback registration: #{Crashtracking::RUNTIME_STACKS_FAILURE}")
             return
           end
 
-          unless Datadog.const_defined?(:RuntimeStacks, false)
-            logger.debug('Skipping runtime stack callback registration: Datadog::RuntimeStacks not defined')
+          unless Crashtracking.const_defined?(:RuntimeStacks, false)
+            logger.debug('Skipping runtime stack callback registration: Crashtracking::RuntimeStacks not defined')
             return
           end
 
-          success = Datadog::RuntimeStacks._native_register_runtime_stack_callback
+          success = Crashtracking::RuntimeStacks._native_register_runtime_stack_callback
 
-          unless success
-            error_message = 'Failed to register runtime stack callback: registration returned false'
-            logger.error(error_message)
-            raise StandardError, error_message
-          end
+          raise StandardError, 'Failed to register runtime stack callback: registration returned false' unless success
         rescue => e
-          logger.error("Failed to register runtime stack callback: #{e.message}")
+          logger.warn("Failed to register runtime stack callback: #{e.message}")
           raise
         end
 
diff --git a/lib/datadog/core/crashtracking/crashtracking_runtime_stacks.rb b/lib/datadog/core/crashtracking/crashtracking_runtime_stacks.rb
diff --git a/sig/datadog/core.rbs b/sig/datadog/core.rbs
@@ -1,7 +1,6 @@
 module Datadog
   module Core
     LIBDATADOG_API_FAILURE: ::String?
-    RUNTIME_STACKS_FAILURE: ::String?
     extend Core::Deprecations
   end
 end
diff --git a/sig/datadog/core/crashtracking/component.rbs b/sig/datadog/core/crashtracking/component.rbs
@@ -1,6 +1,8 @@
 module Datadog
   module Core
     module Crashtracking
+      RUNTIME_STACKS_FAILURE: ::String?
+
       class Component
         ONLY_ONCE: Core::Utils::OnlyOnce
 
diff --git a/sig/datadog/core/crashtracking/crashtracking_runtime_stacks.rbs b/sig/datadog/core/crashtracking/crashtracking_runtime_stacks.rbs
@@ -0,0 +1,10 @@
+module Datadog
+  module Core
+    module Crashtracking
+      class RuntimeStacks
+      end
+    end
+  end
+end
+
+
diff --git a/sig/datadog/runtime_stacks.rbs b/sig/datadog/runtime_stacks.rbs
diff --git a/spec/datadog/core/crashtracking/component_spec.rb b/spec/datadog/core/crashtracking/component_spec.rb
@@ -1,6 +1,25 @@
 require 'spec_helper'
 require 'datadog/core/crashtracking/component'
-require 'datadog/runtime_stacks'
+require 'datadog/core/crashtracking/crashtracking_runtime_stacks'
+
+# Test helper toexpose runtime stack callback state used only inside this spec
+module Datadog
+  module Core
+    module Crashtracking
+      class Component
+        def runtime_callback_registered?
+          return false if Crashtracking::RUNTIME_STACKS_FAILURE
+          return false unless Crashtracking.const_defined?(:RuntimeStacks, false)
+
+          Crashtracking::RuntimeStacks._native_is_runtime_callback_registered
+        rescue => e
+          logger.debug("Runtime stack callback status check not available: #{e.message}")
+          false
+        end
+      end
+    end
+  end
+end
 
 require 'webrick'
 require 'fiddle'
@@ -10,7 +29,7 @@
 
   shared_context 'runtime stack emission enabled' do
     around do |example|
-      ClimateControl.modify('DD_CRASHTRACKER_EMIT_RUNTIME_STACKS' => 'true') do
+      ClimateControl.modify('DD_CRASHTRACKING_EMIT_RUNTIME_STACKS' => 'true') do
         example.run
       end
     end
@@ -125,7 +144,7 @@
         crashtracker = build_crashtracker(logger: logger)
 
         expect(described_class).to receive(:_native_start_or_update_on_fork)
-        expect(Datadog::RuntimeStacks).to receive(:_native_register_runtime_stack_callback).and_return(true)
+        expect(Datadog::Core::Crashtracking::RuntimeStacks).to receive(:_native_register_runtime_stack_callback).and_return(true)
 
         crashtracker.start
       end
@@ -136,9 +155,9 @@
           error = StandardError.new('Callback registration failed')
 
           expect(described_class).to receive(:_native_start_or_update_on_fork)
-          expect(Datadog::RuntimeStacks).to receive(:_native_register_runtime_stack_callback).and_raise(error)
+          expect(Datadog::Core::Crashtracking::RuntimeStacks).to receive(:_native_register_runtime_stack_callback).and_raise(error)
           allow(logger).to receive(:debug) # Allow other debug messages
-          expect(logger).to receive(:error).with('Failed to register runtime stack callback: Callback registration failed')
+          expect(logger).to receive(:warn).with('Failed to register runtime stack callback: Callback registration failed')
 
           expect { crashtracker.start }.to raise_error(error)
         end
@@ -428,15 +447,15 @@ def ruby_method_with_c_calls
       it 'returns true when callback is registered' do
         crashtracker = build_crashtracker(logger: logger)
 
-        expect(Datadog::RuntimeStacks).to receive(:_native_is_runtime_callback_registered).and_return(true)
+        expect(Datadog::Core::Crashtracking::RuntimeStacks).to receive(:_native_is_runtime_callback_registered).and_return(true)
 
         expect(crashtracker.runtime_callback_registered?).to be true
       end
 
       it 'returns false when callback is not registered' do
         crashtracker = build_crashtracker(logger: logger)
 
-        expect(Datadog::RuntimeStacks).to receive(:_native_is_runtime_callback_registered).and_return(false)
+        expect(Datadog::Core::Crashtracking::RuntimeStacks).to receive(:_native_is_runtime_callback_registered).and_return(false)
 
         expect(crashtracker.runtime_callback_registered?).to be false
       end
@@ -445,7 +464,7 @@ def ruby_method_with_c_calls
         crashtracker = build_crashtracker(logger: logger)
         error = StandardError.new('Native error')
 
-        expect(Datadog::RuntimeStacks).to receive(:_native_is_runtime_callback_registered).and_raise(error)
+        expect(Datadog::Core::Crashtracking::RuntimeStacks).to receive(:_native_is_runtime_callback_registered).and_raise(error)
         expect(logger).to receive(:debug).with('Runtime stack callback status check not available: Native error')
 
         expect(crashtracker.runtime_callback_registered?).to be false
diff --git a/supported-configurations.json b/supported-configurations.json
@@ -76,7 +76,7 @@
     "DD_APPSEC_WAF_TIMEOUT": {
       "version": ["A"]
     },
-    "DD_CRASHTRACKER_EMIT_RUNTIME_STACKS": {
+    "DD_CRASHTRACKING_EMIT_RUNTIME_STACKS": {
       "version": ["A"]
     },
     "DD_CRASHTRACKING_ENABLED": {

Original file line number	Diff line number	Diff line change
`@@ -229,6 +229,8 @@ namespace :spec do`
`229`	`229`	`t.pattern = CORE_WITH_LIBDATADOG_API.join(', ')`
`230`	`230`	`t.rspec_opts = args.to_a.join(' ')`
`231`	`231`	`end.tap do \|t\|`
	`232`	`+ # Core with libdatadog is dependent on profiling because crashtracking runtime stacks logic`
	`233`	`+ # lives in the profiling extension (they share same logic accessing Ruby internals)`
`232`	`234`	`Rake::Task[t.name].enhance(`
`233`	`235`	`[`
`234`	`236`	`"compile:libdatadog_api.#{RUBY_VERSION[/\d+.\d+/]}_#{RUBY_PLATFORM}",`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +#pragma once
++
 +void crashtracking_runtime_stacks_init(VALUE datadog_module);
++