Merge branch 'master' into fix-#1301

sentry-rails/lib/sentry/rails/capture_exceptions.rb

@@ -29,11 +29,14 @@ def capture_exception(exception)
         Sentry::Rails.capture_exception(exception)
       end
 
-      def finish_span(span, status_code)
-        if @assets_regex.nil? || !span.name.match?(@assets_regex)
-          span.set_http_status(status_code)
-          span.finish
+      def start_transaction(env, scope)
+        transaction = super
+
+        if @assets_regex && transaction.name.match?(@assets_regex)
+          transaction.instance_variable_set(:@sampled, false)
         end
+
+        transaction
       end
     end
   end

sentry-ruby/CHANGELOG.md

@@ -5,6 +5,9 @@
 - Refactor interface construction [#1296](https://github.com/getsentry/sentry-ruby/pull/1296)
 - Treat query string as pii too [#1302](https://github.com/getsentry/sentry-ruby/pull/1302)
   - Fixes [#1301](https://github.com/getsentry/sentry-ruby/issues/1301)
+- Ignore sentry-trace when tracing is not enabled [#1308](https://github.com/getsentry/sentry-ruby/pull/1308)
+  - Fixes [#1307](https://github.com/getsentry/sentry-ruby/issues/1307)
+- Refactor tracing implementation [#1309](https://github.com/getsentry/sentry-ruby/pull/1309)
 
 ## 4.2.2
 

sentry-ruby/lib/sentry/hub.rb

@@ -69,9 +69,9 @@ def pop_scope
       @stack.pop
     end
 
-    def start_transaction(transaction: nil, **options)
+    def start_transaction(transaction: nil, configuration: Sentry.configuration, **options)
       transaction ||= Transaction.new(**options)
-      transaction.set_initial_sample_desicion
+      transaction.set_initial_sample_decision(configuration: current_client.configuration)
       transaction
     end
 

sentry-ruby/lib/sentry/rack/capture_exceptions.rb

@@ -16,27 +16,24 @@ def call(env)
           scope.set_transaction_name(env["PATH_INFO"]) if env["PATH_INFO"]
           scope.set_rack_env(env)
 
-          sentry_trace = env["HTTP_SENTRY_TRACE"]
-          span = Sentry::Transaction.from_sentry_trace(sentry_trace, name: scope.transaction_name, op: transaction_op) if sentry_trace
-          span ||= Sentry.start_transaction(name: scope.transaction_name, op: transaction_op)
-
-          scope.set_span(span)
+          transaction = start_transaction(env, scope)
+          scope.set_span(transaction)
 
           begin
             response = @app.call(env)
           rescue Sentry::Error
-            finish_span(span, 500)
+            finish_transaction(transaction, 500)
             raise # Don't capture Sentry errors
           rescue Exception => e
             capture_exception(e)
-            finish_span(span, 500)
+            finish_transaction(transaction, 500)
             raise
           end
 
           exception = collect_exception(env)
           capture_exception(exception) if exception
 
-          finish_span(span, response[0])
+          finish_transaction(transaction, response[0])
 
           response
         end
@@ -56,9 +53,16 @@ def capture_exception(exception)
         Sentry.capture_exception(exception)
       end
 
-      def finish_span(span, status_code)
-        span.set_http_status(status_code)
-        span.finish
+      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)
+      end
+
+
+      def finish_transaction(transaction, status_code)
+        transaction.set_http_status(status_code)
+        transaction.finish
       end
     end
   end

sentry-ruby/lib/sentry/transaction.rb

@@ -25,7 +25,8 @@ def set_span_recorder
       @span_recorder.add(self)
     end
 
-    def self.from_sentry_trace(sentry_trace, **options)
+    def self.from_sentry_trace(sentry_trace, configuration: Sentry.configuration, **options)
+      return unless configuration.tracing_enabled?
       return unless sentry_trace
 
       match = SENTRY_TRACE_REGEXP.match(sentry_trace)
@@ -67,8 +68,8 @@ def deep_dup
       copy
     end
 
-    def set_initial_sample_desicion(sampling_context = {})
-      unless Sentry.configuration.tracing_enabled?
+    def set_initial_sample_decision(sampling_context: {}, configuration: Sentry.configuration)
+      unless configuration.tracing_enabled?
         @sampled = false
         return
       end
@@ -77,9 +78,9 @@ def set_initial_sample_desicion(sampling_context = {})
 
       transaction_description = generate_transaction_description
 
-      logger = Sentry.configuration.logger
-      sample_rate = Sentry.configuration.traces_sample_rate
-      traces_sampler = Sentry.configuration.traces_sampler
+      logger = configuration.logger
+      sample_rate = configuration.traces_sample_rate
+      traces_sampler = configuration.traces_sampler
 
       if traces_sampler.is_a?(Proc)
         sampling_context = sampling_context.merge(

sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb

@@ -287,16 +287,38 @@
         Sentry.configuration.traces_sample_rate = nil
       end
 
+      let(:stack) do
+        described_class.new(
+          ->(_) do
+            [200, {}, ["ok"]]
+          end
+        )
+      end
+
       it "doesn't record transaction" do
-        app = ->(_) do
-          [200, {}, ["ok"]]
+        stack.call(env)
+
+        expect(transport.events.count).to eq(0)
+      end
+
+      context "when sentry-trace header is sent" do
+        let(:external_transaction) do
+          Sentry::Transaction.new(
+            op: "pageload",
+            status: "ok",
+            sampled: true,
+            name: "a/path"
+          )
         end
 
-        stack = described_class.new(app)
+        it "doesn't cause the transaction to be recorded" do
+          env["HTTP_SENTRY_TRACE"] = external_transaction.to_sentry_trace
 
-        stack.call(env)
+          response = stack.call(env)
 
-        expect(transport.events.count).to eq(0)
+          expect(response[0]).to eq(200)
+          expect(transport.events).to be_empty
+        end
       end
     end
   end

sentry-ruby/spec/sentry/transaction_spec.rb

@@ -15,20 +15,40 @@
   describe ".from_sentry_trace" do
     let(:sentry_trace) { subject.to_sentry_trace }
 
-    it "returns correctly-formatted value" do
-      child_transaction = described_class.from_sentry_trace(sentry_trace, op: "child")
-
-      expect(child_transaction.trace_id).to eq(subject.trace_id)
-      expect(child_transaction.parent_span_id).to eq(subject.span_id)
-      expect(child_transaction.parent_sampled).to eq(true)
-      expect(child_transaction.sampled).to eq(true)
-      expect(child_transaction.op).to eq("child")
+    let(:configuration) do
+      Sentry::Configuration.new
     end
 
-    it "handles invalid values without crashing" do
-      child_transaction = described_class.from_sentry_trace("dummy", op: "child")
+    context "when tracing is enabled" do
+      before do
+        configuration.traces_sample_rate = 1.0
+      end
+
+      it "returns correctly-formatted value" do
+        child_transaction = described_class.from_sentry_trace(sentry_trace, op: "child", configuration: configuration)
+
+        expect(child_transaction.trace_id).to eq(subject.trace_id)
+        expect(child_transaction.parent_span_id).to eq(subject.span_id)
+        expect(child_transaction.parent_sampled).to eq(true)
+        expect(child_transaction.sampled).to eq(true)
+        expect(child_transaction.op).to eq("child")
+      end
+
+      it "handles invalid values without crashing" do
+        child_transaction = described_class.from_sentry_trace("dummy", op: "child", configuration: configuration)
 
-      expect(child_transaction).to be_nil
+        expect(child_transaction).to be_nil
+      end
+    end
+
+    context "when tracing is disabled" do
+      before do
+        configuration.traces_sample_rate = 0.0
+      end
+
+      it "returns nil" do
+        expect(described_class.from_sentry_trace(sentry_trace, op: "child", configuration: configuration)).to be_nil
+      end
     end
   end
 
@@ -102,7 +122,7 @@
     end
   end
 
-  describe "#set_initial_sample_desicion" do
+  describe "#set_initial_sample_decision" do
     before do
       perform_basic_setup
     end
@@ -116,7 +136,7 @@
         allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(false)
 
         transaction = described_class.new(sampled: true)
-        transaction.set_initial_sample_desicion
+        transaction.set_initial_sample_decision
         expect(transaction.sampled).to eq(false)
       end
     end
@@ -131,11 +151,11 @@
       context "when the transaction already has a decision" do
         it "doesn't change it" do
           transaction = described_class.new(sampled: true)
-          transaction.set_initial_sample_desicion
+          transaction.set_initial_sample_decision
           expect(transaction.sampled).to eq(true)
 
           transaction = described_class.new(sampled: false)
-          transaction.set_initial_sample_desicion
+          transaction.set_initial_sample_decision
           expect(transaction.sampled).to eq(false)
         end
       end
@@ -151,7 +171,7 @@
             "[Tracing] Starting <rack.request> transaction"
           )
 
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
           expect(subject.sampled).to eq(true)
         end
 
@@ -161,14 +181,14 @@
             "[Tracing] Discarding <rack.request> transaction because it's not included in the random sample (sampling rate = 0.5)"
           )
 
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
           expect(subject.sampled).to eq(false)
         end
 
         it "accepts integer traces_sample_rate" do
           Sentry.configuration.traces_sample_rate = 1
 
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
           expect(subject.sampled).to eq(true)
         end
       end
@@ -178,7 +198,7 @@
           Sentry.configuration.traces_sampler = ""
 
           expect do
-            subject.set_initial_sample_desicion
+            subject.set_initial_sample_decision
           end.not_to raise_error
         end
 
@@ -189,7 +209,7 @@
             sampling_context = context
           end
 
-          subject.set_initial_sample_desicion(foo: "bar")
+          subject.set_initial_sample_decision(sampling_context: { foo: "bar" })
 
           # transaction_context's sampled attribute will be the old value
           expect(sampling_context[:transaction_context].keys).to eq(subject.to_hash.keys)
@@ -203,7 +223,7 @@
           )
 
           Sentry.configuration.traces_sampler = -> (_) { "foo" }
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
 
           expect(subject.sampled).to eq(false)
         end
@@ -215,17 +235,17 @@
 
           subject = described_class.new
           Sentry.configuration.traces_sampler = -> (_) { true }
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
           expect(subject.sampled).to eq(true)
 
           subject = described_class.new
           Sentry.configuration.traces_sampler = -> (_) { 1.0 }
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
           expect(subject.sampled).to eq(true)
 
           subject = described_class.new
           Sentry.configuration.traces_sampler = -> (_) { 1 }
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
           expect(subject.sampled).to eq(true)
         end
 
@@ -236,12 +256,12 @@
 
           subject = described_class.new
           Sentry.configuration.traces_sampler = -> (_) { false }
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
           expect(subject.sampled).to eq(false)
 
           subject = described_class.new
           Sentry.configuration.traces_sampler = -> (_) { 0.0 }
-          subject.set_initial_sample_desicion
+          subject.set_initial_sample_decision
           expect(subject.sampled).to eq(false)
         end
       end