diff --git a/sentry-ruby/lib/sentry-ruby.rb b/sentry-ruby/lib/sentry-ruby.rb index 2c1382517..048cd7936 100644 --- a/sentry-ruby/lib/sentry-ruby.rb +++ b/sentry-ruby/lib/sentry-ruby.rb @@ -164,6 +164,8 @@ def capture_event(event) # Takes or initializes a new Sentry::Transaction and makes a sampling decision for it. def start_transaction(**options) + return unless configuration.tracing_enabled? + get_current_hub&.start_transaction(**options) end diff --git a/sentry-ruby/lib/sentry/configuration.rb b/sentry-ruby/lib/sentry/configuration.rb index 4b2123c66..e50062562 100644 --- a/sentry-ruby/lib/sentry/configuration.rb +++ b/sentry-ruby/lib/sentry/configuration.rb @@ -293,7 +293,7 @@ def enabled_in_current_env? end def tracing_enabled? - !!((@traces_sample_rate && @traces_sample_rate > 0.0) || @traces_sampler) + !!(@traces_sampler || (@traces_sample_rate.is_a?(Numeric) && @traces_sample_rate >= 0.0 && @traces_sample_rate <= 1.0)) end def stacktrace_builder 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..df68114c2 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb @@ -17,23 +17,23 @@ def call(env) scope.set_rack_env(env) transaction = start_transaction(env, scope) - scope.set_span(transaction) + transaction ? scope.set_span(transaction) : nil begin response = @app.call(env) rescue Sentry::Error - finish_transaction(transaction, 500) + transaction ? finish_transaction(transaction, 500) : nil raise # Don't capture Sentry errors rescue Exception => e capture_exception(e) - finish_transaction(transaction, 500) + transaction ? finish_transaction(transaction, 500) : nil raise end exception = collect_exception(env) capture_exception(exception) if exception - finish_transaction(transaction, response[0]) + transaction ? finish_transaction(transaction, response[0]) : nil response end @@ -54,13 +54,20 @@ def capture_exception(exception) end def start_transaction(env, scope) + return unless Sentry.configuration.tracing_enabled? + 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} + + # if tracing is disabled, these will both return nil + 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..4881dbe3c 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -33,9 +33,16 @@ 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" + if sampled_flag == "1" + parent_sampled = true + elsif sampled_flag == "0" + parent_sampled = false + else + parent_sampled = nil + end - new(trace_id: trace_id, parent_span_id: parent_span_id, parent_sampled: sampled, sampled: sampled, **options) + + new(trace_id: trace_id, parent_span_id: parent_span_id, parent_sampled: parent_sampled, **options) end def to_hash @@ -69,26 +76,28 @@ def deep_dup end def set_initial_sample_decision(sampling_context: {}, configuration: Sentry.configuration) + logger = configuration.logger + transaction_description = generate_transaction_description + unless configuration.tracing_enabled? - @sampled = false - return + # we shouldn't ever get here (because if tracing is disabled, start_transaction should bail + # before it calls set_initial_sample_decision), so warn, because something's gone wrong + logger.warn("#{MESSAGE_PREFIX} Tracing is disabled. #{transaction_description} will not be sent.") + return nil end return unless @sampled.nil? - transaction_description = generate_transaction_description - - logger = configuration.logger - sample_rate = configuration.traces_sample_rate - traces_sampler = configuration.traces_sampler - - if traces_sampler.is_a?(Proc) + if configuration.traces_sampler.is_a?(Proc) sampling_context = sampling_context.merge( parent_sampled: @parent_sampled, transaction_context: self.to_hash ) - - sample_rate = traces_sampler.call(sampling_context) + sample_rate = configuration.traces_sampler.call(sampling_context) + elsif @parent_sampled + sample_rate = @parent_sampled + else + sample_rate = configuration.traces_sample_rate end unless [true, false].include?(sample_rate) || (sample_rate.is_a?(Numeric) && sample_rate >= 0.0 && sample_rate <= 1.0) @@ -99,7 +108,7 @@ def set_initial_sample_decision(sampling_context: {}, configuration: Sentry.conf if sample_rate == 0.0 || sample_rate == false @sampled = false - logger.debug("#{MESSAGE_PREFIX} Discarding #{transaction_description} because traces_sampler returned 0 or false") + logger.debug("#{MESSAGE_PREFIX} Discarding #{transaction_description} because traces_sample_rate is 0 or traces_sampler returned 0 or false") return end @@ -119,14 +128,16 @@ def set_initial_sample_decision(sampling_context: {}, configuration: Sentry.conf end def finish(hub: nil) + # it's unlikely we'd get this far if tracing is disabled, but just in case... + return unless Sentry.configuration.tracing_enabled? + return unless @sampled + super() # Span#finish doesn't take arguments if @name.nil? @name = UNLABELD_NAME end - return unless @sampled || @parent_sampled - hub ||= Sentry.get_current_hub event = hub.current_client.event_from_transaction(self) hub.capture_event(event) diff --git a/sentry-ruby/spec/sentry/configuration_spec.rb b/sentry-ruby/spec/sentry/configuration_spec.rb index b867a898e..d8e08514f 100644 --- a/sentry-ruby/spec/sentry/configuration_spec.rb +++ b/sentry-ruby/spec/sentry/configuration_spec.rb @@ -11,19 +11,21 @@ end describe "#tracing_enabled?" do - it "returns false by default" do - expect(subject.tracing_enabled?).to eq(false) + context "when neither traces_sample_rate nor traces_sampler is set" do + it "returns false" do + expect(subject.tracing_enabled?).to eq(false) + end end context "when traces_sample_rate == 0.0" do - it "returns false" do + it "returns true" do subject.traces_sample_rate = 0.0 - expect(subject.tracing_enabled?).to eq(false) + expect(subject.tracing_enabled?).to eq(true) end end - context "when traces_sample_rate > 0" do + context "when traces_sample_rate > 0 and <= 1" do it "returns true" do subject.traces_sample_rate = 0.1 @@ -31,6 +33,14 @@ end end + context "when traces_sample_rate > 1" do + it "returns false" do + subject.traces_sample_rate = 1.1 + + expect(subject.tracing_enabled?).to eq(false) + end + end + context "when traces_sampler is set" do it "returns true" do subject.traces_sampler = proc { true } diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index 3a011e0a7..31edb9c27 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb @@ -182,6 +182,7 @@ stack.call(env) + transaction = transport.events.last expect(transaction.type).to eq("transaction") expect(transaction.timestamp).not_to be_nil @@ -215,7 +216,7 @@ allow(Random).to receive(:rand).and_return(0.4) end - it "starts a span and finishes it" do + it "starts a transaction and finishes it" do app = ->(_) do [200, {}, ["ok"]] end @@ -282,9 +283,9 @@ end end - context "when traces_sample_rate is not set" do + context "when tracing is disabled" do before do - Sentry.configuration.traces_sample_rate = nil + allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(false) end let(:stack) do diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index be7d2cffe..c6563b973 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -7,7 +7,6 @@ description: "SELECT * FROM users;", status: "ok", sampled: true, - parent_sampled: true, name: "foo" ) end @@ -24,13 +23,22 @@ configuration.traces_sample_rate = 1.0 end - it "returns correctly-formatted value" do + it "correctly parses sentry-trace header when sampled = true" 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.parent_sampled).to eq(subject.sampled) + expect(child_transaction.op).to eq("child") + end + + it "correctly parses sentry-trace header when sampled = nil" do + # slice of the last character to eliminate the sampling decision + child_transaction = described_class.from_sentry_trace(sentry_trace[0..-2], 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(nil) expect(child_transaction.op).to eq("child") end @@ -42,10 +50,6 @@ 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 @@ -127,40 +131,77 @@ perform_basic_setup end - context "when tracing is not enabled" do + context "when tracing is disabled" do before do allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(false) end - it "sets @sampled to false and return" do - allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(false) + it "warns and returns" do + transaction = described_class.new + expect(Sentry.configuration.logger).to receive(:warn).with( + /^\[Tracing\] Tracing is disabled.*$/ + ) + transaction.set_initial_sample_decision + expect(transaction.sampled).to be_nil + end + it "warns and returns even if passed an explicit sampling decision" do transaction = described_class.new(sampled: true) + expect(Sentry.configuration.logger).to receive(:warn).with( + /^\[Tracing\] Tracing is disabled.*$/ + ) transaction.set_initial_sample_decision - expect(transaction.sampled).to eq(false) + end + + it "warns and returns even if passed a parent sampling decision" do + transaction = described_class.new + expect(Sentry.configuration.logger).to receive(:warn).with( + /^\[Tracing\] Tracing is disabled.*$/ + ) + transaction.set_initial_sample_decision(sampling_context: { parent_sampled: true }) + expect(transaction.sampled).to be_nil end end context "when tracing is enabled" do - let(:subject) { described_class.new(op: "rack.request", parent_sampled: true) } + let(:subject) { described_class.new(op: "rack.request") } before do allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(true) end - context "when the transaction already has a decision" do - it "doesn't change it" do - transaction = described_class.new(sampled: true) + context "when precedence comes into play" do + # For each of these tests, successively fewer of the conflicting options are set, so that + # we can see which of the remaining options takes precedence. In each case, the option which + # should take precedence leads the transaction to be sampled, while the other ones would make + # it unsampled (if they were to win, which they won't). + + it "gives explicitly passed decisions top priority" do + allow(Sentry.configuration).to receive(:traces_sample_rate).and_return(0) + allow(Sentry.configuration).to receive(:traces_sampler).and_return(proc { false }) + transaction = described_class.new(sampled: true, parent_sampled: false) transaction.set_initial_sample_decision expect(transaction.sampled).to eq(true) + end + + it "uses traces_sampler over inherited decision or sample rate" do + allow(Sentry.configuration).to receive(:traces_sample_rate).and_return(0) + allow(Sentry.configuration).to receive(:traces_sampler).and_return(proc { true }) + transaction = described_class.new(sampled: nil, parent_sampled: false) + transaction.set_initial_sample_decision + expect(transaction.sampled).to eq(true) + end - transaction = described_class.new(sampled: false) + it "uses inherited decision over sample rate" do + allow(Sentry.configuration).to receive(:traces_sample_rate).and_return(0) + allow(Sentry.configuration).to receive(:traces_sampler).and_return(nil) + transaction = described_class.new(sampled: nil, parent_sampled: true) transaction.set_initial_sample_decision - expect(transaction.sampled).to eq(false) + expect(transaction.sampled).to eq(true) end end - context "when traces_sampler is not set" do + context "when using traces_sample_rate" do before do Sentry.configuration.traces_sample_rate = 0.5 end @@ -193,7 +234,7 @@ end end - context "when traces_sampler is provided" do + context "when using traces_sampler" do it "ignores the sampler if it's not callable" do Sentry.configuration.traces_sampler = "" @@ -203,21 +244,21 @@ end it "calls the sampler with sampling_context" do + transaction = described_class.new(parent_sampled: true) sampling_context = {} Sentry.configuration.traces_sampler = lambda do |context| sampling_context = context end - subject.set_initial_sample_decision(sampling_context: { foo: "bar" }) + transaction.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) expect(sampling_context[:parent_sampled]).to eq(true) expect(sampling_context[:foo]).to eq("bar") end - it "disgards the transaction if generated sample rate is not valid" do + it "discards the transaction if generated sample rate is not valid" do expect(Sentry.configuration.logger).to receive(:warn).with( "[Tracing] Discarding transaction because of invalid sample_rate: foo" ) @@ -251,7 +292,7 @@ it "uses the genereted rate for sampling (negative)" do expect(Sentry.configuration.logger).to receive(:debug).with( - "[Tracing] Discarding transaction because traces_sampler returned 0 or false" + /^\[Tracing\] Discarding transaction.*$/ ).exactly(2) subject = described_class.new @@ -278,7 +319,6 @@ expect(hash[:trace_id].length).to eq(32) expect(hash[:span_id].length).to eq(16) expect(hash[:sampled]).to eq(true) - expect(hash[:parent_sampled]).to eq(true) expect(hash[:name]).to eq("foo") end end @@ -292,33 +332,55 @@ Sentry.get_current_client.transport.events end - it "finishes the transaction, converts it into an Event and send it" do - subject.finish + context "tracing is enabled" do - expect(events.count).to eq(1) - event = events.last.to_hash + before do + allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(true) + end - # don't contain itself - expect(event[:spans]).to be_empty - end + it "finishes the transaction, converts it into an Event and sends it" do + subject.finish - context "if the transaction is not sampled" do - subject { described_class.new(sampled: false) } + expect(events.count).to eq(1) + event = events.last.to_hash - it "doesn't send it" do - subject.finish + # don't contain itself + expect(event[:spans]).to be_empty + end - expect(events.count).to eq(0) + context "if the transaction is not sampled" do + subject { described_class.new(sampled: false) } + + it "doesn't send it" do + subject.finish + + expect(events.count).to eq(0) + end + end + + context "if the transaction doesn't have a name" do + subject { described_class.new(sampled: true) } + + it "adds a default name" do + subject.finish + + expect(subject.name).to eq("") + end end end - context "if the transaction doesn't have a name" do - subject { described_class.new(sampled: true) } + context "tracing is disabled" do + before do + allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(false) + end + + # this shouldn't happen, but just in case + it "doesn't send the transaction, even if sampled == true" do + subject { described_class.new(sampled: true) } - it "adds a default name" do subject.finish - expect(subject.name).to eq("") + expect(events.count).to eq(0) end end end diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index f3d70a979..de62bf5ea 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -178,19 +178,78 @@ 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") + context "when tracing is enabled with traces_sample_rate > 0" do + it "starts a new transaction" do + described_class.init do |config| + config.traces_sample_rate = 1 + end + + transaction = described_class.start_transaction(op: "foo") + expect(transaction).to be_a(Sentry::Transaction) + expect(transaction.op).to eq("foo") + end + end + + context "when tracing is enabled with traces_sample_rate == 0" do + it "starts a new transaction" do + described_class.init do |config| + config.traces_sample_rate = 0 + end + + transaction = described_class.start_transaction(op: "foo") + expect(transaction).to be_a(Sentry::Transaction) + expect(transaction.op).to eq("foo") + end + end + + context "when tracing is enabled with traces_sampler" do + it "starts a new transaction" do + described_class.init do |config| + config.traces_sampler = proc { true } + end + + transaction = described_class.start_transaction(op: "foo") + expect(transaction).to be_a(Sentry::Transaction) + expect(transaction.op).to eq("foo") + end + end + + context "when neither traces_sample_rate nor traces_sampler is set" do + it "does not start a new transaction" do + transaction = described_class.start_transaction(op: "foo") + expect(transaction).to be_nil + end end context "when given an transaction object" do - it "adds sample decision to it" do - transaction = Sentry::Transaction.new + before do + described_class.init + end + + context "when tracing is enabled" do + it "adds sample decision to it" do + # described_class.init + allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(true) + allow(Sentry.configuration).to receive(:traces_sample_rate).and_return(1) + + transaction = Sentry::Transaction.new - described_class.start_transaction(transaction: transaction) + described_class.start_transaction(transaction: transaction) - expect(transaction.sampled).to eq(false) + expect(transaction.sampled).to eq(true) + end + end + + context "when tracing is disabled" do + it "bails early" do + allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(false) + + transaction = Sentry::Transaction.new + + described_class.start_transaction(transaction: transaction) + + expect(transaction.sampled).to eq(nil) + end end end end