From cb3c7b0b46e154daabf818bc403fbf647b82296a Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Wed, 3 Mar 2021 22:37:44 +0800 Subject: [PATCH 1/2] Ignore sentry-trace when tracing is not enabled (#1308) * Ignore sentry-trace when tracing is not enabled * Update changelog --- sentry-ruby/CHANGELOG.md | 2 + sentry-ruby/lib/sentry/transaction.rb | 1 + .../sentry/rack/capture_exceptions_spec.rb | 32 ++++++++++++--- sentry-ruby/spec/sentry/transaction_spec.rb | 41 ++++++++++++++----- 4 files changed, 60 insertions(+), 16 deletions(-) diff --git a/sentry-ruby/CHANGELOG.md b/sentry-ruby/CHANGELOG.md index 6c8a75c3d..375d69308 100644 --- a/sentry-ruby/CHANGELOG.md +++ b/sentry-ruby/CHANGELOG.md @@ -3,6 +3,8 @@ ## Unreleased - Refactor interface construction [#1296](https://github.com/getsentry/sentry-ruby/pull/1296) +- 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) ## 4.2.2 diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index f0725d19a..f47b29a1b 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -26,6 +26,7 @@ def set_span_recorder end def self.from_sentry_trace(sentry_trace, **options) + return unless Sentry.configuration.tracing_enabled? return unless sentry_trace match = SENTRY_TRACE_REGEXP.match(sentry_trace) diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index 36c9a5b7d..3a011e0a7 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/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 diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index 0a96e8ac3..336d57936 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -15,20 +15,39 @@ 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") + before do + configuration = Sentry::Configuration.new + configuration.traces_sample_rate = 1.0 + allow(Sentry).to receive(:configuration).and_return(configuration) end - it "handles invalid values without crashing" do - child_transaction = described_class.from_sentry_trace("dummy", op: "child") + context "when tracing is enabled" do + 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") + end + + it "handles invalid values without crashing" do + child_transaction = described_class.from_sentry_trace("dummy", op: "child") - expect(child_transaction).to be_nil + expect(child_transaction).to be_nil + end + end + + context "when tracing is disabled" do + before do + configuration = Sentry::Configuration.new + allow(Sentry).to receive(:configuration).and_return(configuration) + end + + it "returns nil" do + expect(described_class.from_sentry_trace(sentry_trace, op: "child")).to be_nil + end end end From f6310a1dda42967cb95e132fca84afb095107e0d Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 4 Mar 2021 12:24:10 +0800 Subject: [PATCH 2/2] Refactor tracing implementation (#1309) * Rename CaptureExceptions' span to transaction * Add CaptureExceptions#start_transaction for customization * Correct typo * Allow passing different configuration when starting transactions The original implementation always uses the active hub's configuration when starting transactions. But since we allow users to work with multiple hubs with different configurations, we should also allow them to apply on the performance monitoring feature. * Update changelog --- .../lib/sentry/rails/capture_exceptions.rb | 11 +++-- sentry-ruby/CHANGELOG.md | 1 + sentry-ruby/lib/sentry/hub.rb | 4 +- .../lib/sentry/rack/capture_exceptions.rb | 26 +++++----- sentry-ruby/lib/sentry/transaction.rb | 14 +++--- sentry-ruby/spec/sentry/transaction_spec.rb | 49 ++++++++++--------- 6 files changed, 57 insertions(+), 48 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/capture_exceptions.rb b/sentry-rails/lib/sentry/rails/capture_exceptions.rb index 8bb7efbbc..c45730c91 100644 --- a/sentry-rails/lib/sentry/rails/capture_exceptions.rb +++ b/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 diff --git a/sentry-ruby/CHANGELOG.md b/sentry-ruby/CHANGELOG.md index 375d69308..8013e70e4 100644 --- a/sentry-ruby/CHANGELOG.md +++ b/sentry-ruby/CHANGELOG.md @@ -5,6 +5,7 @@ - Refactor interface construction [#1296](https://github.com/getsentry/sentry-ruby/pull/1296) - 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 diff --git a/sentry-ruby/lib/sentry/hub.rb b/sentry-ruby/lib/sentry/hub.rb index a421596f7..e86ece913 100644 --- a/sentry-ruby/lib/sentry/hub.rb +++ b/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 diff --git a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb index f664c7331..5d8b5afbb 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/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 diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index f47b29a1b..8d22323a5 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -25,8 +25,8 @@ def set_span_recorder @span_recorder.add(self) end - def self.from_sentry_trace(sentry_trace, **options) - return unless Sentry.configuration.tracing_enabled? + 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) @@ -68,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 @@ -78,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( diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index 336d57936..be7d2cffe 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -15,15 +15,17 @@ describe ".from_sentry_trace" do let(:sentry_trace) { subject.to_sentry_trace } - before do - configuration = Sentry::Configuration.new - configuration.traces_sample_rate = 1.0 - allow(Sentry).to receive(:configuration).and_return(configuration) + let(:configuration) do + Sentry::Configuration.new end 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") + 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) @@ -33,7 +35,7 @@ end it "handles invalid values without crashing" do - child_transaction = described_class.from_sentry_trace("dummy", op: "child") + child_transaction = described_class.from_sentry_trace("dummy", op: "child", configuration: configuration) expect(child_transaction).to be_nil end @@ -41,12 +43,11 @@ context "when tracing is disabled" do before do - configuration = Sentry::Configuration.new - allow(Sentry).to receive(:configuration).and_return(configuration) + configuration.traces_sample_rate = 0.0 end it "returns nil" do - expect(described_class.from_sentry_trace(sentry_trace, op: "child")).to be_nil + expect(described_class.from_sentry_trace(sentry_trace, op: "child", configuration: configuration)).to be_nil end end end @@ -121,7 +122,7 @@ end end - describe "#set_initial_sample_desicion" do + describe "#set_initial_sample_decision" do before do perform_basic_setup end @@ -135,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 @@ -150,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 @@ -170,7 +171,7 @@ "[Tracing] Starting transaction" ) - subject.set_initial_sample_desicion + subject.set_initial_sample_decision expect(subject.sampled).to eq(true) end @@ -180,14 +181,14 @@ "[Tracing] Discarding 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 @@ -197,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 @@ -208,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) @@ -222,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 @@ -234,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 @@ -255,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