feat(data-collection): Change stack_frame_variables to KeyValueCollection

sentry-rails/spec/active_job/support/harness.rb

@@ -61,7 +61,7 @@ def perform(*_args, **_kwargs)
       config.dsn = "http://12345:67890@sentry.localdomain:3000/sentry/42"
       config.transport.transport_class = Sentry::DummyTransport
       config.background_worker_threads = 0
-      config.include_local_variables = true
+      config.data_collection.stack_frame_variables = true
       configure_sentry.call(config, ::Rails.application) if configure_sentry
     end
 

sentry-rails/spec/support/test_helper.rb

@@ -20,7 +20,7 @@ def make_basic_app(&block)
 
               # For sending events synchronously
               config.background_worker_threads = 0
-              config.include_local_variables = true
+              config.data_collection.stack_frame_variables = true
 
               yield(config, app) if block_given?
             end

sentry-ruby/lib/sentry-ruby.rb

@@ -277,7 +277,7 @@ def init(&block)
       @background_worker = Sentry::BackgroundWorker.new(config)
       @session_flusher = config.session_tracking? ? Sentry::SessionFlusher.new(config, client) : nil
       @backpressure_monitor = config.enable_backpressure_handling ? Sentry::BackpressureMonitor.new(config, client) : nil
-      exception_locals_tp.enable if config.include_local_variables
+      exception_locals_tp.enable if config.data_collection.collect_stack_frame_variables?
       at_exit { close }
     end
 
@@ -301,7 +301,7 @@ def close
         client.configuration.run_after_close_callbacks
         client.flush
 
-        if client.configuration.include_local_variables
+        if client.configuration.data_collection.collect_stack_frame_variables?
           exception_locals_tp.disable
         end
       end

sentry-ruby/lib/sentry/configuration.rb

@@ -171,7 +171,12 @@ class Configuration
     # Whether to capture local variables from the raised exception's frame. Default is false.
     # @deprecated Use {#data_collection} and `stack_frame_variables` instead.
     # @return [Boolean]
-    attr_accessor :include_local_variables
+    attr_reader :include_local_variables
+
+    def include_local_variables=(value)
+      @include_local_variables = value
+      @data_collection.backfill_stack_frame_variables(self)
+    end
 
     # Whether to capture events and traces into Spotlight. Default is false.
     # If you set this to true, Sentry will send events and traces to the local
@@ -529,6 +534,7 @@ def build_validation_proc(optional, type)
     def initialize
       run_callbacks(:before, :initialize)
 
+      self.data_collection = DataCollection.new
       self.app_dirs_pattern = APP_DIRS_PATTERN
       self.debug = Sentry::Utils::EnvHelper.env_to_bool(ENV["SENTRY_DEBUG"])
       self.background_worker_threads = (processor_count / 2.0).ceil
@@ -552,7 +558,6 @@ def initialize
 
       self.sample_rate = 1.0
       self.send_modules = true
-      self.data_collection = DataCollection.new
       self.send_default_pii = false
       self.skip_rake_integration = false
       self.send_client_reports = true

sentry-ruby/lib/sentry/data_collection.rb

@@ -108,9 +108,10 @@ def initialize(document:, variables:)
     # @default `true`
     attr_accessor :queues
 
-    # @return [Boolean]
-    # @default `false`
-    attr_accessor :stack_frame_variables
+    # @return [Boolean, KeyValueCollection]
+    # A boolean is shorthand for `mode: :deny_list` (`true`) or `mode: :off` (`false`).
+    # @default `false` (Ruby-specific to avoid TracePoint overhead)
+    attr_reader :stack_frame_variables
 
     # @return [Integer]
     # @default `3`
@@ -128,8 +129,8 @@ def self.default_filter
     # Builds data collection settings compatible with the legacy send_default_pii
     # configuration.
     def self.backfill(configuration)
-      # the new DataCollection defaults are already correct if pii is enabled
       data_collection = new
