diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 3d32ea624..c8d64f473 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -21,18 +21,16 @@ def self.included(base) def capture_and_reraise_with_sentry(job, scope, block) scope.set_transaction_name(job.class.name) - span = Sentry.start_transaction(name: scope.transaction_name, op: "active_job") + transaction = Sentry.start_transaction(name: scope.transaction_name, op: "active_job") - scope.set_span(span) + scope.set_span(transaction) if transaction block.call - span.set_http_status(200) - span.finish + finish_transaction(transaction, 200) rescue Exception => e # rubocop:disable Lint/RescueException rescue_handler_result = rescue_with_handler(e) - span.set_http_status(500) - span.finish + finish_transaction(transaction, 500) return rescue_handler_result if rescue_handler_result Sentry::Rails.capture_exception( @@ -46,6 +44,13 @@ def capture_and_reraise_with_sentry(job, scope, block) raise e end + def finish_transaction(transaction, status) + return unless transaction + + transaction.set_http_status(status) + transaction.finish + end + def already_supported_by_specific_integration?(job) Sentry.configuration.rails.skippable_job_adapters.include?(job.class.queue_adapter.class.to_s) end diff --git a/sentry-rails/lib/sentry/rails/capture_exceptions.rb b/sentry-rails/lib/sentry/rails/capture_exceptions.rb index c45730c91..86dd4fe00 100644 --- a/sentry-rails/lib/sentry/rails/capture_exceptions.rb +++ b/sentry-rails/lib/sentry/rails/capture_exceptions.rb @@ -32,6 +32,8 @@ def capture_exception(exception) def start_transaction(env, scope) transaction = super + return unless transaction + if @assets_regex && transaction.name.match?(@assets_regex) transaction.instance_variable_set(:@sampled, false) end diff --git a/sentry-ruby/CHANGELOG.md b/sentry-ruby/CHANGELOG.md index 2f4653ed8..8aa8a49a9 100644 --- a/sentry-ruby/CHANGELOG.md +++ b/sentry-ruby/CHANGELOG.md @@ -29,6 +29,7 @@ config.max_breadcrumbs = 10 - Return nil from logger methods instead of breadcrumb buffer [#1299](https://github.com/getsentry/sentry-ruby/pull/1299) - Exceptions with nil message shouldn't cause issues [#1327](https://github.com/getsentry/sentry-ruby/pull/1327) - Fixes [#1323](https://github.com/getsentry/sentry-ruby/issues/1323) +- Fix sampling decision with sentry-trace and add more tests [#1326](https://github.com/getsentry/sentry-ruby/pull/1326) ## 4.2.2 diff --git a/sentry-ruby/lib/sentry/hub.rb b/sentry-ruby/lib/sentry/hub.rb index e86ece913..1dccc3733 100644 --- a/sentry-ruby/lib/sentry/hub.rb +++ b/sentry-ruby/lib/sentry/hub.rb @@ -70,6 +70,8 @@ def pop_scope end def start_transaction(transaction: nil, configuration: Sentry.configuration, **options) + return unless configuration.tracing_enabled? + transaction ||= Transaction.new(**options) transaction.set_initial_sample_decision(configuration: current_client.configuration) transaction diff --git a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb index 5d8b5afbb..3b7ce2e04 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb @@ -17,7 +17,7 @@ def call(env) scope.set_rack_env(env) transaction = start_transaction(env, scope) - scope.set_span(transaction) + scope.set_span(transaction) if transaction begin response = @app.call(env) @@ -55,12 +55,15 @@ def capture_exception(exception) def start_transaction(env, scope) sentry_trace = env["HTTP_SENTRY_TRACE"] - transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, name: scope.transaction_name, op: transaction_op) if sentry_trace - transaction || Sentry.start_transaction(name: scope.transaction_name, op: transaction_op) + options = { name: scope.transaction_name, op: transaction_op } + transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, **options) if sentry_trace + Sentry.start_transaction(transaction: transaction, **options) end def finish_transaction(transaction, status_code) + return unless transaction + transaction.set_http_status(status_code) transaction.finish end diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index 8d22323a5..bff2975b0 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -33,7 +33,12 @@ def self.from_sentry_trace(sentry_trace, configuration: Sentry.configuration, ** return if match.nil? trace_id, parent_span_id, sampled_flag = match[1..3] - sampled = sampled_flag != "0" + sampled = + if sampled_flag.nil? + nil + else + sampled_flag != "0" + end new(trace_id: trace_id, parent_span_id: parent_span_id, parent_sampled: sampled, sampled: sampled, **options) end diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index 3a011e0a7..6b5adb3b5 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb @@ -173,40 +173,111 @@ ) end - before do - allow(Random).to receive(:rand).and_return(0.4) - end - - it "inherits trace info from the transaction" do - env["HTTP_SENTRY_TRACE"] = external_transaction.to_sentry_trace - - stack.call(env) - - transaction = transport.events.last + def verify_transaction_attributes(transaction) expect(transaction.type).to eq("transaction") expect(transaction.timestamp).not_to be_nil expect(transaction.contexts.dig(:trace, :status)).to eq("ok") expect(transaction.contexts.dig(:trace, :op)).to eq("rack.request") expect(transaction.spans.count).to eq(0) + end - # should inherit information from the external_transaction + def verify_transaction_inherits_external_transaction(transaction, external_transaction) expect(transaction.contexts.dig(:trace, :trace_id)).to eq(external_transaction.trace_id) expect(transaction.contexts.dig(:trace, :parent_span_id)).to eq(external_transaction.span_id) - expect(transaction.contexts.dig(:trace, :span_id)).not_to eq(external_transaction.span_id) end - it "safely handles bugus header values" do - env["HTTP_SENTRY_TRACE"] = 'null' + def verify_transaction_doesnt_inherit_external_transaction(transaction, external_transaction) + expect(transaction.contexts.dig(:trace, :trace_id)).not_to eq(external_transaction.trace_id) + expect(transaction.contexts.dig(:trace, :parent_span_id)).not_to eq(external_transaction.span_id) + end - stack.call(env) + def wont_be_sampled_by_sdk + allow(Random).to receive(:rand).and_return(1.0) + end - # creates a new transaction - transaction = transport.events.last - expect(transaction.type).to eq("transaction") - expect(transaction.timestamp).not_to be_nil - expect(transaction.contexts.dig(:trace, :status)).to eq("ok") - expect(transaction.contexts.dig(:trace, :op)).to eq("rack.request") - expect(transaction.spans.count).to eq(0) + def will_be_sampled_by_sdk + allow(Random).to receive(:rand).and_return(0.3) + end + + before do + env["HTTP_SENTRY_TRACE"] = trace + end + + let(:transaction) do + transport.events.last + end + + context "with sampled trace" do + let(:trace) do + "#{external_transaction.trace_id}-#{external_transaction.span_id}-1" + end + + it "inherits trace info and sampled decision from the trace and ignores later sampling" do + wont_be_sampled_by_sdk + + stack.call(env) + + verify_transaction_attributes(transaction) + verify_transaction_inherits_external_transaction(transaction, external_transaction) + end + end + + context "with unsampled trace" do + let(:trace) do + "#{external_transaction.trace_id}-#{external_transaction.span_id}-0" + end + + it "doesn't sample any transaction" do + will_be_sampled_by_sdk + + stack.call(env) + + expect(transaction).to be_nil + end + end + + context "with trace that has no sampling bit" do + let(:trace) do + "#{external_transaction.trace_id}-#{external_transaction.span_id}-" + end + + it "inherits trace info but not the sampling decision (later sampled)" do + will_be_sampled_by_sdk + + stack.call(env) + + verify_transaction_attributes(transaction) + verify_transaction_inherits_external_transaction(transaction, external_transaction) + end + + it "inherits trace info but not the sampling decision (later unsampled)" do + wont_be_sampled_by_sdk + + stack.call(env) + + expect(transaction).to eq(nil) + end + end + + context "with bugus trace" do + let(:trace) { "null" } + + it "starts a new transaction and follows SDK sampling decision (sampled)" do + will_be_sampled_by_sdk + + stack.call(env) + + verify_transaction_attributes(transaction) + verify_transaction_doesnt_inherit_external_transaction(transaction, external_transaction) + end + + it "starts a new transaction and follows SDK sampling decision (unsampled)" do + wont_be_sampled_by_sdk + + stack.call(env) + + expect(transaction).to eq(nil) + end end end diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index f3d70a979..8910163c1 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -178,19 +178,31 @@ end describe ".start_transaction" do - it "starts a new transaction" do - transaction = described_class.start_transaction(op: "foo") - expect(transaction).to be_a(Sentry::Transaction) - expect(transaction.op).to eq("foo") - end + context "when tracing is enabled" do + before do + Sentry.configuration.traces_sample_rate = 1.0 + end + + it "starts a new transaction" do + transaction = described_class.start_transaction(op: "foo") + expect(transaction).to be_a(Sentry::Transaction) + expect(transaction.op).to eq("foo") + end - context "when given an transaction object" do - it "adds sample decision to it" do - transaction = Sentry::Transaction.new + context "when given an transaction object" do + it "adds sample decision to it" do + transaction = Sentry::Transaction.new - described_class.start_transaction(transaction: transaction) + described_class.start_transaction(transaction: transaction) + + expect(transaction.sampled).to eq(true) + end + end + end - expect(transaction.sampled).to eq(false) + context "when tracing is disabled" do + it "returns nil" do + expect(described_class.start_transaction(op: "foo")).to eq(nil) end end end diff --git a/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb b/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb index 66a4f251a..941f866b4 100644 --- a/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb +++ b/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb @@ -14,22 +14,27 @@ def call(_worker, job, queue) scope.set_extras(sidekiq: job.merge("queue" => queue)) scope.set_transaction_name(context_filter.transaction_name) transaction = Sentry.start_transaction(name: scope.transaction_name, op: "sidekiq") - scope.set_span(transaction) + scope.set_span(transaction) if transaction begin yield rescue => e - transaction.set_http_status(500) - transaction.finish + finish_transaction(transaction, 500) raise end - transaction.set_http_status(200) - transaction.finish + finish_transaction(transaction, 200) # don't need to use ensure here # if the job failed, we need to keep the scope for error handler. and the scope will be cleared there scope.clear end + + def finish_transaction(transaction, status) + return unless transaction + + transaction.set_http_status(status) + transaction.finish + end end end end