From 4d8fa6e685ffe898a3b06703b3a96aa3025af7a2 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Wed, 3 Apr 2024 14:48:15 +0200 Subject: [PATCH] Add mechanism interface and default to handled false in integrations (#2280) --- CHANGELOG.md | 4 ++++ sentry-ruby/lib/sentry/client.rb | 3 ++- sentry-ruby/lib/sentry/error_event.rb | 4 ++-- sentry-ruby/lib/sentry/integrable.rb | 4 ++++ sentry-ruby/lib/sentry/interface.rb | 1 + .../lib/sentry/interfaces/exception.rb | 7 ++++--- .../lib/sentry/interfaces/mechanism.rb | 20 +++++++++++++++++++ .../lib/sentry/interfaces/single_exception.rb | 10 ++++++---- .../lib/sentry/rack/capture_exceptions.rb | 7 ++++++- sentry-ruby/lib/sentry/rake.rb | 4 +++- sentry-ruby/spec/sentry/client_spec.rb | 17 +++++++++++++++- sentry-ruby/spec/sentry/integrable_spec.rb | 8 +++++++- .../sentry/rack/capture_exceptions_spec.rb | 11 ++++++++++ sentry-ruby/spec/sentry_spec.rb | 19 ++++++++++++++++++ 14 files changed, 105 insertions(+), 14 deletions(-) create mode 100644 sentry-ruby/lib/sentry/interfaces/mechanism.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index ece53ce61..b5c9b9c86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## Unreleased +### Features + +- Add `Mechanism` interface and default to unhandled for integration exceptions [#2280](https://github.com/getsentry/sentry-ruby/pull/2280) + ### Bug Fixes - Don't instantiate connection in `ActiveRecordSubscriber` ([#2278](https://github.com/getsentry/sentry-ruby/pull/2278)) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index c9f96f600..d5e633739 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -88,9 +88,10 @@ def event_from_exception(exception, hint = {}) return if !ignore_exclusions && !@configuration.exception_class_allowed?(exception) integration_meta = Sentry.integrations[hint[:integration]] + mechanism = hint.delete(:mechanism) { Mechanism.new } ErrorEvent.new(configuration: configuration, integration_meta: integration_meta).tap do |event| - event.add_exception_interface(exception) + event.add_exception_interface(exception, mechanism: mechanism) event.add_threads_interface(crashed: true) event.level = :error end diff --git a/sentry-ruby/lib/sentry/error_event.rb b/sentry-ruby/lib/sentry/error_event.rb index 2ea52a2ec..13a891f71 100644 --- a/sentry-ruby/lib/sentry/error_event.rb +++ b/sentry-ruby/lib/sentry/error_event.rb @@ -27,12 +27,12 @@ def add_threads_interface(backtrace: nil, **options) end # @!visibility private - def add_exception_interface(exception) + def add_exception_interface(exception, mechanism:) if exception.respond_to?(:sentry_context) @extra.merge!(exception.sentry_context) end - @exception = Sentry::ExceptionInterface.build(exception: exception, stacktrace_builder: @stacktrace_builder) + @exception = Sentry::ExceptionInterface.build(exception: exception, stacktrace_builder: @stacktrace_builder, mechanism: mechanism) end end end diff --git a/sentry-ruby/lib/sentry/integrable.rb b/sentry-ruby/lib/sentry/integrable.rb index a1edeadb6..a45dd7839 100644 --- a/sentry-ruby/lib/sentry/integrable.rb +++ b/sentry-ruby/lib/sentry/integrable.rb @@ -14,6 +14,10 @@ def integration_name def capture_exception(exception, **options, &block) options[:hint] ||= {} options[:hint][:integration] = integration_name + + # within an integration, we usually intercept uncaught exceptions so we set handled to false. + options[:hint][:mechanism] ||= Sentry::Mechanism.new(type: integration_name, handled: false) + Sentry.capture_exception(exception, **options, &block) end diff --git a/sentry-ruby/lib/sentry/interface.rb b/sentry-ruby/lib/sentry/interface.rb index 2e6046053..d54d605b1 100644 --- a/sentry-ruby/lib/sentry/interface.rb +++ b/sentry-ruby/lib/sentry/interface.rb @@ -14,3 +14,4 @@ def to_hash require "sentry/interfaces/single_exception" require "sentry/interfaces/stacktrace" require "sentry/interfaces/threads" +require "sentry/interfaces/mechanism" diff --git a/sentry-ruby/lib/sentry/interfaces/exception.rb b/sentry-ruby/lib/sentry/interfaces/exception.rb index e90e70db4..58cb1a576 100644 --- a/sentry-ruby/lib/sentry/interfaces/exception.rb +++ b/sentry-ruby/lib/sentry/interfaces/exception.rb @@ -24,17 +24,18 @@ def to_hash # @param stacktrace_builder [StacktraceBuilder] # @see SingleExceptionInterface#build_with_stacktrace # @see SingleExceptionInterface#initialize + # @param mechanism [Mechanism] # @return [ExceptionInterface] - def self.build(exception:, stacktrace_builder:) + def self.build(exception:, stacktrace_builder:, mechanism:) 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) + SingleExceptionInterface.build_with_stacktrace(exception: e, stacktrace_builder: stacktrace_builder, mechanism: mechanism) else - SingleExceptionInterface.new(exception: exception) + SingleExceptionInterface.new(exception: exception, mechanism: mechanism) end end diff --git a/sentry-ruby/lib/sentry/interfaces/mechanism.rb b/sentry-ruby/lib/sentry/interfaces/mechanism.rb new file mode 100644 index 000000000..95179a77a --- /dev/null +++ b/sentry-ruby/lib/sentry/interfaces/mechanism.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Sentry + class Mechanism < Interface + # Generic identifier, mostly the source integration for this exception. + # @return [String] + attr_accessor :type + + # A manually captured exception has handled set to true, + # false if coming from an integration where we intercept an uncaught exception. + # Defaults to true here and will be set to false explicitly in integrations. + # @return [Boolean] + attr_accessor :handled + + def initialize(type: 'generic', handled: true) + @type = type + @handled = handled + end + end +end diff --git a/sentry-ruby/lib/sentry/interfaces/single_exception.rb b/sentry-ruby/lib/sentry/interfaces/single_exception.rb index 96217c077..d35491206 100644 --- a/sentry-ruby/lib/sentry/interfaces/single_exception.rb +++ b/sentry-ruby/lib/sentry/interfaces/single_exception.rb @@ -11,10 +11,10 @@ class SingleExceptionInterface < Interface OMISSION_MARK = "...".freeze MAX_LOCAL_BYTES = 1024 - attr_reader :type, :module, :thread_id, :stacktrace + attr_reader :type, :module, :thread_id, :stacktrace, :mechanism attr_accessor :value - def initialize(exception:, stacktrace: nil) + def initialize(exception:, mechanism:, stacktrace: nil) @type = exception.class.to_s exception_message = if exception.respond_to?(:detailed_message) @@ -29,17 +29,19 @@ def initialize(exception:, stacktrace: nil) @module = exception.class.to_s.split('::')[0...-1].join('::') @thread_id = Thread.current.object_id @stacktrace = stacktrace + @mechanism = mechanism end def to_hash data = super data[:stacktrace] = data[:stacktrace].to_hash if data[:stacktrace] + data[:mechanism] = data[:mechanism].to_hash data end # 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:) + def self.build_with_stacktrace(exception:, stacktrace_builder:, mechanism:) stacktrace = stacktrace_builder.build(backtrace: exception.backtrace) if locals = exception.instance_variable_get(:@sentry_locals) @@ -61,7 +63,7 @@ def self.build_with_stacktrace(exception:, stacktrace_builder:) stacktrace.frames.last.vars = locals end - new(exception: exception, stacktrace: stacktrace) + new(exception: exception, stacktrace: stacktrace, mechanism: mechanism) end end end diff --git a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb index 2248799a3..99a84a0fe 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb @@ -4,6 +4,7 @@ module Sentry module Rack class CaptureExceptions ERROR_EVENT_ID_KEY = "sentry.error_event_id" + MECHANISM_TYPE = "rack" def initialize(app) @app = app @@ -56,7 +57,7 @@ def transaction_op end def capture_exception(exception, env) - Sentry.capture_exception(exception).tap do |event| + Sentry.capture_exception(exception, hint: { mechanism: mechanism }).tap do |event| env[ERROR_EVENT_ID_KEY] = event.event_id if event end end @@ -74,6 +75,10 @@ def finish_transaction(transaction, status_code) transaction.set_http_status(status_code) transaction.finish end + + def mechanism + Sentry::Mechanism.new(type: MECHANISM_TYPE, handled: false) + end end end end diff --git a/sentry-ruby/lib/sentry/rake.rb b/sentry-ruby/lib/sentry/rake.rb index b6f4d0fcb..0e3e6e8c7 100644 --- a/sentry-ruby/lib/sentry/rake.rb +++ b/sentry-ruby/lib/sentry/rake.rb @@ -8,7 +8,9 @@ module Rake module Application # @api private def display_error_message(ex) - Sentry.capture_exception(ex) do |scope| + mechanism = Sentry::Mechanism.new(type: 'rake', handled: false) + + Sentry.capture_exception(ex, hint: { mechanism: mechanism }) do |scope| task_name = top_level_tasks.join(' ') scope.set_transaction_name(task_name, source: :task) scope.set_tag("rake_task", task_name) diff --git a/sentry-ruby/spec/sentry/client_spec.rb b/sentry-ruby/spec/sentry/client_spec.rb index 5f95cf831..4d98b456e 100644 --- a/sentry-ruby/spec/sentry/client_spec.rb +++ b/sentry-ruby/spec/sentry/client_spec.rb @@ -395,7 +395,7 @@ module ExcTag; end context "when exclusions overridden with :ignore_exclusions" do it 'returns Sentry::ErrorEvent' do config.excluded_exceptions << Sentry::Test::BaseExc - expect(subject.event_from_exception(Sentry::Test::BaseExc.new, ignore_exclusions: true)).to be_a(Sentry::ErrorEvent) + expect(subject.event_from_exception(Sentry::Test::BaseExc.new, { ignore_exclusions: true })).to be_a(Sentry::ErrorEvent) end end end @@ -565,6 +565,21 @@ module ExcTag; end end end end + + describe 'mechanism' do + it 'has type generic and handled true by default' do + mechanism = hash[:exception][:values][0][:mechanism] + expect(mechanism).to eq({ type: 'generic', handled: true }) + end + + it 'has correct custom mechanism when passed' do + mech = Sentry::Mechanism.new(type: 'custom', handled: false) + event = subject.event_from_exception(exception, mechanism: mech) + hash = event.to_hash + mechanism = hash[:exception][:values][0][:mechanism] + expect(mechanism).to eq({ type: 'custom', handled: false }) + end + end end describe "#event_from_check_in" do diff --git a/sentry-ruby/spec/sentry/integrable_spec.rb b/sentry-ruby/spec/sentry/integrable_spec.rb index 5dc18d045..269ba821e 100644 --- a/sentry-ruby/spec/sentry/integrable_spec.rb +++ b/sentry-ruby/spec/sentry/integrable_spec.rb @@ -48,15 +48,21 @@ module AnotherIntegration; end it "generates Sentry::FakeIntegration.capture_exception" do hint = nil + event = nil - Sentry.configuration.before_send = lambda do |event, h| + Sentry.configuration.before_send = lambda do |e, h| hint = h + event = e event end Sentry::FakeIntegration.capture_exception(exception, hint: { additional_hint: "foo" }) expect(hint).to eq({ additional_hint: "foo", integration: "fake_integration", exception: exception }) + + mechanism = event.exception.values.first.mechanism + expect(mechanism.type).to eq('fake_integration') + expect(mechanism.handled).to eq(false) end it "generates Sentry::FakeIntegration.capture_message" do diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index 6ff81eaf5..c5751bba0 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb @@ -25,6 +25,17 @@ expect(last_frame[:vars]).to eq(nil) end + it 'has the correct mechanism' do + app = ->(_e) { raise exception } + stack = described_class.new(app) + + expect { stack.call(env) }.to raise_error(ZeroDivisionError) + + event = last_sentry_event.to_hash + mechanism = event.dig(:exception, :values, 0, :mechanism) + expect(mechanism).to eq({ type: 'rack', handled: false }) + end + it 'captures the exception from rack.exception' do app = lambda do |e| e['rack.exception'] = exception diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index 2fe6fa8f6..976ad213b 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -252,6 +252,25 @@ end.to change { sentry_events.count }.by(1) end + describe 'mechanism' do + it 'has default mechanism' do + event = described_class.capture_exception(exception) + mechanism = event.exception.values.first.mechanism + expect(mechanism).to be_a(Sentry::Mechanism) + expect(mechanism.type).to eq('generic') + expect(mechanism.handled).to eq(true) + end + + it 'has custom mechanism if passed' do + mech = Sentry::Mechanism.new(type: 'custom', handled: false) + event = described_class.capture_exception(exception, hint: { mechanism: mech }) + mechanism = event.exception.values.first.mechanism + expect(mechanism).to be_a(Sentry::Mechanism) + expect(mechanism.type).to eq('custom') + expect(mechanism.handled).to eq(false) + end + end + context "with include_local_variables = false (default)" do it "doens't capture local variables" do begin