+      data_collection.backfill_stack_frame_variables(configuration)
       return data_collection if configuration.send_default_pii
 
       # TODO-neel-data map to exact ruby behaviour for backwards compat behavior
@@ -145,7 +146,6 @@ def self.backfill(configuration)
       data_collection.graphql.variables = false
       data_collection.database_query_data = false
       data_collection.queues = false
-      data_collection.stack_frame_variables = configuration.include_local_variables
       data_collection.frame_context_lines = configuration.context_lines
       data_collection
     end
@@ -162,10 +162,24 @@ def initialize
       @database_query_data = true
       @graphql = GraphQL.new(document: true, variables: true)
       @queues = true
-      @stack_frame_variables = false
+      self.stack_frame_variables = false
       @frame_context_lines = 3
     end
 
+    # Copies the legacy stack frame variable configuration.
+    def backfill_stack_frame_variables(configuration)
+      self.stack_frame_variables = configuration.include_local_variables
+    end
+
+    # Returns whether stack frame local variables should be captured.
+    def collect_stack_frame_variables?
+      stack_frame_variables.mode != :off
+    end
+
+    def stack_frame_variables=(value)
+      @stack_frame_variables = KeyValueCollection.from(value)
+    end
+
     def cookies=(value)
       @cookies = KeyValueCollection.from(value)
     end

sentry-ruby/lib/sentry/data_collection/key_value_collection.rb

@@ -73,14 +73,14 @@ def initialize(mode:, terms:)
       def mode=(mode)
         @mode = case mode
         when true then :deny_list
-        when false then :off
+        when false, nil then :off
         else mode
         end
       end
 
       # Converts the boolean shorthand into a collection configuration.
       def self.from(value)
-        return new(mode: value, terms: nil) if value.equal?(true) || value.equal?(false)
+        return new(mode: value, terms: nil) if value.equal?(true) || value.equal?(false) || value.nil?
 
         value
       end

sentry-ruby/lib/sentry/error_event.rb

@@ -32,7 +32,12 @@ def add_exception_interface(exception, mechanism:)
         @extra.merge!(exception.sentry_context)
       end
 
-      @exception = Sentry::ExceptionInterface.build(exception: exception, stacktrace_builder: @stacktrace_builder, mechanism: mechanism)
+      @exception = Sentry::ExceptionInterface.build(
+        exception: exception,
+        stacktrace_builder: @stacktrace_builder,
+        mechanism: mechanism,
+        data_collection: @data_collection
+      )
     end
   end
 end

sentry-ruby/lib/sentry/interfaces/exception.rb

@@ -25,15 +25,21 @@ def to_h
     # @see SingleExceptionInterface#build_with_stacktrace
     # @see SingleExceptionInterface#initialize
     # @param mechanism [Mechanism]
+    # @param data_collection [DataCollection]
     # @return [ExceptionInterface]
-    def self.build(exception:, stacktrace_builder:, mechanism:)
+    def self.build(exception:, stacktrace_builder:, mechanism:, data_collection:)
       exceptions = Sentry::Utils::ExceptionCauseChain.exception_to_array(exception).reverse
       processed_backtrace_ids = Set.new
 
       exceptions = exceptions.map do |e|
         if e.backtrace && !processed_backtrace_ids.include?(e.backtrace.object_id)
           processed_backtrace_ids << e.backtrace.object_id
-          SingleExceptionInterface.build_with_stacktrace(exception: e, stacktrace_builder: stacktrace_builder, mechanism: mechanism)
+          SingleExceptionInterface.build_with_stacktrace(
+            exception: e,
+            stacktrace_builder: stacktrace_builder,
+            mechanism: mechanism,
+            data_collection: data_collection
+          )
         else
           SingleExceptionInterface.new(exception: exception, mechanism: mechanism)
         end

sentry-ruby/lib/sentry/interfaces/single_exception.rb

@@ -41,7 +41,7 @@ def to_h
 
     # patch this method if you want to change an exception's stacktrace frames
     # also see `StacktraceBuilder.build`.
