SymDB remote: route diagnostics through component.logger

p-ddsign · p-ddsign · commit 794fa9855cd5 · 2026-05-18T12:15:37.000-04:00
#5717 used Datadog.logger directly in Remote.process_change / enable_upload / disable_upload / parse_config — a Component Pattern violation, since the component was in scope at every call site and the logger was the dependency that should have been used. - Expose Component#logger via attr_reader (and update the RBS). - Replace Datadog.logger with component.logger in the four sites where component is available. The receiver-block fallback at remote.rb:48 keeps Datadog.logger because the component lookup itself may return nil there. - Thread the logger explicitly through parse_config(content, logger) rather than relying on a global. - Update the RBS for parse_config and remote_spec.rb's parse_config / component test doubles. Also adds a justification comment to the remote.rb:106 # steep:ignore NoMethod directive, matching the pattern already used at line 113.
diff --git a/lib/datadog/symbol_database/component.rb b/lib/datadog/symbol_database/component.rb
@@ -107,7 +107,7 @@ def self.build(settings, agent_settings, logger, telemetry: nil)
         end
       end
 
-      attr_reader :settings, :last_upload_time, :last_upload_scope_count, :upload_in_progress
+      attr_reader :settings, :logger, :last_upload_time, :last_upload_scope_count, :upload_in_progress
 
       # Initialize component.
       # @param settings [Configuration::Settings] Tracer settings
@@ -224,7 +224,10 @@ def stop_upload
       def wait_for_idle(timeout: 30)
         deadline = Datadog::Core::Utils::Time.get_time + timeout
         Component.upload_done_mutex.synchronize do
-          until Component.send(:instance_variable_get, :@uploaded_this_process)
+          # Read @uploaded_this_process directly: we already hold
+          # Component.upload_done_mutex here, and uploaded_this_process?
+          # would try to re-acquire it (non-reentrant), deadlocking.
+          until Component.instance_variable_get(:@uploaded_this_process)
             remaining = deadline - Datadog::Core::Utils::Time.get_time
             return false if remaining <= 0
             Component.upload_done_cv.wait(Component.upload_done_mutex, remaining)
diff --git a/lib/datadog/symbol_database/remote.rb b/lib/datadog/symbol_database/remote.rb
@@ -102,11 +102,13 @@ def process_change(component, change, telemetry)
             disable_upload(component)
             change.previous&.applied
           else
-            Datadog.logger.debug { "symdb: unrecognized change type: #{change.type}" }
+            component.logger.debug { "symdb: unrecognized change type: #{change.type}" }
+            # Steep cannot narrow `change.content` from a respond_to? check — it sees
+            # the Repository::Change union type where `Deleted` lacks `content`.
             change.content.errored("Unrecognized change type: #{change.type}") if change.respond_to?(:content) # steep:ignore NoMethod
           end
         rescue => e
-          Datadog.logger.debug { "symdb: error processing remote config change: #{e.class}: #{e.message}" }
+          component.logger.debug { "symdb: error processing remote config change: #{e.class}: #{e.message}" }
           telemetry&.report(e, description: 'symdb: error processing remote config change')
           # Rescue runs regardless of which branch raised — Steep cannot narrow the
           # union type from a respond_to? check.
@@ -120,17 +122,17 @@ def process_change(component, change, telemetry)
         # @return [void]
         # @api private
         def enable_upload(component, content)
-          config = parse_config(content)
+          config = parse_config(content, component.logger)
 
           unless config
             return
           end
 
           if config['upload_symbols']
-            Datadog.logger.debug { "symdb: upload enabled via remote config" }
+            component.logger.debug { "symdb: upload enabled via remote config" }
             component.start_upload
           else
-            Datadog.logger.debug { "symdb: upload disabled in config" }
+            component.logger.debug { "symdb: upload disabled in config" }
           end
         end
 
@@ -139,28 +141,29 @@ def enable_upload(component, content)
         # @return [void]
         # @api private
         def disable_upload(component)
-          Datadog.logger.debug { "symdb: upload disabled via remote config" }
+          component.logger.debug { "symdb: upload disabled via remote config" }
           component.stop_upload
         end
 
         # Parse and validate remote config content.
         # @param content [Content] Remote config content
