From 3fc4501170fe331fc52153d2317c3dd6b416d47e Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 3 Mar 2021 22:41:00 +0800 Subject: [PATCH 1/5] Rename CaptureExceptions' span to transaction --- .../lib/sentry/rails/capture_exceptions.rb | 8 ++++---- .../lib/sentry/rack/capture_exceptions.rb | 18 +++++++++--------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/capture_exceptions.rb b/sentry-rails/lib/sentry/rails/capture_exceptions.rb index 8bb7efbbc..7d787fe0b 100644 --- a/sentry-rails/lib/sentry/rails/capture_exceptions.rb +++ b/sentry-rails/lib/sentry/rails/capture_exceptions.rb @@ -29,10 +29,10 @@ 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 finish_transaction(transaction, status_code) + if @assets_regex.nil? || !transaction.name.match?(@assets_regex) + transaction.set_http_status(status_code) + transaction.finish end end end diff --git a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb index f664c7331..289363cdb 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb @@ -17,26 +17,26 @@ def call(env) 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) + 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) - scope.set_span(span) + 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 +56,9 @@ def capture_exception(exception) Sentry.capture_exception(exception) end - def finish_span(span, status_code) - span.set_http_status(status_code) - span.finish + def finish_transaction(transaction, status_code) + transaction.set_http_status(status_code) + transaction.finish end end end From a0d8a7f9c94dacd4ce993b56c974a2e3e7b4ff12 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 3 Mar 2021 22:46:27 +0800 Subject: [PATCH 2/5] Add CaptureExceptions#start_transaction for customization --- sentry-rails/lib/sentry/rails/capture_exceptions.rb | 11 +++++++---- sentry-ruby/lib/sentry/rack/capture_exceptions.rb | 12 ++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/capture_exceptions.rb b/sentry-rails/lib/sentry/rails/capture_exceptions.rb index 7d787fe0b..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_transaction(transaction, status_code) - if @assets_regex.nil? || !transaction.name.match?(@assets_regex) - transaction.set_http_status(status_code) - transaction.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/lib/sentry/rack/capture_exceptions.rb b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb index 289363cdb..5d8b5afbb 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb @@ -16,10 +16,7 @@ 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"] - 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) - + transaction = start_transaction(env, scope) scope.set_span(transaction) begin @@ -56,6 +53,13 @@ def capture_exception(exception) Sentry.capture_exception(exception) end + 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 From 4d9fdfa9277a776c09af0c40ab7c9b85ec1d5d67 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 3 Mar 2021 22:57:52 +0800 Subject: [PATCH 3/5] Correct typo --- sentry-ruby/lib/sentry/hub.rb | 2 +- sentry-ruby/lib/sentry/transaction.rb | 2 +- sentry-ruby/spec/sentry/transaction_spec.rb | 30 ++++++++++----------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/sentry-ruby/lib/sentry/hub.rb b/sentry-ruby/lib/sentry/hub.rb index a421596f7..c617396e7 100644 --- a/sentry-ruby/lib/sentry/hub.rb +++ b/sentry-ruby/lib/sentry/hub.rb @@ -71,7 +71,7 @@ def pop_scope def start_transaction(transaction: nil, **options) transaction ||= Transaction.new(**options) - transaction.set_initial_sample_desicion + transaction.set_initial_sample_decision transaction end diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index f47b29a1b..42dbabc80 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -68,7 +68,7 @@ def deep_dup copy end - def set_initial_sample_desicion(sampling_context = {}) + def set_initial_sample_decision(sampling_context = {}) unless Sentry.configuration.tracing_enabled? @sampled = false return diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index 336d57936..983f3f3f2 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -121,7 +121,7 @@ end end - describe "#set_initial_sample_desicion" do + describe "#set_initial_sample_decision" do before do perform_basic_setup end @@ -135,7 +135,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 +150,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 +170,7 @@ "[Tracing] Starting transaction" ) - subject.set_initial_sample_desicion + subject.set_initial_sample_decision expect(subject.sampled).to eq(true) end @@ -180,14 +180,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 +197,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 +208,7 @@ sampling_context = context end - subject.set_initial_sample_desicion(foo: "bar") + subject.set_initial_sample_decision(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 +222,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 +234,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 +255,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 From 36fec3c4632d9d4184f5d3be98372297ef326050 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 3 Mar 2021 23:07:14 +0800 Subject: [PATCH 4/5] 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. --- sentry-ruby/lib/sentry/hub.rb | 4 ++-- sentry-ruby/lib/sentry/transaction.rb | 14 +++++++------- sentry-ruby/spec/sentry/transaction_spec.rb | 21 +++++++++++---------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/sentry-ruby/lib/sentry/hub.rb b/sentry-ruby/lib/sentry/hub.rb index c617396e7..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_decision + transaction.set_initial_sample_decision(configuration: current_client.configuration) transaction end diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index 42dbabc80..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_decision(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_decision(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 983f3f3f2..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 @@ -208,7 +209,7 @@ sampling_context = context end - subject.set_initial_sample_decision(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) From 91d6ad20e1265508b4ed207ff3886b4779cf8ad6 Mon Sep 17 00:00:00 2001 From: st0012 Date: Thu, 4 Mar 2021 12:15:49 +0800 Subject: [PATCH 5/5] Update changelog --- sentry-ruby/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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