Fix sampling decision with sentry-trace and add more tests

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

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

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
 

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

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

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

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
 

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

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