From 11ae1a636da9d2208b369305d4595303274e676d Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 12 Jul 2022 15:05:49 +0200 Subject: [PATCH 1/7] Implement close API that cleans up state and reverses init logic The only logic not reversed is `apply_patches` which is fine since they already check if the patch is already applied and also no-op if `initialized?` is false. --- sentry-ruby/lib/sentry-ruby.rb | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/sentry-ruby/lib/sentry-ruby.rb b/sentry-ruby/lib/sentry-ruby.rb index 01cda0623..d6ebb8175 100644 --- a/sentry-ruby/lib/sentry-ruby.rb +++ b/sentry-ruby/lib/sentry-ruby.rb @@ -60,11 +60,11 @@ def exception_locals_tp end # @!attribute [rw] background_worker - # @return [BackgroundWorker] + # @return [BackgroundWorker, nil] attr_accessor :background_worker # @!attribute [r] session_flusher - # @return [SessionFlusher] + # @return [SessionFlusher, nil] attr_reader :session_flusher ##### Patch Registration ##### @@ -215,8 +215,31 @@ def init(&block) at_exit do @session_flusher&.kill + @background_worker&.shutdown + end + end + + # Flushes pending events and cleans up SDK state. + # SDK will stop sending events and all top-level APIs will be no-ops after this. + # + # @return [void] + def close + if @background_worker @background_worker.shutdown + @background_worker = nil + end + + if @session_flusher + @session_flusher.kill + @session_flusher = nil + end + + if config.capture_exception_frame_locals + exception_locals_tp.disable end + + @main_hub = nil + Thread.current.thread_variable_set(THREAD_LOCAL, nil) end # Returns true if the SDK is initialized. @@ -287,6 +310,7 @@ def get_current_scope # # @return [void] def clone_hub_to_current_thread + return unless initialized? Thread.current.thread_variable_set(THREAD_LOCAL, get_main_hub.clone) end From 80f0d949800bb59dfb61242907e19a43fd9b9bfe Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Wed, 13 Jul 2022 16:06:27 +0200 Subject: [PATCH 2/7] Specs --- sentry-ruby/lib/sentry-ruby.rb | 2 +- sentry-ruby/spec/sentry_spec.rb | 139 ++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/sentry-ruby/lib/sentry-ruby.rb b/sentry-ruby/lib/sentry-ruby.rb index d6ebb8175..43751c146 100644 --- a/sentry-ruby/lib/sentry-ruby.rb +++ b/sentry-ruby/lib/sentry-ruby.rb @@ -234,7 +234,7 @@ def close @session_flusher = nil end - if config.capture_exception_frame_locals + if configuration.capture_exception_frame_locals exception_locals_tp.disable end diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index 504051438..ddaa5b590 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -784,4 +784,143 @@ end end + describe ".close" do + context "when closing initialized SDK" do + before do + perform_basic_setup do |config| + config.capture_exception_frame_locals = true + end + end + + it "not initialized?" do + expect(described_class.initialized?).to eq(true) + described_class.close + expect(described_class.initialized?).to eq(false) + end + + it "removes main hub" do + expect(described_class.get_main_hub).to be_a(Sentry::Hub) + described_class.close + expect(described_class.get_main_hub).to eq(nil) + end + + it "removes thread local" do + expect(Thread.current.thread_variable_get(described_class::THREAD_LOCAL)).to be_a(Sentry::Hub) + described_class.close + expect(Thread.current.thread_variable_get(described_class::THREAD_LOCAL)).to eq(nil) + + end + + it "calls background worker shutdown" do + expect(described_class.background_worker).to receive(:shutdown) + described_class.close + expect(described_class.background_worker).to eq(nil) + end + + it "kills session flusher" do + expect(described_class.session_flusher).to receive(:kill) + described_class.close + expect(described_class.session_flusher).to eq(nil) + end + + it "disables Tracepoint" do + expect(described_class.exception_locals_tp).to receive(:disable) + described_class.close + end + + it "no-ops top level API getters" do + described_class.close + expect(described_class.configuration).to eq(nil) + expect(described_class.csp_report_uri).to eq(nil) + expect(described_class.get_main_hub).to eq(nil) + expect(described_class.get_current_hub).to eq(nil) + expect(described_class.get_current_client).to eq(nil) + expect(described_class.get_current_scope).to eq(nil) + expect(described_class.last_event_id).to eq(nil) + end + + it "no-ops top level API actions" do + described_class.close + + expect_any_instance_of(Sentry::DummyTransport).not_to receive(:send_event) + expect_any_instance_of(Sentry::DummyTransport).not_to receive(:send_envelope) + + described_class.send_event(event) + described_class.capture_event(event) + described_class.capture_message("test") + + exception = ZeroDivisionError.new("divided by 0") + described_class.capture_exception(exception) + expect(described_class.exception_captured?(exception)).to eq(false) + + expect do + described_class.with_exception_captured { 1 / 0 } + end.to raise_error(ZeroDivisionError) + + expect_any_instance_of(Sentry::Scope).not_to receive(:set_tags) + described_class.set_tags(foo: "bar") + + expect_any_instance_of(Sentry::Scope).not_to receive(:set_extras) + described_class.set_extras(foo: "bar") + + expect_any_instance_of(Sentry::Scope).not_to receive(:set_user) + described_class.set_user(id: 1) + + expect_any_instance_of(Sentry::Scope).not_to receive(:set_context) + described_class.set_context("character", { name: "John", age: 25 }) + + expect_any_instance_of(Sentry::Scope).not_to receive(:add_breadcrumb) + crumb = Sentry::Breadcrumb.new(message: "foo") + described_class.add_breadcrumb(crumb) + + expect_any_instance_of(Sentry::Hub).not_to receive(:configure_scope) + described_class.configure_scope do |scope| + scope.set_tags(foo: "bar") + end + + expect_any_instance_of(Sentry::Hub).not_to receive(:with_scope) + described_class.configure_scope do |scope| + scope.set_tags(foo: "bar") + Sentry.capture_message("test message") + end + + expect_any_instance_of(Sentry::Hub).not_to receive(:with_session_tracking) + described_class.with_session_tracking do + 1 + 1 + end + + expect_any_instance_of(Sentry::Hub).not_to receive(:start_transaction) + described_class.start_transaction(name: "foobar") + + expect_any_instance_of(Sentry::Span).not_to receive(:with_child_span) + described_class.with_child_span do |child_span| + 1 + 1 + end + end + end + + it "can reinitialize closed SDK" do + perform_basic_setup + + expect do + described_class.capture_event(event) + end.to change { transport.events.count }.by(1) + + described_class.close + + expect do + described_class.capture_event(event) + end.to change { transport.events.count }.by(0) + + perform_basic_setup + + expect(described_class.initialized?).to eq(true) + + new_transport = described_class.get_current_client.transport + + expect do + described_class.capture_event(event) + end.to change { new_transport.events.count }.by(1) + end + end end From 09e45614c96f97429c85f5e4e0c001c41d172082 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Wed, 13 Jul 2022 16:14:37 +0200 Subject: [PATCH 3/7] Fix Tracepoint test --- sentry-ruby/spec/sentry_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index ddaa5b590..749febe19 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -786,12 +786,6 @@ describe ".close" do context "when closing initialized SDK" do - before do - perform_basic_setup do |config| - config.capture_exception_frame_locals = true - end - end - it "not initialized?" do expect(described_class.initialized?).to eq(true) described_class.close @@ -824,6 +818,10 @@ end it "disables Tracepoint" do + perform_basic_setup do |config| + config.capture_exception_frame_locals = true + end + expect(described_class.exception_locals_tp).to receive(:disable) described_class.close end From 5ee483d2451687ac295d6a50c1db71b952de51e8 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 18 Jul 2022 14:37:42 +0200 Subject: [PATCH 4/7] Remove no-ops and fix tracepoint receive --- sentry-ruby/spec/sentry_spec.rb | 72 +-------------------------------- 1 file changed, 1 insertion(+), 71 deletions(-) diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index 749febe19..890ca3693 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -822,79 +822,9 @@ config.capture_exception_frame_locals = true end - expect(described_class.exception_locals_tp).to receive(:disable) + expect(described_class.exception_locals_tp).to receive(:disable).and_call_original described_class.close end - - it "no-ops top level API getters" do - described_class.close - expect(described_class.configuration).to eq(nil) - expect(described_class.csp_report_uri).to eq(nil) - expect(described_class.get_main_hub).to eq(nil) - expect(described_class.get_current_hub).to eq(nil) - expect(described_class.get_current_client).to eq(nil) - expect(described_class.get_current_scope).to eq(nil) - expect(described_class.last_event_id).to eq(nil) - end - - it "no-ops top level API actions" do - described_class.close - - expect_any_instance_of(Sentry::DummyTransport).not_to receive(:send_event) - expect_any_instance_of(Sentry::DummyTransport).not_to receive(:send_envelope) - - described_class.send_event(event) - described_class.capture_event(event) - described_class.capture_message("test") - - exception = ZeroDivisionError.new("divided by 0") - described_class.capture_exception(exception) - expect(described_class.exception_captured?(exception)).to eq(false) - - expect do - described_class.with_exception_captured { 1 / 0 } - end.to raise_error(ZeroDivisionError) - - expect_any_instance_of(Sentry::Scope).not_to receive(:set_tags) - described_class.set_tags(foo: "bar") - - expect_any_instance_of(Sentry::Scope).not_to receive(:set_extras) - described_class.set_extras(foo: "bar") - - expect_any_instance_of(Sentry::Scope).not_to receive(:set_user) - described_class.set_user(id: 1) - - expect_any_instance_of(Sentry::Scope).not_to receive(:set_context) - described_class.set_context("character", { name: "John", age: 25 }) - - expect_any_instance_of(Sentry::Scope).not_to receive(:add_breadcrumb) - crumb = Sentry::Breadcrumb.new(message: "foo") - described_class.add_breadcrumb(crumb) - - expect_any_instance_of(Sentry::Hub).not_to receive(:configure_scope) - described_class.configure_scope do |scope| - scope.set_tags(foo: "bar") - end - - expect_any_instance_of(Sentry::Hub).not_to receive(:with_scope) - described_class.configure_scope do |scope| - scope.set_tags(foo: "bar") - Sentry.capture_message("test message") - end - - expect_any_instance_of(Sentry::Hub).not_to receive(:with_session_tracking) - described_class.with_session_tracking do - 1 + 1 - end - - expect_any_instance_of(Sentry::Hub).not_to receive(:start_transaction) - described_class.start_transaction(name: "foobar") - - expect_any_instance_of(Sentry::Span).not_to receive(:with_child_span) - described_class.with_child_span do |child_span| - 1 + 1 - end - end end it "can reinitialize closed SDK" do From fb4fa9b81f2aa97ab0d87b2a3f3d6f5f50ce5a2f Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 18 Jul 2022 14:47:44 +0200 Subject: [PATCH 5/7] changelog --- sentry-ruby/CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sentry-ruby/CHANGELOG.md b/sentry-ruby/CHANGELOG.md index 74a145f65..eefa62f3a 100644 --- a/sentry-ruby/CHANGELOG.md +++ b/sentry-ruby/CHANGELOG.md @@ -2,6 +2,15 @@ Individual gem's changelog has been deprecated. Please check the [project changelog](https://github.com/getsentry/sentry-ruby/blob/master/CHANGELOG.md). +## Unreleased + +### Features + +- Expose `:values` in `ExceptionInterface`, so that it can be accessed in `before_send` under `event.exception.values` [#1843](https://github.com/getsentry/sentry-ruby/pull/1843) +- Add top level `Sentry.close` API [#1844](https://github.com/getsentry/sentry-ruby/pull/1844) + - Cleans up SDK state and sets it to uninitialized + - No-ops all SDK APIs and also disables the transport layer, so nothing will be sent to Sentry after closing the SDK + ## 4.4.2 - Fix NoMethodError when SDK's dsn is nil [#1433](https://github.com/getsentry/sentry-ruby/pull/1433) From b3bdc483084e50fb0474b0afd4c32a495764c6a1 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 19 Jul 2022 11:44:02 +0200 Subject: [PATCH 6/7] Fix changelog --- CHANGELOG.md | 6 ++++++ sentry-ruby/CHANGELOG.md | 9 --------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b80c45c5..88a393032 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ### Features +- Expose `:values` in `ExceptionInterface`, so that it can be accessed in `before_send` under `event.exception.values` [#1843](https://github.com/getsentry/sentry-ruby/pull/1843) + +- Add top level `Sentry.close` API [#1844](https://github.com/getsentry/sentry-ruby/pull/1844) + - Cleans up SDK state and sets it to uninitialized + - No-ops all SDK APIs and also disables the transport layer, so nothing will be sent to Sentry after closing the SDK + - Handle exception with large stacktrace without dropping entire item [#1807](https://github.com/getsentry/sentry-ruby/pull/1807) - Capture Rails runner's exceptions before exiting [#1820](https://github.com/getsentry/sentry-ruby/pull/1820) diff --git a/sentry-ruby/CHANGELOG.md b/sentry-ruby/CHANGELOG.md index eefa62f3a..74a145f65 100644 --- a/sentry-ruby/CHANGELOG.md +++ b/sentry-ruby/CHANGELOG.md @@ -2,15 +2,6 @@ Individual gem's changelog has been deprecated. Please check the [project changelog](https://github.com/getsentry/sentry-ruby/blob/master/CHANGELOG.md). -## Unreleased - -### Features - -- Expose `:values` in `ExceptionInterface`, so that it can be accessed in `before_send` under `event.exception.values` [#1843](https://github.com/getsentry/sentry-ruby/pull/1843) -- Add top level `Sentry.close` API [#1844](https://github.com/getsentry/sentry-ruby/pull/1844) - - Cleans up SDK state and sets it to uninitialized - - No-ops all SDK APIs and also disables the transport layer, so nothing will be sent to Sentry after closing the SDK - ## 4.4.2 - Fix NoMethodError when SDK's dsn is nil [#1433](https://github.com/getsentry/sentry-ruby/pull/1433) From eb48be25778f0a88ec309f6fabce3f0791a22036 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 19 Jul 2022 11:59:14 +0200 Subject: [PATCH 7/7] Call close on at_exit --- sentry-ruby/lib/sentry-ruby.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/sentry-ruby/lib/sentry-ruby.rb b/sentry-ruby/lib/sentry-ruby.rb index 43751c146..1bd3c092e 100644 --- a/sentry-ruby/lib/sentry-ruby.rb +++ b/sentry-ruby/lib/sentry-ruby.rb @@ -213,10 +213,7 @@ def init(&block) exception_locals_tp.enable end - at_exit do - @session_flusher&.kill - @background_worker&.shutdown - end + at_exit { close } end # Flushes pending events and cleans up SDK state. @@ -234,7 +231,7 @@ def close @session_flusher = nil end - if configuration.capture_exception_frame_locals + if configuration&.capture_exception_frame_locals exception_locals_tp.disable end