From 089f8238d1c08d42e12374583f734c4d8532e7fd Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 8 Oct 2021 17:20:26 +0800 Subject: [PATCH 1/2] Connect Sidekiq's transaction with the previous one If there's a parent transaction on the client side, its sentry_trace should be passed and picked by the server side middleware, so the 2 transactions can connect. --- .../sidekiq/sentry_context_middleware.rb | 12 ++++- .../sidekiq/sentry_context_middleware_spec.rb | 49 ++++++++++++++++--- sentry-sidekiq/spec/spec_helper.rb | 3 +- 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb b/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb index 6fd9962ab..60e3be843 100644 --- a/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb +++ b/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb @@ -16,12 +16,12 @@ def call(_worker, job, queue) scope.set_tags(queue: queue, jid: job["jid"]) scope.set_contexts(sidekiq: job.merge("queue" => queue)) scope.set_transaction_name(context_filter.transaction_name) - transaction = Sentry.start_transaction(name: scope.transaction_name, op: "sidekiq") + transaction = start_transaction(scope.transaction_name, job["sentry_trace"]) scope.set_span(transaction) if transaction begin yield - rescue => e + rescue finish_transaction(transaction, 500) raise end @@ -32,6 +32,12 @@ def call(_worker, job, queue) scope.clear end + def start_transaction(transaction_name, sentry_trace) + options = { name: transaction_name, op: "sidekiq" } + transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, **options) if sentry_trace + Sentry.start_transaction(transaction: transaction, **options) + end + def finish_transaction(transaction, status) return unless transaction @@ -45,7 +51,9 @@ def call(_worker_class, job, _queue, _redis_pool) return yield unless Sentry.initialized? user = Sentry.get_current_scope.user + transaction = Sentry.get_current_scope.get_transaction job["sentry_user"] = user unless user.empty? + job["sentry_trace"] = transaction.to_sentry_trace if transaction yield end end diff --git a/sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb b/sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb index 5c2cdbc88..af4f1a674 100644 --- a/sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb +++ b/sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb @@ -51,6 +51,19 @@ event = transport.events.last expect(event.user).to eq(user) end + + context "with sentry_trace" do + let(:parent_transaction) { Sentry.start_transaction(op: "sidekiq") } + + it "starts the transaction from it" do + execute_worker(processor, HappyWorker, sentry_trace: parent_transaction.to_sentry_trace) + + expect(transport.events.count).to eq(1) + transaction = transport.events.first + expect(transaction).not_to be_nil + expect(transaction.contexts.dig(:trace, :trace_id)).to eq(parent_transaction.trace_id) + end + end end end @@ -70,22 +83,44 @@ before do queue.clear - perform_basic_setup + perform_basic_setup do |config| + config.traces_sample_rate = 1.0 + end end - it "does not add user to the job if user is absence in the current scope" do + it "does not add user or sentry_trace to the job if they're absence in the current scope" do client.push('queue' => 'default', 'class' => HappyWorker, 'args' => []) expect(queue.size).to be(1) expect(queue.first["sentry_user"]).to be_nil + expect(queue.first["sentry_trace"]).to be_nil end - it "sets user of the current scope to the job if present" do - Sentry.set_user(user) + describe "with user" do + before do + Sentry.set_user(user) + end - client.push('queue' => 'default', 'class' => HappyWorker, 'args' => []) + it "sets user of the current scope to the job" do + client.push('queue' => 'default', 'class' => HappyWorker, 'args' => []) - expect(queue.size).to be(1) - expect(queue.first["sentry_user"]).to eq(user) + expect(queue.size).to be(1) + expect(queue.first["sentry_user"]).to eq(user) + end + end + + describe "with transaction" do + let(:transaction) { Sentry.start_transaction(op: "sidekiq") } + + before do + Sentry.get_current_scope.set_span(transaction) + end + + it "sets user of the current scope to the job" do + client.push('queue' => 'default', 'class' => HappyWorker, 'args' => []) + + expect(queue.size).to be(1) + expect(queue.first["sentry_trace"]).to eq(transaction.to_sentry_trace) + end end end diff --git a/sentry-sidekiq/spec/spec_helper.rb b/sentry-sidekiq/spec/spec_helper.rb index 0e000c8dd..b353512f5 100644 --- a/sentry-sidekiq/spec/spec_helper.rb +++ b/sentry-sidekiq/spec/spec_helper.rb @@ -153,9 +153,8 @@ def perform end end -def execute_worker(processor, klass) +def execute_worker(processor, klass, **options) klass_options = klass.sidekiq_options_hash || {} - options = {} # for Ruby < 2.6 klass_options.each do |k, v| From e588ef24efada5e2d989714183b8c639cd986998 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sat, 9 Oct 2021 20:08:37 +0800 Subject: [PATCH 2/2] Update changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a19e646b2..52418f490 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,11 @@ - Start Testing Against Rails 7.0 [#1581](https://github.com/getsentry/sentry-ruby/pull/1581) - Tracing subscribers should be multi-event based [#1587](https://github.com/getsentry/sentry-ruby/pull/1587) +### Bug Fixes + +- Connect `Sidekiq`'s transaction with its parent when possible [#1590](https://github.com/getsentry/sentry-ruby/pull/1590) + - Fixes [#1586](https://github.com/getsentry/sentry-ruby/issues/1586) + ## 4.7.3 - Avoid leaking tracing timestamp to breadcrumbs [#1575](https://github.com/getsentry/sentry-ruby/pull/1575)