+        # @param logger [SymbolDatabase::Logger] Logger for invalid-config diagnostics
         # @return [Hash, nil] Parsed config or nil if invalid
         # @api private
         #
         # JSON::ParserError is intentionally NOT rescued here — it propagates to
         # process_change's rescue, which logs and reports to telemetry. Catching
         # it locally would swallow the error from telemetry observability.
-        def parse_config(content)
+        def parse_config(content, logger)
           config = JSON.parse(content.data)
 
           unless config.is_a?(Hash)
-            Datadog.logger.debug { "symdb: invalid config format: expected Hash, got #{config.class}" }
+            logger.debug { "symdb: invalid config format: expected Hash, got #{config.class}" }
             return nil
           end
 
           unless config.key?('upload_symbols')
-            Datadog.logger.debug { "symdb: missing 'upload_symbols' key in config" }
+            logger.debug { "symdb: missing 'upload_symbols' key in config" }
             return nil
           end
 
diff --git a/sig/datadog/symbol_database/component.rbs b/sig/datadog/symbol_database/component.rbs
@@ -37,6 +37,7 @@ module Datadog
       def initialize: (Datadog::Core::Configuration::Settings settings, Datadog::Core::Configuration::AgentSettings agent_settings, SymbolDatabase::Logger logger, ?telemetry: Datadog::Core::Telemetry::Component?) -> void
 
       attr_reader settings: Datadog::Core::Configuration::Settings
+      attr_reader logger: SymbolDatabase::Logger
       attr_reader last_upload_time: ::Time?
       attr_reader last_upload_scope_count: ::Integer?
       attr_reader upload_in_progress: bool
diff --git a/sig/datadog/symbol_database/remote.rbs b/sig/datadog/symbol_database/remote.rbs
@@ -19,7 +19,7 @@ module Datadog
 
       def self.disable_upload: (Component component) -> void
 
-      def self.parse_config: (Datadog::Core::Remote::Configuration::Content content) -> ::Hash[::String, untyped]?
+      def self.parse_config: (Datadog::Core::Remote::Configuration::Content content, SymbolDatabase::Logger logger) -> ::Hash[::String, untyped]?
     end
   end
 end
diff --git a/spec/datadog/symbol_database/remote_spec.rb b/spec/datadog/symbol_database/remote_spec.rb
@@ -13,7 +13,8 @@
 require 'datadog/symbol_database/component'
 
 RSpec.describe Datadog::SymbolDatabase::Remote do
-  let(:component) { instance_double(Datadog::SymbolDatabase::Component) }
+  let(:logger) { instance_double(Datadog::SymbolDatabase::Logger, debug: nil) }
+  let(:component) { instance_double(Datadog::SymbolDatabase::Component, logger: logger) }
 
   # Helper to create a mock change object
   def mock_change(type:, data:)
@@ -166,26 +167,26 @@ def mock_change(type:, data:)
   describe '.parse_config' do
     it 'parses valid upload_symbols config' do
       content = instance_double('Content', data: '{"upload_symbols": true}')
-      result = described_class.send(:parse_config, content)
+      result = described_class.send(:parse_config, content, logger)
       expect(result).to eq({'upload_symbols' => true})
     end
 
     it 'returns nil for missing upload_symbols key' do
       content = instance_double('Content', data: '{"other": true}')
-      result = described_class.send(:parse_config, content)
+      result = described_class.send(:parse_config, content, logger)
       expect(result).to be_nil
     end
 
     it 'raises JSON::ParserError for invalid JSON (caller process_change rescues + reports)' do
       content = instance_double('Content', data: 'bad json')
       expect {
-        described_class.send(:parse_config, content)
+        described_class.send(:parse_config, content, logger)
       }.to raise_error(JSON::ParserError)
     end
 
     it 'returns nil for non-Hash JSON' do
       content = instance_double('Content', data: '[1, 2, 3]')
-      result = described_class.send(:parse_config, content)
+      result = described_class.send(:parse_config, content, logger)
       expect(result).to be_nil
     end
   end