-    def self.build_with_stacktrace(exception:, stacktrace_builder:, mechanism:)
+    def self.build_with_stacktrace(exception:, stacktrace_builder:, mechanism:, data_collection:)
       stacktrace = stacktrace_builder.build(backtrace: exception.backtrace)
 
       if locals = exception.instance_variable_get(:@sentry_locals)
@@ -60,7 +60,7 @@ def self.build_with_stacktrace(exception:, stacktrace_builder:, mechanism:)
             end
         end
 
-        stacktrace.frames.last&.vars = locals
+        stacktrace.frames.last&.vars = data_collection.stack_frame_variables.filter(locals)
       end
 
       new(exception: exception, stacktrace: stacktrace, mechanism: mechanism)

sentry-ruby/lib/sentry/test_helper.rb

@@ -33,7 +33,7 @@ def setup_sentry_test(&block)
       dummy_config.background_worker_threads = 0
 
       # user can overwrite some of the configs, with a few exceptions like:
-      # - include_local_variables
+      # - data_collection
       # - auto_session_tracking
       block&.call(dummy_config)
 

sentry-ruby/spec/sentry/client_spec.rb

@@ -608,8 +608,9 @@ def sentry_context
 
       context "if local variable contains illegal/malformed encoding characters" do
         before do
+          configuration.data_collection.stack_frame_variables = true
           perform_basic_setup do |config|
-            config.include_local_variables = true
+            config.data_collection.stack_frame_variables = true
           end
         end
 

sentry-ruby/spec/sentry/configuration_spec.rb

@@ -10,6 +10,49 @@
 
       expect(configuration.data_collection.user_info).to eq(false)
     end
+
+    it "falls back to include_local_variables for stack frame variables" do
+      configuration = Sentry::Configuration.new do |config|
+        config.include_local_variables = true
+      end
+
+      expect(configuration.data_collection.stack_frame_variables.mode).to eq(:deny_list)
+    end
+
+    it "disables stack frame variables when include_local_variables is false" do
+      configuration = Sentry::Configuration.new do |config|
+        config.include_local_variables = false
+      end
+
+      expect(configuration.data_collection.stack_frame_variables.mode).to eq(:off)
+    end
+
+    it "treats nil include_local_variables as disabled" do
+      configuration = Sentry::Configuration.new do |config|
+        config.include_local_variables = nil
+      end
+
+      expect(configuration.data_collection.stack_frame_variables.mode).to eq(:off)
+    end
+
+    it "only backfills stack frame variables when include_local_variables changes" do
+      configuration = Sentry::Configuration.new do |config|
+        config.data_collection.user_info = true
+        config.include_local_variables = true
+      end
+
+      expect(configuration.data_collection.user_info).to eq(true)
+      expect(configuration.data_collection.stack_frame_variables.mode).to eq(:deny_list)
+    end
+
+    it "preserves include_local_variables when send_default_pii is set later" do
+      configuration = Sentry::Configuration.new do |config|
+        config.include_local_variables = true
+        config.send_default_pii = true
+      end
+
+      expect(configuration.data_collection.stack_frame_variables.mode).to eq(:deny_list)
+    end
   end
 
   describe "deprecated configuration warnings" do

sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb

@@ -25,6 +25,10 @@
       expect(described_class.from(false).mode).to eq(:off)
     end
 
+    it "maps nil to off mode" do
+      expect(described_class.from(nil).mode).to eq(:off)
+    end
+
     it "returns an existing collection unchanged" do
       expect(described_class.from(collection)).to be(collection)
     end
@@ -37,9 +41,11 @@
       expect(collection.mode).to eq(:deny_list)
     end
 
-    it "maps false to off mode" do
+    it "maps false and nil to off mode" do
       collection.mode = false
+      expect(collection.mode).to eq(:off)
 
+      collection.mode = nil
       expect(collection.mode).to eq(:off)
     end
   end

sentry-ruby/spec/sentry/data_collection_spec.rb

@@ -29,7 +29,7 @@
       expect(data_collection.graphql.document).to eq(false)
       expect(data_collection.graphql.variables).to eq(false)
       expect(data_collection.queues).to eq(false)
-      expect(data_collection.stack_frame_variables).to eq(false)
+      expect(data_collection.stack_frame_variables.mode).to eq(:off)
       expect(data_collection.frame_context_lines).to eq(3)
     end
 
@@ -85,7 +85,7 @@
       expect(data_collection.graphql.document).to eq(true)
       expect(data_collection.graphql.variables).to eq(true)
       expect(data_collection.queues).to eq(true)
-      expect(data_collection.stack_frame_variables).to eq(false)
+      expect(data_collection.stack_frame_variables.mode).to eq(:off)
       expect(data_collection.frame_context_lines).to eq(3)
     end
   end
@@ -116,11 +116,28 @@
       data_collection.http_headers.request = false
       data_collection.http_headers.response = true
       data_collection.url_query_params = false
+      data_collection.stack_frame_variables = false
 
       expect(data_collection.cookies.mode).to eq(:deny_list)
       expect(data_collection.http_headers.request.mode).to eq(:off)
       expect(data_collection.http_headers.response.mode).to eq(:deny_list)
       expect(data_collection.url_query_params.mode).to eq(:off)
+      expect(data_collection.stack_frame_variables.mode).to eq(:off)
+
+      data_collection.stack_frame_variables = true
+      expect(data_collection.stack_frame_variables.mode).to eq(:deny_list)
+    end
+
+    it "supports configuring stack frame variables by name" do
+      data_collection.stack_frame_variables.mode = :allow_list
+      data_collection.stack_frame_variables.terms = ["user"]
+
+      expect(data_collection.stack_frame_variables.mode).to eq(:allow_list)
+      expect(data_collection.stack_frame_variables.terms).to eq(["user"])
+    end
+
+    it "does not collect stack frame variables by default" do
+      expect(data_collection.collect_stack_frame_variables?).to eq(false)
     end
 
     it "supports configuring request and response headers independently" do

sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb

@@ -93,10 +93,10 @@
       expect(env.key?("sentry.error_event_id")).to eq(false)
     end
 
-    context "with config.include_local_variables = true" do
+    context "with config.data_collection.stack_frame_variables = true" do
       before do
         perform_basic_setup do |config|
-          config.include_local_variables = true
+          config.data_collection.stack_frame_variables = true
         end
       end
 
@@ -121,6 +121,43 @@
         expect(last_frame[:vars]).to include({ a: "1", b: "0" })
       end
 
+      it 'filters locals by name' do
+        perform_basic_setup do |config|
+          config.data_collection.stack_frame_variables.mode = :allow_list
+          config.data_collection.stack_frame_variables.terms = ["safe"]
+        end
+
+        app = ->(_e) do
+          safe = "visible"
+          foo = "bar"
+          raise "boom"
+        end
+
+        stack = Sentry::Rack::CaptureExceptions.new(app)
+
+        expect { stack.call(env) }.to raise_error(RuntimeError, "boom")
+
+        event = last_sentry_event.to_h
+        last_frame = event.dig(:exception, :values, 0, :stacktrace, :frames).last
+        expect(last_frame[:vars]).to include(safe: "visible", foo: "[Filtered]")
+      end
+
+      it 'filters locals matching the sensitive denylist by default' do
+        app = ->(_e) do
+          safe = "visible"
+          password = "filtered"
+          raise "boom"
+        end
+
+        stack = Sentry::Rack::CaptureExceptions.new(app)
+
+        expect { stack.call(env) }.to raise_error(RuntimeError, "boom")
+
+        event = last_sentry_event.to_h
+        last_frame = event.dig(:exception, :values, 0, :stacktrace, :frames).last
+        expect(last_frame[:vars]).to include(safe: "visible", password: "[Filtered]")
+      end
+
       it 'ignores problematic locals' do
         class Foo
           def inspect