From abd5d860009efb1fb5443cc33e9aaf9155ab735e Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Thu, 15 Sep 2022 15:56:38 +0200 Subject: [PATCH 01/18] Baggage creation for head SDK --- sentry-ruby/lib/sentry/span.rb | 2 +- sentry-ruby/lib/sentry/transaction.rb | 61 +++++++++++++++++++-- sentry-ruby/lib/sentry/transaction_event.rb | 2 +- 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/sentry-ruby/lib/sentry/span.rb b/sentry-ruby/lib/sentry/span.rb index e957f8227..94c276b92 100644 --- a/sentry-ruby/lib/sentry/span.rb +++ b/sentry-ruby/lib/sentry/span.rb @@ -108,7 +108,7 @@ def to_sentry_trace # from the incoming baggage stored on the transation. # @return [String, nil] def to_baggage - transaction&.baggage&.serialize + transaction&.get_baggage&.serialize end # @return [Hash] diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index 2dbe0f4c1..f0f91659c 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -24,10 +24,6 @@ class Transaction < Span # @return [String] attr_reader :parent_sampled - # The parsed incoming W3C baggage header - # @return [Baggage, nil] - attr_reader :baggage - # @deprecated Use Sentry.get_current_hub instead. attr_reader :hub @@ -37,6 +33,10 @@ class Transaction < Span # @deprecated Use Sentry.logger instead. attr_reader :logger + # The effective sample rate at which this transaction was sampled. + # @return [Float, nil] + attr_reader :effective_sample_rate + def initialize(name: nil, parent_sampled: nil, baggage: nil, hub:, **options) super(**options) @@ -50,6 +50,7 @@ def initialize(name: nil, parent_sampled: nil, baggage: nil, hub:, **options) @traces_sampler = hub.configuration.traces_sampler @traces_sample_rate = hub.configuration.traces_sample_rate @logger = hub.configuration.logger + @effective_sample_rate = nil init_span_recorder end @@ -78,7 +79,12 @@ def self.from_sentry_trace(sentry_trace, baggage: nil, hub: Sentry.get_current_h sampled_flag != "0" end + # If there's an incoming sentry-trace but no incoming baggage header, + # for instance in traces coming from older SDKs, + # baggage will be empty and frozen and won't be populated as head SDK. baggage = Baggage.from_incoming_header(baggage) if baggage + baggage ||= Baggage.new({}) + baggage.freeze! new( trace_id: trace_id, @@ -120,7 +126,10 @@ def set_initial_sample_decision(sampling_context:) return end - return unless @sampled.nil? + unless @sampled.nil? + @effective_sample_rate = @sampled ? 1.0 : 0.0 + return + end sample_rate = if @traces_sampler.is_a?(Proc) @@ -139,6 +148,12 @@ def set_initial_sample_decision(sampling_context:) return end + @effective_sample_rate = if [true, false].include?(sample_rate) + sample_rate ? 1.0 : 0.0 + else + sample_rate.to_f + end + if sample_rate == 0.0 || sample_rate == false @sampled = false log_debug("#{MESSAGE_PREFIX} Discarding #{transaction_description} because traces_sampler returned 0 or false") @@ -189,6 +204,14 @@ def finish(hub: nil) end end + # Get the existing frozen incoming baggage + # or populate one with sentry- items as the head SDK. + # @return [Baggage] + def get_baggage + populate_head_baggage if @baggage.nil? || @baggage.mutable + @baggage + end + protected def init_span_recorder(limit = 1000) @@ -205,6 +228,34 @@ def generate_transaction_description result end + def populate_head_baggage + sentry_items = {} + sentry_items["trace_id"] = trace_id + + # TODO-neel filter for high cardinality / low quality tx source here + sentry_items["transaction"] = name + sentry_items["sample_rate"] = effective_sample_rate&.to_s + + configuration = Sentry.configuration + + if configuration + sentry_items["environment"] = configuration.environment + sentry_items["release"] = configuration.release + sentry_items["public_key"] = configuration.dsn&.public_key + end + + user = Sentry.get_current_scope&.user + sentry_items["user_segment"] = user["segment"] if user && user["segment"] + + # there's an existing baggage but it was mutable, + # which is why we are creating this new baggage. + # However, if by chance the user put some sentry items in there, give them precedence. + sentry_items.merge!(@baggage.sentry_items) if @baggage && !@baggage.sentry_items.empty? + sentry_items.compact! + + @baggage = Baggage.new(sentry_items, mutable: false) + end + class SpanRecorder attr_reader :max_length, :spans diff --git a/sentry-ruby/lib/sentry/transaction_event.rb b/sentry-ruby/lib/sentry/transaction_event.rb index 355558726..4e6092112 100644 --- a/sentry-ruby/lib/sentry/transaction_event.rb +++ b/sentry-ruby/lib/sentry/transaction_event.rb @@ -22,7 +22,7 @@ def initialize(transaction:, **options) self.timestamp = transaction.timestamp self.start_timestamp = transaction.start_timestamp self.tags = transaction.tags - self.dynamic_sampling_context = transaction.baggage&.dynamic_sampling_context + self.dynamic_sampling_context = transaction.get_baggage.dynamic_sampling_context finished_spans = transaction.span_recorder.spans.select { |span| span.timestamp && span != transaction } self.spans = finished_spans.map(&:to_hash) From d7e2797f70fcc4268fd2b8efbd3377ff4c60bede Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Fri, 16 Sep 2022 18:32:07 +0200 Subject: [PATCH 02/18] Fix specs for head SDK --- sentry-ruby/spec/sentry/client_spec.rb | 53 +++++++++++++---------- sentry-ruby/spec/sentry/net/http_spec.rb | 16 +++++-- sentry-ruby/spec/sentry/transport_spec.rb | 50 ++++++++++----------- 3 files changed, 66 insertions(+), 53 deletions(-) diff --git a/sentry-ruby/spec/sentry/client_spec.rb b/sentry-ruby/spec/sentry/client_spec.rb index ca0658579..204bb01cc 100644 --- a/sentry-ruby/spec/sentry/client_spec.rb +++ b/sentry-ruby/spec/sentry/client_spec.rb @@ -93,11 +93,14 @@ def sentry_context let(:hub) do Sentry::Hub.new(subject, Sentry::Scope.new) end + let(:transaction) do - Sentry::Transaction.new(name: "test transaction", hub: hub, sampled: true) + hub.start_transaction(name: "test transaction") end before do + configuration.traces_sample_rate = 1.0 + transaction.start_child(op: "unfinished child") transaction.start_child(op: "finished child", timestamp: Time.now.utc.iso8601) end @@ -114,33 +117,37 @@ def sentry_context expect(event_hash[:spans].count).to eq(1) expect(event_hash[:spans][0][:op]).to eq("finished child") expect(event_hash[:level]).to eq(nil) - expect(event.dynamic_sampling_context).to eq(nil) end - context "when baggage present" do - let(:transaction) do - baggage = Sentry::Baggage.from_incoming_header( - "other-vendor-value-1=foo;bar;baz, "\ - "sentry-trace_id=771a43a4192642f0b136d5159a501700, "\ - "sentry-public_key=49d0f7386ad645858ae85020e393bef3, "\ - "sentry-sample_rate=0.01337, "\ - "sentry-user_id=Am%C3%A9lie, "\ - "other-vendor-value-2=foo;bar;" - ) + it "correct dynamic_sampling_context when incoming baggage header" do + baggage = Sentry::Baggage.from_incoming_header( + "other-vendor-value-1=foo;bar;baz, "\ + "sentry-trace_id=771a43a4192642f0b136d5159a501700, "\ + "sentry-public_key=49d0f7386ad645858ae85020e393bef3, "\ + "sentry-sample_rate=0.01337, "\ + "sentry-user_id=Am%C3%A9lie, "\ + "other-vendor-value-2=foo;bar;" + ) + transaction = Sentry::Transaction.new(name: "test transaction", hub: hub, baggage: baggage, sampled: true) + event = subject.event_from_transaction(transaction) - Sentry::Transaction.new(name: "test transaction", hub: hub, baggage: baggage, sampled: true) - end + expect(event.dynamic_sampling_context).to eq({ + "sample_rate" => "0.01337", + "public_key" => "49d0f7386ad645858ae85020e393bef3", + "trace_id" => "771a43a4192642f0b136d5159a501700", + "user_id" => "Amélie" + }) + end - it "TransactionEvent has dynamic_sampling_context" do - event = subject.event_from_transaction(transaction) - expect(event.dynamic_sampling_context).to eq({ - "sample_rate" => "0.01337", - "public_key" => "49d0f7386ad645858ae85020e393bef3", - "trace_id" => "771a43a4192642f0b136d5159a501700", - "user_id" => "Amélie" - }) - end + it "correct dynamic_sampling_context when head SDK" do + event = subject.event_from_transaction(transaction) + + expect(event.dynamic_sampling_context).to eq({ + "sample_rate" => "1.0", + "transaction" => "test transaction", + "trace_id" => transaction.trace_id + }) end end diff --git a/sentry-ruby/spec/sentry/net/http_spec.rb b/sentry-ruby/spec/sentry/net/http_spec.rb index d4ab16af4..fcacd7956 100644 --- a/sentry-ruby/spec/sentry/net/http_spec.rb +++ b/sentry-ruby/spec/sentry/net/http_spec.rb @@ -92,8 +92,7 @@ expect(request["sentry-trace"]).to eq(request_span.to_sentry_trace) end - # TODO-neel change this when head SDK implemented - it "does not add empty baggage header to the request header when no incoming trace" do + it "adds baggage header to the request header as head SDK when no incoming trace" do stub_normal_response uri = URI("http://example.com/path") @@ -106,10 +105,18 @@ response = http.request(request) expect(response.code).to eq("200") - expect(string_io.string).not_to match( + expect(string_io.string).to match( /\[Tracing\] Adding baggage header to outgoing request:/ ) - expect(request.key?("baggage")).to eq(false) + + request_span = transaction.span_recorder.spans.last + expect(request["baggage"]).to eq(request_span.to_baggage) + expect(request["baggage"]).to eq( + "sentry-trace_id=#{transaction.trace_id},"\ + "sentry-sample_rate=1.0,"\ + "sentry-environment=development,"\ + "sentry-public_key=foobarbaz" + ) end it "adds baggage header to the request header when continuing incoming trace" do @@ -136,6 +143,7 @@ expect(string_io.string).to match( /\[Tracing\] Adding baggage header to outgoing request:/ ) + request_span = transaction.span_recorder.spans.last expect(request["baggage"]).to eq(request_span.to_baggage) expect(request["baggage"]).to eq( diff --git a/sentry-ruby/spec/sentry/transport_spec.rb b/sentry-ruby/spec/sentry/transport_spec.rb index 92e2c32d0..334effb5e 100644 --- a/sentry-ruby/spec/sentry/transport_spec.rb +++ b/sentry-ruby/spec/sentry/transport_spec.rb @@ -46,19 +46,37 @@ let(:transaction) do Sentry::Transaction.new(name: "test transaction", op: "rack.request", hub: hub) end - let(:event) { client.event_from_transaction(transaction) } + + let(:dynamic_sampling_context) do + { + "sample_rate" => "0.01337", + "public_key" => "49d0f7386ad645858ae85020e393bef3", + "trace_id" => "771a43a4192642f0b136d5159a501700", + "user_id" => "Amélie" + } + end + + let(:event) do + event = client.event_from_transaction(transaction) + event.dynamic_sampling_context = dynamic_sampling_context + event + end + let(:envelope) { subject.envelope_from_event(event) } it "generates correct envelope content" do result, _ = subject.serialize_envelope(envelope) envelope_header, item_header, item = result.split("\n") + envelope_header_parsed = JSON.parse(envelope_header) - expect(envelope_header).to eq( - <<~ENVELOPE_HEADER.chomp - {"event_id":"#{event.event_id}","dsn":"#{Sentry::TestHelper::DUMMY_DSN}","sdk":#{Sentry.sdk_meta.to_json},"sent_at":"#{Time.now.utc.iso8601}"} - ENVELOPE_HEADER - ) + expect(envelope_header_parsed).to eq({ + "event_id" => event.event_id, + "dsn" => Sentry::TestHelper::DUMMY_DSN, + "sdk" => Sentry.sdk_meta, + "sent_at" => Time.now.utc.iso8601, + "trace" => dynamic_sampling_context + }) expect(item_header).to eq( '{"type":"transaction","content_type":"application/json"}' @@ -66,26 +84,6 @@ expect(item).to eq(event.to_hash.to_json) end - - context "when dynamic_sampling_context present" do - before do - event.dynamic_sampling_context = { - "sample_rate" => "0.01337", - "public_key" => "49d0f7386ad645858ae85020e393bef3", - "trace_id" => "771a43a4192642f0b136d5159a501700", - "user_id" => "Amélie" - } - end - - it "adds the trace header to envelope" do - result, _ = subject.serialize_envelope(envelope) - envelope_header, _, _ = result.split("\n") - header_parsed = JSON.parse(envelope_header) - - expect(header_parsed).to include("trace") - expect(header_parsed["trace"]).to eq(event.dynamic_sampling_context) - end - end end context "client report" do From 6c0df8d10f5c6908593b6f5664134f193b5fd277 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Fri, 16 Sep 2022 19:39:46 +0200 Subject: [PATCH 03/18] Transaction specs for head SDK baggage --- sentry-ruby/lib/sentry/baggage.rb | 6 + sentry-ruby/lib/sentry/transaction.rb | 6 + sentry-ruby/spec/sentry/transaction_spec.rb | 128 ++++++++++++++++++++ 3 files changed, 140 insertions(+) diff --git a/sentry-ruby/lib/sentry/baggage.rb b/sentry-ruby/lib/sentry/baggage.rb index 33c275bd3..6192fe2f5 100644 --- a/sentry-ruby/lib/sentry/baggage.rb +++ b/sentry-ruby/lib/sentry/baggage.rb @@ -19,6 +19,12 @@ class Baggage user_segment ).freeze + # @return [Hash] + attr_reader :sentry_items + + # @return [String] + attr_reader :third_party_items + # @return [Boolean] attr_reader :mutable diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index f0f91659c..fd076323e 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -24,6 +24,12 @@ class Transaction < Span # @return [String] attr_reader :parent_sampled + # The parsed incoming W3C baggage header. + # This is only for accessing the current baggage variable. + # Please use the #get_baggage method for interfacing outside this class. + # @return [Baggage, nil] + attr_reader :baggage + # @deprecated Use Sentry.get_current_hub instead. attr_reader :hub diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index 7ac37b2f2..25f58516c 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -20,6 +20,15 @@ describe ".from_sentry_trace" do let(:sentry_trace) { subject.to_sentry_trace } + let(:baggage) { + "other-vendor-value-1=foo;bar;baz, "\ + "sentry-trace_id=771a43a4192642f0b136d5159a501700, "\ + "sentry-public_key=49d0f7386ad645858ae85020e393bef3, "\ + "sentry-sample_rate=0.01337, "\ + "sentry-user_id=Am%C3%A9lie, "\ + "other-vendor-value-2=foo;bar;" + } + let(:configuration) do Sentry.configuration end @@ -45,6 +54,31 @@ expect(child_transaction).to be_nil end + + it "stores frozen empty baggage on incoming traces from older SDKs" do + child_transaction = described_class.from_sentry_trace(sentry_trace, baggage: nil, op: "child") + expect(child_transaction.baggage).not_to be_nil + expect(child_transaction.baggage.mutable).to be(false) + expect(child_transaction.baggage.sentry_items).to eq({}) + expect(child_transaction.baggage.third_party_items).to eq("") + end + + it "stores correct baggage on incoming baggage header" do + child_transaction = described_class.from_sentry_trace(sentry_trace, baggage: baggage, op: "child") + expect(child_transaction.baggage).not_to be_nil + expect(child_transaction.baggage.mutable).to be(false) + + expect(child_transaction.baggage.sentry_items).to eq({ + "sample_rate" => "0.01337", + "public_key" => "49d0f7386ad645858ae85020e393bef3", + "trace_id" => "771a43a4192642f0b136d5159a501700", + "user_id" => "Amélie" + }) + + expect(child_transaction.baggage.third_party_items).to eq( + "other-vendor-value-1=foo;bar;baz,other-vendor-value-2=foo;bar;" + ) + end end context "when tracing is enabled (= 0)" do @@ -178,10 +212,12 @@ transaction = described_class.new(sampled: true, hub: Sentry.get_current_hub) transaction.set_initial_sample_decision(sampling_context: {}) expect(transaction.sampled).to eq(true) + expect(transaction.effective_sample_rate).to eq(1.0) transaction = described_class.new(sampled: false, hub: Sentry.get_current_hub) transaction.set_initial_sample_decision(sampling_context: {}) expect(transaction.sampled).to eq(false) + expect(transaction.effective_sample_rate).to eq(0.0) end end @@ -195,6 +231,7 @@ subject.set_initial_sample_decision(sampling_context: { parent_sampled: false }) expect(subject.sampled).to eq(false) + expect(subject.effective_sample_rate).to eq(0.0) end it "uses traces_sample_rate for sampling (positive result)" do @@ -202,6 +239,7 @@ subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(true) + expect(subject.effective_sample_rate).to eq(0.5) expect(string_io.string).to include( "[Tracing] Starting transaction" ) @@ -212,6 +250,7 @@ subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(false) + expect(subject.effective_sample_rate).to eq(0.5) expect(string_io.string).to include( "[Tracing] Discarding transaction because it's not included in the random sample (sampling rate = 0.5)" ) @@ -222,6 +261,7 @@ subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(true) + expect(subject.effective_sample_rate).to eq(1.0) end end @@ -232,6 +272,7 @@ subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(false) + expect(subject.effective_sample_rate).to eq(0.0) end it "prioritizes traces_sampler over inherited decision" do @@ -239,6 +280,7 @@ subject.set_initial_sample_decision(sampling_context: { parent_sampled: true }) expect(subject.sampled).to eq(false) + expect(subject.effective_sample_rate).to eq(0.0) end it "ignores the sampler if it's not callable" do @@ -267,16 +309,19 @@ subject = described_class.new(hub: Sentry.get_current_hub) subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(true) + expect(subject.effective_sample_rate).to eq(1.0) Sentry.configuration.traces_sampler = -> (_) { 1.0 } subject = described_class.new(hub: Sentry.get_current_hub) subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(true) + expect(subject.effective_sample_rate).to eq(1.0) Sentry.configuration.traces_sampler = -> (_) { 1 } subject = described_class.new(hub: Sentry.get_current_hub) subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(true) + expect(subject.effective_sample_rate).to eq(1.0) expect(string_io.string).to include( "[Tracing] Starting transaction" @@ -290,11 +335,13 @@ subject = described_class.new(hub: Sentry.get_current_hub) subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(false) + expect(subject.effective_sample_rate).to eq(0.0) Sentry.configuration.traces_sampler = -> (_) { 0.0 } subject = described_class.new(hub: Sentry.get_current_hub) subject.set_initial_sample_decision(sampling_context: {}) expect(subject.sampled).to eq(false) + expect(subject.effective_sample_rate).to eq(0.0) expect(string_io.string).to include( "[Tracing] Discarding transaction because traces_sampler returned 0 or false" @@ -410,4 +457,85 @@ end end end + + describe "#get_baggage" do + subject do + transaction = described_class.new( + sampled: true, + parent_sampled: true, + name: "foo", + hub: Sentry.get_current_hub, + baggage: incoming_baggage + ) + + transaction.set_initial_sample_decision(sampling_context: {}) + transaction + end + + context "when no incoming baggage" do + let(:incoming_baggage) { nil } + + it "populates baggage as head SDK" do + expect(subject).to receive(:populate_head_baggage).and_call_original + + baggage = subject.get_baggage + expect(baggage.mutable).to eq(false) + expect(baggage.third_party_items).to eq("") + expect(baggage.sentry_items).to eq({ + "environment" => "development", + "public_key" => "12345", + "trace_id" => subject.trace_id, + "transaction"=>"foo" + }) + end + end + + context "when incoming empty frozen baggage from old SDK" do + let(:incoming_baggage) { Sentry::Baggage.new({}, mutable: false) } + + it "does not populate new baggage" do + expect(subject).not_to receive(:populate_head_baggage) + + baggage = subject.get_baggage + expect(baggage.mutable).to eq(false) + expect(baggage.third_party_items).to eq("") + expect(baggage.sentry_items).to eq({}) + end + end + + context "when incoming baggage with sentry items" do + let(:incoming_baggage) do + Sentry::Baggage.from_incoming_header("sentry-trace_id=12345,foo=bar") + end + + it "does not populate new baggage" do + expect(subject).not_to receive(:populate_head_baggage) + + baggage = subject.get_baggage + expect(baggage.mutable).to eq(false) + expect(baggage.third_party_items).to eq("foo=bar") + expect(baggage.sentry_items).to eq({ "trace_id" => "12345" }) + end + end + + context "when incoming baggage with no sentry items" do + let(:incoming_baggage) do + Sentry::Baggage.from_incoming_header("foo=bar") + end + + it "populates sentry baggage" do + expect(subject).to receive(:populate_head_baggage).and_call_original + + baggage = subject.get_baggage + expect(baggage.mutable).to eq(false) + expect(baggage.third_party_items).to eq("") + expect(baggage.sentry_items).to eq({ + "environment" => "development", + "public_key" => "12345", + "trace_id" => subject.trace_id, + "transaction"=>"foo" + }) + end + end + end end From ef80c2fb24474d5374a4861a2023b50cd489f1c6 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 19 Sep 2022 13:04:45 +0100 Subject: [PATCH 04/18] Review fixes --- sentry-ruby/lib/sentry/baggage.rb | 7 +-- sentry-ruby/lib/sentry/transaction.rb | 62 ++++++++++----------- sentry-ruby/spec/sentry/client_spec.rb | 2 + sentry-ruby/spec/sentry/transaction_spec.rb | 21 ++----- 4 files changed, 37 insertions(+), 55 deletions(-) diff --git a/sentry-ruby/lib/sentry/baggage.rb b/sentry-ruby/lib/sentry/baggage.rb index 6192fe2f5..7b7cc445b 100644 --- a/sentry-ruby/lib/sentry/baggage.rb +++ b/sentry-ruby/lib/sentry/baggage.rb @@ -20,10 +20,7 @@ class Baggage ).freeze # @return [Hash] - attr_reader :sentry_items - - # @return [String] - attr_reader :third_party_items + attr_reader :items # @return [Boolean] attr_reader :mutable @@ -41,8 +38,6 @@ def initialize(items, mutable: true) # @param header [String] The incoming Baggage header string. # @return [Baggage, nil] def self.from_incoming_header(header) - return nil if header.nil? || header.empty? - items = {} mutable = true diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index fd076323e..b0a2ba225 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -56,6 +56,9 @@ def initialize(name: nil, parent_sampled: nil, baggage: nil, hub:, **options) @traces_sampler = hub.configuration.traces_sampler @traces_sample_rate = hub.configuration.traces_sample_rate @logger = hub.configuration.logger + @release = hub.configuration.release + @environment = hub.configuration.environment + @dsn = hub.configuration.dsn @effective_sample_rate = nil init_span_recorder end @@ -85,11 +88,15 @@ def self.from_sentry_trace(sentry_trace, baggage: nil, hub: Sentry.get_current_h sampled_flag != "0" end - # If there's an incoming sentry-trace but no incoming baggage header, - # for instance in traces coming from older SDKs, - # baggage will be empty and frozen and won't be populated as head SDK. - baggage = Baggage.from_incoming_header(baggage) if baggage - baggage ||= Baggage.new({}) + baggage = if baggage && !baggage.empty? + Baggage.from_incoming_header(baggage) + else + # If there's an incoming sentry-trace but no incoming baggage header, + # for instance in traces coming from older SDKs, + # baggage will be empty and frozen and won't be populated as head SDK. + Baggage.new({}) + end + baggage.freeze! new( @@ -148,18 +155,16 @@ def set_initial_sample_decision(sampling_context:) transaction_description = generate_transaction_description - unless [true, false].include?(sample_rate) || (sample_rate.is_a?(Numeric) && sample_rate >= 0.0 && sample_rate <= 1.0) + if [true, false].include?(sample_rate) + @effective_sample_rate = sample_rate ? 1.0 : 0.0 + elsif sample_rate.is_a?(Numeric) && sample_rate >= 0.0 && sample_rate <= 1.0 + @effective_sample_rate = sample_rate.to_f + else @sampled = false log_warn("#{MESSAGE_PREFIX} Discarding #{transaction_description} because of invalid sample_rate: #{sample_rate}") return end - @effective_sample_rate = if [true, false].include?(sample_rate) - sample_rate ? 1.0 : 0.0 - else - sample_rate.to_f - end - if sample_rate == 0.0 || sample_rate == false @sampled = false log_debug("#{MESSAGE_PREFIX} Discarding #{transaction_description} because traces_sampler returned 0 or false") @@ -235,31 +240,20 @@ def generate_transaction_description end def populate_head_baggage - sentry_items = {} - sentry_items["trace_id"] = trace_id - - # TODO-neel filter for high cardinality / low quality tx source here - sentry_items["transaction"] = name - sentry_items["sample_rate"] = effective_sample_rate&.to_s - - configuration = Sentry.configuration - - if configuration - sentry_items["environment"] = configuration.environment - sentry_items["release"] = configuration.release - sentry_items["public_key"] = configuration.dsn&.public_key - end + items = { + "trace_id" => trace_id, + "transaction" => name,# TODO-neel filter for high cardinality tx source + "sample_rate" => effective_sample_rate&.to_s, + "environment" => @environment, + "release" => @release, + "public_key" => @dsn&.public_key + } user = Sentry.get_current_scope&.user - sentry_items["user_segment"] = user["segment"] if user && user["segment"] - - # there's an existing baggage but it was mutable, - # which is why we are creating this new baggage. - # However, if by chance the user put some sentry items in there, give them precedence. - sentry_items.merge!(@baggage.sentry_items) if @baggage && !@baggage.sentry_items.empty? - sentry_items.compact! + items["user_segment"] = user["segment"] if user && user["segment"] - @baggage = Baggage.new(sentry_items, mutable: false) + items.compact! + @baggage = Baggage.new(items, mutable: false) end class SpanRecorder diff --git a/sentry-ruby/spec/sentry/client_spec.rb b/sentry-ruby/spec/sentry/client_spec.rb index 204bb01cc..6283b7a92 100644 --- a/sentry-ruby/spec/sentry/client_spec.rb +++ b/sentry-ruby/spec/sentry/client_spec.rb @@ -144,6 +144,8 @@ def sentry_context event = subject.event_from_transaction(transaction) expect(event.dynamic_sampling_context).to eq({ + "environment" => "development", + "public_key" => "12345", "sample_rate" => "1.0", "transaction" => "test transaction", "trace_id" => transaction.trace_id diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index 25f58516c..3c7051205 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -59,8 +59,7 @@ child_transaction = described_class.from_sentry_trace(sentry_trace, baggage: nil, op: "child") expect(child_transaction.baggage).not_to be_nil expect(child_transaction.baggage.mutable).to be(false) - expect(child_transaction.baggage.sentry_items).to eq({}) - expect(child_transaction.baggage.third_party_items).to eq("") + expect(child_transaction.baggage.items).to eq({}) end it "stores correct baggage on incoming baggage header" do @@ -68,16 +67,12 @@ expect(child_transaction.baggage).not_to be_nil expect(child_transaction.baggage.mutable).to be(false) - expect(child_transaction.baggage.sentry_items).to eq({ + expect(child_transaction.baggage.items).to eq({ "sample_rate" => "0.01337", "public_key" => "49d0f7386ad645858ae85020e393bef3", "trace_id" => "771a43a4192642f0b136d5159a501700", "user_id" => "Amélie" }) - - expect(child_transaction.baggage.third_party_items).to eq( - "other-vendor-value-1=foo;bar;baz,other-vendor-value-2=foo;bar;" - ) end end @@ -480,8 +475,7 @@ baggage = subject.get_baggage expect(baggage.mutable).to eq(false) - expect(baggage.third_party_items).to eq("") - expect(baggage.sentry_items).to eq({ + expect(baggage.items).to eq({ "environment" => "development", "public_key" => "12345", "trace_id" => subject.trace_id, @@ -498,8 +492,7 @@ baggage = subject.get_baggage expect(baggage.mutable).to eq(false) - expect(baggage.third_party_items).to eq("") - expect(baggage.sentry_items).to eq({}) + expect(baggage.items).to eq({}) end end @@ -513,8 +506,7 @@ baggage = subject.get_baggage expect(baggage.mutable).to eq(false) - expect(baggage.third_party_items).to eq("foo=bar") - expect(baggage.sentry_items).to eq({ "trace_id" => "12345" }) + expect(baggage.items).to eq({ "trace_id" => "12345" }) end end @@ -528,8 +520,7 @@ baggage = subject.get_baggage expect(baggage.mutable).to eq(false) - expect(baggage.third_party_items).to eq("") - expect(baggage.sentry_items).to eq({ + expect(baggage.items).to eq({ "environment" => "development", "public_key" => "12345", "trace_id" => subject.trace_id, From 897c5e7f5fee6d0024f5eb2e99ffa4c7c5aeb1fe Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 19 Sep 2022 14:16:13 +0100 Subject: [PATCH 05/18] Changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 328202561..2df74f2fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,9 +10,10 @@ Note that this is not supported for users still using the `config.async` option. - - Parse incoming [W3C Baggage Headers](https://www.w3.org/TR/baggage/) and propagate them [#1869](https://github.com/getsentry/sentry-ruby/pull/1869) + - Parse incoming [W3C Baggage Headers](https://www.w3.org/TR/baggage/) and propagate them to continue traces [#1869](https://github.com/getsentry/sentry-ruby/pull/1869) - in all outgoing requests in our net/http patch - in Sentry transactions as [Dynamic Sampling Context](https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/) + - Create new Baggage entries as Head SDK (originator of trace) [#1898](https://github.com/getsentry/sentry-ruby/pull/1898) ### Bug Fixes From be5f9f550f41f7a77748938bd693b5216896f475 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 19 Sep 2022 15:29:51 +0100 Subject: [PATCH 06/18] Add source to transaction and add to transaction_info in event --- sentry-ruby/lib/sentry/event.rb | 2 +- sentry-ruby/lib/sentry/scope.rb | 2 +- sentry-ruby/lib/sentry/transaction.rb | 39 +++++++++++++++++++-- sentry-ruby/lib/sentry/transaction_event.rb | 1 + 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/sentry-ruby/lib/sentry/event.rb b/sentry-ruby/lib/sentry/event.rb index e600b9c18..0a76f3f4a 100644 --- a/sentry-ruby/lib/sentry/event.rb +++ b/sentry-ruby/lib/sentry/event.rb @@ -18,7 +18,7 @@ class Event event_id level timestamp release environment server_name modules message user tags contexts extra - fingerprint breadcrumbs transaction + fingerprint breadcrumbs transaction transaction_info platform sdk type ) diff --git a/sentry-ruby/lib/sentry/scope.rb b/sentry-ruby/lib/sentry/scope.rb index 55630a656..75cf2c93b 100644 --- a/sentry-ruby/lib/sentry/scope.rb +++ b/sentry-ruby/lib/sentry/scope.rb @@ -7,7 +7,7 @@ module Sentry class Scope include ArgumentCheckingHelper - ATTRIBUTES = [:transaction_names, :contexts, :extra, :tags, :user, :level, :breadcrumbs, :fingerprint, :event_processors, :rack_env, :span, :session] + ATTRIBUTES = [:transaction_names, :transaction_sources, :contexts, :extra, :tags, :user, :level, :breadcrumbs, :fingerprint, :event_processors, :rack_env, :span, :session] attr_reader(*ATTRIBUTES) diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index b0a2ba225..3a55ce36f 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -14,12 +14,24 @@ class Transaction < Span UNLABELD_NAME = "".freeze MESSAGE_PREFIX = "[Tracing]" + # https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations + SOURCE_CUSTOM = "custom" + SOURCE_URL = "url" + SOURCE_ROUTE = "route" + SOURCE_VIEW = "view" + SOURCE_COMPONENT = "component" + SOURCE_TASK = "task" + include LoggingHelper # The name of the transaction. # @return [String] attr_reader :name + # The source of the transaction name. + # @return [String] + attr_reader :source + # The sampling decision of the parent transaction, which will be considered when making the current transaction's sampling decision. # @return [String] attr_reader :parent_sampled @@ -43,10 +55,18 @@ class Transaction < Span # @return [Float, nil] attr_reader :effective_sample_rate - def initialize(name: nil, parent_sampled: nil, baggage: nil, hub:, **options) + def initialize( + hub:, + name: nil, + source: SOURCE_CUSTOM, + parent_sampled: nil, + baggage: nil, + **options + ) super(**options) @name = name + @source = source @parent_sampled = parent_sampled @transaction = self @hub = hub @@ -112,7 +132,14 @@ def self.from_sentry_trace(sentry_trace, baggage: nil, hub: Sentry.get_current_h # @return [Hash] def to_hash hash = super - hash.merge!(name: @name, sampled: @sampled, parent_sampled: @parent_sampled) + + hash.merge!( + name: @name, + source: @source, + sampled: @sampled, + parent_sampled: @parent_sampled + ) + hash end @@ -242,13 +269,14 @@ def generate_transaction_description def populate_head_baggage items = { "trace_id" => trace_id, - "transaction" => name,# TODO-neel filter for high cardinality tx source "sample_rate" => effective_sample_rate&.to_s, "environment" => @environment, "release" => @release, "public_key" => @dsn&.public_key } + items["transaction"] = name unless source_low_quality? + user = Sentry.get_current_scope&.user items["user_segment"] = user["segment"] if user && user["segment"] @@ -256,6 +284,11 @@ def populate_head_baggage @baggage = Baggage.new(items, mutable: false) end + # These are high cardinality and thus bad + def source_low_quality? + source == SOURCE_URL + end + class SpanRecorder attr_reader :max_length, :spans diff --git a/sentry-ruby/lib/sentry/transaction_event.rb b/sentry-ruby/lib/sentry/transaction_event.rb index 4e6092112..06f89488f 100644 --- a/sentry-ruby/lib/sentry/transaction_event.rb +++ b/sentry-ruby/lib/sentry/transaction_event.rb @@ -18,6 +18,7 @@ def initialize(transaction:, **options) super(**options) self.transaction = transaction.name + self.transaction_info = { source: transaction.source } self.contexts.merge!(trace: transaction.get_trace_context) self.timestamp = transaction.timestamp self.start_timestamp = transaction.start_timestamp From 878db4cdb60fea36e080f04b7c38b82265aa6227 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 19 Sep 2022 15:40:23 +0100 Subject: [PATCH 07/18] Add source to scope.set_transaction_name --- sentry-ruby/lib/sentry/scope.rb | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/sentry-ruby/lib/sentry/scope.rb b/sentry-ruby/lib/sentry/scope.rb index 75cf2c93b..4dc3bfbf2 100644 --- a/sentry-ruby/lib/sentry/scope.rb +++ b/sentry-ruby/lib/sentry/scope.rb @@ -7,7 +7,21 @@ module Sentry class Scope include ArgumentCheckingHelper - ATTRIBUTES = [:transaction_names, :transaction_sources, :contexts, :extra, :tags, :user, :level, :breadcrumbs, :fingerprint, :event_processors, :rack_env, :span, :session] + ATTRIBUTES = [ + :transaction_names, + :transaction_sources, + :contexts, + :extra, + :tags, + :user, + :level, + :breadcrumbs, + :fingerprint, + :event_processors, + :rack_env, + :span, + :session + ] attr_reader(*ATTRIBUTES) @@ -74,6 +88,7 @@ def dup copy.tags = tags.deep_dup copy.user = user.deep_dup copy.transaction_names = transaction_names.deep_dup + copy.transaction_sources = transaction_sources.deep_dup copy.fingerprint = fingerprint.deep_dup copy.span = span.deep_dup copy.session = session.deep_dup @@ -90,6 +105,7 @@ def update_from_scope(scope) self.tags = scope.tags self.user = scope.user self.transaction_names = scope.transaction_names + self.transaction_sources = scope.transaction_sources self.fingerprint = scope.fingerprint self.span = scope.span end @@ -195,8 +211,9 @@ def set_level(level) # The "transaction" here does not refer to `Transaction` objects. # @param transaction_name [String] # @return [void] - def set_transaction_name(transaction_name) + def set_transaction_name(transaction_name, source: Transaction::SOURCE_CUSTOM) @transaction_names << transaction_name + @transaction_sources << source end # Sets the currently active session on the scope. @@ -213,6 +230,13 @@ def transaction_name @transaction_names.last end + # Returns current transaction source. + # The "transaction" here does not refer to `Transaction` objects. + # @return [String, nil] + def transaction_source + @transaction_sources.last + end + # Returns the associated Transaction object. # @return [Transaction, nil] def get_transaction @@ -256,6 +280,7 @@ def set_default_value @level = :error @fingerprint = [] @transaction_names = [] + @transaction_sources = [] @event_processors = [] @rack_env = {} @span = nil From e2dd2f13e7d7a4fd5980391f8e307cfaca7f78b5 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 19 Sep 2022 15:50:43 +0100 Subject: [PATCH 08/18] Change to symbol and annotate rake and rack --- sentry-ruby/lib/sentry/rack/capture_exceptions.rb | 4 ++-- sentry-ruby/lib/sentry/rake.rb | 2 +- sentry-ruby/lib/sentry/scope.rb | 2 +- sentry-ruby/lib/sentry/transaction.rb | 13 ++++--------- 4 files changed, 8 insertions(+), 13 deletions(-) diff --git a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb index 38f954b1e..f7e1c9d95 100644 --- a/sentry-ruby/lib/sentry/rack/capture_exceptions.rb +++ b/sentry-ruby/lib/sentry/rack/capture_exceptions.rb @@ -18,7 +18,7 @@ def call(env) Sentry.with_scope do |scope| Sentry.with_session_tracking do scope.clear_breadcrumbs - scope.set_transaction_name(env["PATH_INFO"]) if env["PATH_INFO"] + scope.set_transaction_name(env["PATH_INFO"], source: :url) if env["PATH_INFO"] scope.set_rack_env(env) transaction = start_transaction(env, scope) @@ -65,7 +65,7 @@ def start_transaction(env, scope) sentry_trace = env["HTTP_SENTRY_TRACE"] baggage = env["HTTP_BAGGAGE"] - options = { name: scope.transaction_name, op: transaction_op } + options = { name: scope.transaction_name, source: scope.transaction_source, op: transaction_op } transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, baggage: baggage, **options) if sentry_trace Sentry.start_transaction(transaction: transaction, custom_sampling_context: { env: env }, **options) end diff --git a/sentry-ruby/lib/sentry/rake.rb b/sentry-ruby/lib/sentry/rake.rb index 8f316ce9e..62e3b4bee 100644 --- a/sentry-ruby/lib/sentry/rake.rb +++ b/sentry-ruby/lib/sentry/rake.rb @@ -10,7 +10,7 @@ module Application def display_error_message(ex) Sentry.capture_exception(ex) do |scope| task_name = top_level_tasks.join(' ') - scope.set_transaction_name(task_name) + scope.set_transaction_name(task_name, source: :task) scope.set_tag("rake_task", task_name) end if Sentry.initialized? && !Sentry.configuration.skip_rake_integration diff --git a/sentry-ruby/lib/sentry/scope.rb b/sentry-ruby/lib/sentry/scope.rb index 4dc3bfbf2..f2083712c 100644 --- a/sentry-ruby/lib/sentry/scope.rb +++ b/sentry-ruby/lib/sentry/scope.rb @@ -211,7 +211,7 @@ def set_level(level) # The "transaction" here does not refer to `Transaction` objects. # @param transaction_name [String] # @return [void] - def set_transaction_name(transaction_name, source: Transaction::SOURCE_CUSTOM) + def set_transaction_name(transaction_name, source: :custom) @transaction_names << transaction_name @transaction_sources << source end diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index 3a55ce36f..28ce97c72 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -15,12 +15,7 @@ class Transaction < Span MESSAGE_PREFIX = "[Tracing]" # https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations - SOURCE_CUSTOM = "custom" - SOURCE_URL = "url" - SOURCE_ROUTE = "route" - SOURCE_VIEW = "view" - SOURCE_COMPONENT = "component" - SOURCE_TASK = "task" + SOURCES = %i(custom url route view component task) include LoggingHelper @@ -58,7 +53,7 @@ class Transaction < Span def initialize( hub:, name: nil, - source: SOURCE_CUSTOM, + source: :custom, parent_sampled: nil, baggage: nil, **options @@ -66,7 +61,7 @@ def initialize( super(**options) @name = name - @source = source + @source = SOURCES.include?(source) ? source : :custom @parent_sampled = parent_sampled @transaction = self @hub = hub @@ -286,7 +281,7 @@ def populate_head_baggage # These are high cardinality and thus bad def source_low_quality? - source == SOURCE_URL + source == :url end class SpanRecorder From fd85d965a04ed0827a4e8737abbf611f88b742b9 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 19 Sep 2022 16:04:37 +0100 Subject: [PATCH 09/18] Add source :task annotation to delayed_job/sidekiq/resque --- sentry-delayed_job/lib/sentry/delayed_job/plugin.rb | 5 +++-- sentry-resque/lib/sentry/resque.rb | 5 +++-- sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb | 2 +- .../lib/sentry/sidekiq/sentry_context_middleware.rb | 8 ++++---- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/sentry-delayed_job/lib/sentry/delayed_job/plugin.rb b/sentry-delayed_job/lib/sentry/delayed_job/plugin.rb index 8949c494f..9754a3bdb 100644 --- a/sentry-delayed_job/lib/sentry/delayed_job/plugin.rb +++ b/sentry-delayed_job/lib/sentry/delayed_job/plugin.rb @@ -14,11 +14,12 @@ class Plugin < ::Delayed::Plugin Sentry.with_scope do |scope| contexts = generate_contexts(job) - scope.set_transaction_name(contexts.dig(ACTIVE_JOB_CONTEXT_KEY, :job_class) || contexts.dig(DELAYED_JOB_CONTEXT_KEY, :job_class)) + name = contexts.dig(ACTIVE_JOB_CONTEXT_KEY, :job_class) || contexts.dig(DELAYED_JOB_CONTEXT_KEY, :job_class) + scope.set_transaction_name(name, source: :task) scope.set_contexts(**contexts) scope.set_tags("delayed_job.queue" => job.queue, "delayed_job.id" => job.id.to_s) - transaction = Sentry.start_transaction(name: scope.transaction_name, op: "delayed_job") + transaction = Sentry.start_transaction(name: scope.transaction_name, source: scope.transaction_source, op: "delayed_job") scope.set_span(transaction) if transaction begin diff --git a/sentry-resque/lib/sentry/resque.rb b/sentry-resque/lib/sentry/resque.rb index 9b7204805..1cf7b3da1 100644 --- a/sentry-resque/lib/sentry/resque.rb +++ b/sentry-resque/lib/sentry/resque.rb @@ -23,8 +23,9 @@ def record(queue, worker, payload, &block) scope.set_contexts(**contexts) scope.set_tags("resque.queue" => queue) - scope.set_transaction_name(contexts.dig(:"Active-Job", :job_class) || contexts.dig(:"Resque", :job_class)) - transaction = Sentry.start_transaction(name: scope.transaction_name, op: "resque") + name = contexts.dig(:"Active-Job", :job_class) || contexts.dig(:"Resque", :job_class) + scope.set_transaction_name(name: name, source: :task) + transaction = Sentry.start_transaction(name: scope.transaction_name, source: scope.transaction_source, op: "resque") scope.set_span(transaction) if transaction yield diff --git a/sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb b/sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb index 4ac2cdff7..ec336cc39 100644 --- a/sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb +++ b/sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb @@ -9,7 +9,7 @@ def call(ex, context) context_filter = Sentry::Sidekiq::ContextFilter.new(context) scope = Sentry.get_current_scope - scope.set_transaction_name(context_filter.transaction_name) unless scope.transaction_name + scope.set_transaction_name(context_filter.transaction_name, source: :task) unless scope.transaction_name if Sentry.configuration.sidekiq.report_after_job_retries && retryable?(context) retry_count = context.dig(:job, "retry_count") diff --git a/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb b/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb index eb10a6ad8..2c1f9e084 100644 --- a/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb +++ b/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb @@ -16,8 +16,8 @@ def call(_worker, job, queue) scope.set_tags(queue: queue, jid: job["jid"]) scope.set_tags(build_tags(job["tags"])) scope.set_contexts(sidekiq: job.merge("queue" => queue)) - scope.set_transaction_name(context_filter.transaction_name) - transaction = start_transaction(scope.transaction_name, job["sentry_trace"]) + scope.set_transaction_name(context_filter.transaction_name, source: :task) + transaction = start_transaction(scope.transaction_name, scope.transaction_source, job["sentry_trace"]) scope.set_span(transaction) if transaction begin @@ -37,8 +37,8 @@ def build_tags(tags) Array(tags).each_with_object({}) { |name, tags_hash| tags_hash[:"sidekiq.#{name}"] = true } end - def start_transaction(transaction_name, sentry_trace) - options = { name: transaction_name, op: "sidekiq" } + def start_transaction(transaction_name, transaction_source, sentry_trace) + options = { name: transaction_name, source: transaction_source, op: "sidekiq" } transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, **options) if sentry_trace Sentry.start_transaction(transaction: transaction, **options) end From 0c051de962e8da53d137d2e42714d9c644d9354c Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Mon, 19 Sep 2022 16:31:57 +0100 Subject: [PATCH 10/18] Annotate rails transaction_source --- sentry-rails/lib/sentry/rails/action_cable.rb | 8 ++++---- sentry-rails/lib/sentry/rails/active_job.rb | 4 ++-- sentry-rails/lib/sentry/rails/capture_exceptions.rb | 9 +-------- sentry-rails/lib/sentry/rails/controller_transaction.rb | 2 +- sentry-resque/lib/sentry/resque.rb | 2 +- 5 files changed, 9 insertions(+), 16 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/action_cable.rb b/sentry-rails/lib/sentry/rails/action_cable.rb index da15e17ae..fa06e870e 100644 --- a/sentry-rails/lib/sentry/rails/action_cable.rb +++ b/sentry-rails/lib/sentry/rails/action_cable.rb @@ -13,8 +13,8 @@ def capture(connection, transaction_name:, extra_context: nil, &block) Sentry.with_scope do |scope| scope.set_rack_env(env) scope.set_context("action_cable", extra_context) if extra_context - scope.set_transaction_name(transaction_name) - transaction = start_transaction(env, scope.transaction_name) + scope.set_transaction_name(transaction_name, source: :view) + transaction = start_transaction(env, scope.transaction_name, scope.transaction_source) scope.set_span(transaction) if transaction begin @@ -29,11 +29,11 @@ def capture(connection, transaction_name:, extra_context: nil, &block) end end - def start_transaction(env, transaction_name) + def start_transaction(env, transaction_name, transaction_source) sentry_trace = env["HTTP_SENTRY_TRACE"] baggage = env["HTTP_BAGGAGE"] - options = { name: transaction_name, op: "rails.action_cable".freeze } + options = { name: transaction_name, source: transaction_source, op: "rails.action_cable".freeze } transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, baggage: baggage, **options) if sentry_trace Sentry.start_transaction(transaction: transaction, **options) end diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb index 171f997e1..9e732183f 100644 --- a/sentry-rails/lib/sentry/rails/active_job.rb +++ b/sentry-rails/lib/sentry/rails/active_job.rb @@ -20,12 +20,12 @@ class << self def record(job, &block) Sentry.with_scope do |scope| begin - scope.set_transaction_name(job.class.name) + scope.set_transaction_name(job.class.name, source: :task) transaction = if job.is_a?(::Sentry::SendEventJob) nil else - Sentry.start_transaction(name: scope.transaction_name, op: "active_job") + Sentry.start_transaction(name: scope.transaction_name, source: scope.transaction_source, op: "active_job") end scope.set_span(transaction) if transaction diff --git a/sentry-rails/lib/sentry/rails/capture_exceptions.rb b/sentry-rails/lib/sentry/rails/capture_exceptions.rb index 47c6ce4d1..6228c6385 100644 --- a/sentry-rails/lib/sentry/rails/capture_exceptions.rb +++ b/sentry-rails/lib/sentry/rails/capture_exceptions.rb @@ -25,13 +25,6 @@ def capture_exception(exception, env) # the exception will be swallowed by ShowExceptions middleware return if request.show_exceptions? && !Sentry.configuration.rails.report_rescued_exceptions - - current_scope = Sentry.get_current_scope - - if original_transaction = env["sentry.original_transaction"] - current_scope.set_transaction_name(original_transaction) - end - Sentry::Rails.capture_exception(exception).tap do |event| env[ERROR_EVENT_ID_KEY] = event.event_id if event end @@ -41,7 +34,7 @@ def start_transaction(env, scope) sentry_trace = env["HTTP_SENTRY_TRACE"] baggage = env["HTTP_BAGGAGE"] - options = { name: scope.transaction_name, op: transaction_op } + options = { name: scope.transaction_name, source: scope.transaction_source, op: transaction_op } if @assets_regex && scope.transaction_name.match?(@assets_regex) options.merge!(sampled: false) diff --git a/sentry-rails/lib/sentry/rails/controller_transaction.rb b/sentry-rails/lib/sentry/rails/controller_transaction.rb index 5b78aa759..c62cc4591 100644 --- a/sentry-rails/lib/sentry/rails/controller_transaction.rb +++ b/sentry-rails/lib/sentry/rails/controller_transaction.rb @@ -3,7 +3,7 @@ module Rails module ControllerTransaction def self.included(base) base.prepend_before_action do |controller| - Sentry.get_current_scope.set_transaction_name("#{controller.class}##{controller.action_name}") + Sentry.get_current_scope.set_transaction_name("#{controller.class}##{controller.action_name}", source: :view) end end end diff --git a/sentry-resque/lib/sentry/resque.rb b/sentry-resque/lib/sentry/resque.rb index 1cf7b3da1..a2c361cdd 100644 --- a/sentry-resque/lib/sentry/resque.rb +++ b/sentry-resque/lib/sentry/resque.rb @@ -24,7 +24,7 @@ def record(queue, worker, payload, &block) scope.set_tags("resque.queue" => queue) name = contexts.dig(:"Active-Job", :job_class) || contexts.dig(:"Resque", :job_class) - scope.set_transaction_name(name: name, source: :task) + scope.set_transaction_name(name, source: :task) transaction = Sentry.start_transaction(name: scope.transaction_name, source: scope.transaction_source, op: "resque") scope.set_span(transaction) if transaction From 829ee6ad800ce2aa4cde6ca4ff0db73d1916800b Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 20 Sep 2022 16:01:31 +0100 Subject: [PATCH 11/18] Specs WIP --- sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index e6d758af8..966ab3c0f 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb @@ -248,6 +248,8 @@ def inspect def verify_transaction_attributes(transaction) expect(transaction.type).to eq("transaction") + expect(transaction.transaction).to eq("/test") + expect(transaction.transaction_info).to eq({ source: :url }) 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") @@ -430,6 +432,8 @@ def will_be_sampled_by_sdk transaction = last_sentry_event expect(transaction.type).to eq("transaction") + expect(transaction.transaction).to eq("/test") + expect(transaction.transaction_info).to eq({ source: :url }) 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") @@ -453,6 +457,8 @@ def will_be_sampled_by_sdk transaction = last_sentry_event expect(transaction.type).to eq("transaction") expect(transaction.timestamp).not_to be_nil + expect(transaction.transaction).to eq("/test") + expect(transaction.transaction_info).to eq({ source: :url }) 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(2) From 2ed350168f39567a3d8e26a6ba53d0a674e8c7d9 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 27 Sep 2022 16:27:03 +0200 Subject: [PATCH 12/18] Don't override transaction/transaction_info in scope#apply_to_event --- sentry-ruby/lib/sentry/scope.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sentry-ruby/lib/sentry/scope.rb b/sentry-ruby/lib/sentry/scope.rb index f2083712c..d352a682d 100644 --- a/sentry-ruby/lib/sentry/scope.rb +++ b/sentry-ruby/lib/sentry/scope.rb @@ -46,7 +46,12 @@ def apply_to_event(event, hint = nil) event.user = user.merge(event.user) event.extra = extra.merge(event.extra) event.contexts = contexts.merge(event.contexts) - event.transaction = transaction_name if transaction_name + event.transaction ||= transaction_name if transaction_name + + if transaction_sources + event.transaction_info ||= {} + event.transaction_info = { source: transaction_source }.merge(event.transaction_info) + end if span event.contexts[:trace] = span.get_trace_context From 0a1f644d6f5189d018b58e137b3863c222520cff Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 27 Sep 2022 16:27:25 +0200 Subject: [PATCH 13/18] Scope spec --- sentry-ruby/spec/sentry/scope_spec.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/sentry-ruby/spec/sentry/scope_spec.rb b/sentry-ruby/spec/sentry/scope_spec.rb index 51cb30d45..45dc97e51 100644 --- a/sentry-ruby/spec/sentry/scope_spec.rb +++ b/sentry-ruby/spec/sentry/scope_spec.rb @@ -23,6 +23,7 @@ expect(subject.user).to eq({}) expect(subject.fingerprint).to eq([]) expect(subject.transaction_names).to eq([]) + expect(subject.transaction_sources).to eq([]) end it "allows setting breadcrumb buffer's size limit" do @@ -41,6 +42,7 @@ copy.tags.merge!(foo: "bar") copy.user.merge!(foo: "bar") copy.transaction_names << "foo" + copy.transaction_sources << :url copy.fingerprint << "bar" expect(subject.breadcrumbs.to_hash).to eq({ values: [] }) @@ -51,6 +53,7 @@ expect(subject.user).to eq({}) expect(subject.fingerprint).to eq([]) expect(subject.transaction_names).to eq([]) + expect(subject.transaction_sources).to eq([]) expect(subject.span).to eq(nil) end @@ -133,6 +136,7 @@ expect(subject.user).to eq({}) expect(subject.fingerprint).to eq([]) expect(subject.transaction_names).to eq([]) + expect(subject.transaction_sources).to eq([]) expect(subject.span).to eq(nil) end end @@ -188,7 +192,7 @@ scope.set_tags({foo: "bar"}) scope.set_extras({additional_info: "hello"}) scope.set_user({id: 1}) - scope.set_transaction_name("WelcomeController#index") + scope.set_transaction_name("WelcomeController#index", source: :view) scope.set_fingerprint(["foo"]) scope end @@ -203,6 +207,7 @@ expect(event.user).to eq({id: 1}) expect(event.extra).to eq({additional_info: "hello"}) expect(event.transaction).to eq("WelcomeController#index") + expect(event.transaction_info).to eq({ source: :view }) expect(event.breadcrumbs).to be_a(Sentry::BreadcrumbBuffer) expect(event.fingerprint).to eq(["foo"]) expect(event.contexts[:os].keys).to match_array([:name, :version, :build, :kernel_version]) @@ -214,12 +219,16 @@ event.user = {id: 2} event.extra = {additional_info: "nothing"} event.contexts = {os: nil} + event.transaction = "/some/url" + event.transaction_info = { source: :url } subject.apply_to_event(event) expect(event.tags).to eq({foo: "baz"}) expect(event.user).to eq({id: 2}) expect(event.extra[:additional_info]).to eq("nothing") expect(event.contexts[:os]).to eq(nil) + expect(event.transaction).to eq("/some/url") + expect(event.transaction_info).to eq({ source: :url }) end it "applies event processors to the event" do From 08240700c39db95083622eedefd746024dffb51e Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 27 Sep 2022 17:19:30 +0200 Subject: [PATCH 14/18] Transaction spec --- sentry-ruby/spec/sentry/transaction_spec.rb | 24 +++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index 3c7051205..2d4716f66 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -13,6 +13,7 @@ sampled: true, parent_sampled: true, name: "foo", + source: :view, hub: Sentry.get_current_hub ) end @@ -358,6 +359,7 @@ expect(hash[:sampled]).to eq(true) expect(hash[:parent_sampled]).to eq(true) expect(hash[:name]).to eq("foo") + expect(hash[:source]).to eq(:view) end end @@ -454,11 +456,18 @@ end describe "#get_baggage" do + before do + allow(Sentry.configuration).to receive(:tracing_enabled?).and_return(true) + end + + let(:source) { :view } + subject do transaction = described_class.new( sampled: true, parent_sampled: true, name: "foo", + source: source, hub: Sentry.get_current_hub, baggage: incoming_baggage ) @@ -479,9 +488,19 @@ "environment" => "development", "public_key" => "12345", "trace_id" => subject.trace_id, - "transaction"=>"foo" + "transaction"=>"foo", + "sample_rate" => "1.0" }) end + + context "when source is low quality" do + let(:source) { :url } + + it "populates baggage as head SDK without transaction" do + baggage = subject.get_baggage + expect(baggage.items).not_to include("transaction") + end + end end context "when incoming empty frozen baggage from old SDK" do @@ -524,7 +543,8 @@ "environment" => "development", "public_key" => "12345", "trace_id" => subject.trace_id, - "transaction"=>"foo" + "transaction"=>"foo", + "sample_rate" => "1.0" }) end end From 76b4a54d0f338005556a8d6c70d9c171220f52a7 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 27 Sep 2022 17:27:05 +0200 Subject: [PATCH 15/18] Source specs for all other gems --- sentry-delayed_job/spec/sentry/delayed_job_spec.rb | 14 ++++++++------ sentry-resque/spec/sentry/tracing_spec.rb | 2 ++ sentry-sidekiq/spec/sentry/sidekiq_spec.rb | 2 ++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/sentry-delayed_job/spec/sentry/delayed_job_spec.rb b/sentry-delayed_job/spec/sentry/delayed_job_spec.rb index b3d11ab24..a7b0ed402 100644 --- a/sentry-delayed_job/spec/sentry/delayed_job_spec.rb +++ b/sentry-delayed_job/spec/sentry/delayed_job_spec.rb @@ -248,7 +248,7 @@ def perform config.rails.skippable_job_adapters << "ActiveJob::QueueAdapters::DelayedJobAdapter" end end - + it "records transaction" do ReportingJob.perform_later @@ -257,13 +257,13 @@ def perform expect(transport.events.count).to eq(2) transaction = transport.events.last - + expect(transaction.transaction).to eq("ReportingJob") expect(transaction.contexts.dig(:trace, :trace_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :span_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :status)).to eq("ok") end - + it "records transaction with exception" do FailedJob.perform_later enqueued_job = Delayed::Backend::ActiveRecord::Job.last @@ -272,15 +272,15 @@ def perform rescue ZeroDivisionError nil end - + expect(transport.events.count).to eq(2) transaction = transport.events.last - + expect(transaction.transaction).to eq("FailedJob") expect(transaction.contexts.dig(:trace, :trace_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :span_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :status)).to eq("internal_error") - + event = transport.events.last expect(event.contexts.dig(:trace, :trace_id)).to eq(transaction.contexts.dig(:trace, :trace_id)) end @@ -325,6 +325,7 @@ def perform transaction = transport.events.last expect(transaction.transaction).to eq("Post#do_nothing") + expect(transaction.transaction_info).to eq({ source: :task }) expect(transaction.contexts.dig(:trace, :trace_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :span_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :status)).to eq("ok") @@ -343,6 +344,7 @@ def perform transaction = transport.events.last expect(transaction.transaction).to eq("Post#raise_error") + expect(transaction.transaction_info).to eq({ source: :task }) expect(transaction.contexts.dig(:trace, :trace_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :span_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :status)).to eq("internal_error") diff --git a/sentry-resque/spec/sentry/tracing_spec.rb b/sentry-resque/spec/sentry/tracing_spec.rb index 220c11d5b..3145e42f5 100644 --- a/sentry-resque/spec/sentry/tracing_spec.rb +++ b/sentry-resque/spec/sentry/tracing_spec.rb @@ -38,6 +38,7 @@ def self.perform(msg) tracing_event = transport.events.last.to_hash expect(tracing_event[:transaction]).to eq("MessageJob") + expect(tracing_event[:transaction_info]).to eq({ source: :task }) expect(tracing_event[:type]).to eq("transaction") expect(tracing_event.dig(:contexts, :trace, :status)).to eq("ok") expect(tracing_event.dig(:contexts, :trace, :op)).to eq("resque") @@ -54,6 +55,7 @@ def self.perform(msg) tracing_event = transport.events.last.to_hash expect(tracing_event[:transaction]).to eq("FailedJob") + expect(tracing_event[:transaction_info]).to eq({ source: :task }) expect(tracing_event[:type]).to eq("transaction") expect(tracing_event.dig(:contexts, :trace, :status)).to eq("internal_error") expect(tracing_event.dig(:contexts, :trace, :op)).to eq("resque") diff --git a/sentry-sidekiq/spec/sentry/sidekiq_spec.rb b/sentry-sidekiq/spec/sentry/sidekiq_spec.rb index 12f9a3072..677259332 100644 --- a/sentry-sidekiq/spec/sentry/sidekiq_spec.rb +++ b/sentry-sidekiq/spec/sentry/sidekiq_spec.rb @@ -195,6 +195,7 @@ def retry_last_failed_job transaction = transport.events.first expect(transaction.transaction).to eq("Sidekiq/HappyWorker") + expect(transaction.transaction_info).to eq({ source: :task }) expect(transaction.contexts.dig(:trace, :trace_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :span_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :status)).to eq("ok") @@ -207,6 +208,7 @@ def retry_last_failed_job transaction = transport.events.first expect(transaction.transaction).to eq("Sidekiq/SadWorker") + expect(transaction.transaction_info).to eq({ source: :task }) expect(transaction.contexts.dig(:trace, :trace_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :span_id)).to be_a(String) expect(transaction.contexts.dig(:trace, :status)).to eq("internal_error") From b630c5ce59af3deeeb552e27ee08e482171a33fc Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 27 Sep 2022 17:50:18 +0200 Subject: [PATCH 16/18] Rails specs; revert scope changes --- sentry-rails/spec/sentry/rails/action_cable_spec.rb | 12 ++++++++++++ sentry-rails/spec/sentry/rails/activejob_spec.rb | 2 ++ sentry-rails/spec/sentry/rails_spec.rb | 6 +++++- sentry-ruby/lib/sentry/scope.rb | 8 ++------ sentry-ruby/spec/sentry/scope_spec.rb | 4 ---- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/sentry-rails/spec/sentry/rails/action_cable_spec.rb b/sentry-rails/spec/sentry/rails/action_cable_spec.rb index 52521aafb..53e528c0e 100644 --- a/sentry-rails/spec/sentry/rails/action_cable_spec.rb +++ b/sentry-rails/spec/sentry/rails/action_cable_spec.rb @@ -76,6 +76,7 @@ def disconnect event = transport.events.last.to_json_compatible expect(event["transaction"]).to eq("FailToOpenConnection#connect") + expect(event["transaction_info"]).to eq({ "source" => "view" }) end end @@ -89,6 +90,7 @@ def disconnect event = transport.events.last.to_json_compatible expect(event["transaction"]).to eq("FailToCloseConnection#disconnect") + expect(event["transaction_info"]).to eq({ "source" => "view" }) end end end @@ -105,6 +107,7 @@ def disconnect event = transport.events.last.to_json_compatible expect(event["transaction"]).to eq("ChatChannel#subscribed") + expect(event["transaction_info"]).to eq({ "source" => "view" }) expect(event["contexts"]).to include("action_cable" => { "params" => { "room_id" => 42 } }) end end @@ -119,6 +122,7 @@ def disconnect event = transport.events.last.to_json_compatible expect(event["transaction"]).to eq("AppearanceChannel#appear") + expect(event["transaction_info"]).to eq({ "source" => "view" }) expect(event["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 }, @@ -134,6 +138,7 @@ def disconnect event = transport.events.last.to_json_compatible expect(event["transaction"]).to eq("AppearanceChannel#unsubscribed") + expect(event["transaction_info"]).to eq({ "source" => "view" }) expect(event["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 } @@ -164,6 +169,7 @@ def disconnect expect(transaction["type"]).to eq("transaction") expect(transaction["transaction"]).to eq("ChatChannel#subscribed") + expect(transaction["transaction_info"]).to eq({ "source" => "view" }) expect(transaction["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 } @@ -189,6 +195,7 @@ def disconnect expect(subscription_transaction["type"]).to eq("transaction") expect(subscription_transaction["transaction"]).to eq("AppearanceChannel#subscribed") + expect(subscription_transaction["transaction_info"]).to eq({ "source" => "view" }) expect(subscription_transaction["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 } @@ -204,6 +211,7 @@ def disconnect event = transport.events[1].to_json_compatible expect(event["transaction"]).to eq("AppearanceChannel#appear") + expect(event["transaction_info"]).to eq({ "source" => "view" }) expect(event["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 }, @@ -215,6 +223,7 @@ def disconnect expect(action_transaction["type"]).to eq("transaction") expect(action_transaction["transaction"]).to eq("AppearanceChannel#appear") + expect(action_transaction["transaction_info"]).to eq({ "source" => "view" }) expect(action_transaction["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 }, @@ -237,6 +246,7 @@ def disconnect expect(subscription_transaction["type"]).to eq("transaction") expect(subscription_transaction["transaction"]).to eq("AppearanceChannel#subscribed") + expect(subscription_transaction["transaction_info"]).to eq({ "source" => "view" }) expect(subscription_transaction["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 } @@ -252,6 +262,7 @@ def disconnect event = transport.events[1].to_json_compatible expect(event["transaction"]).to eq("AppearanceChannel#unsubscribed") + expect(event["transaction_info"]).to eq({ "source" => "view" }) expect(event["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 } @@ -262,6 +273,7 @@ def disconnect expect(transaction["type"]).to eq("transaction") expect(transaction["transaction"]).to eq("AppearanceChannel#unsubscribed") + expect(transaction["transaction_info"]).to eq({ "source" => "view" }) expect(transaction["contexts"]).to include( "action_cable" => { "params" => { "room_id" => 42 } diff --git a/sentry-rails/spec/sentry/rails/activejob_spec.rb b/sentry-rails/spec/sentry/rails/activejob_spec.rb index 62f01bb24..8127082ae 100644 --- a/sentry-rails/spec/sentry/rails/activejob_spec.rb +++ b/sentry-rails/spec/sentry/rails/activejob_spec.rb @@ -166,6 +166,7 @@ def post.to_global_id expect(transport.events.count).to eq(1) transaction = transport.events.last expect(transaction.transaction).to eq("QueryPostJob") + expect(transaction.transaction_info).to eq({ source: :task }) expect(transaction.contexts.dig(:trace, :trace_id)).to be_present expect(transaction.contexts.dig(:trace, :span_id)).to be_present expect(transaction.contexts.dig(:trace, :status)).to eq("ok") @@ -182,6 +183,7 @@ def post.to_global_id transaction = transport.events.first expect(transaction.transaction).to eq("FailedWithExtraJob") + expect(transaction.transaction_info).to eq({ source: :task }) expect(transaction.contexts.dig(:trace, :trace_id)).to be_present expect(transaction.contexts.dig(:trace, :span_id)).to be_present expect(transaction.contexts.dig(:trace, :status)).to eq("internal_error") diff --git a/sentry-rails/spec/sentry/rails_spec.rb b/sentry-rails/spec/sentry/rails_spec.rb index 173abc2cd..cfcfff405 100644 --- a/sentry-rails/spec/sentry/rails_spec.rb +++ b/sentry-rails/spec/sentry/rails_spec.rb @@ -66,14 +66,16 @@ expect(Sentry.configuration.release).to eq('beta') end - it "sets transaction to ControllerName#method" do + it "sets transaction to ControllerName#method and sets correct source" do get "/exception" expect(transport.events.last.transaction).to eq("HelloController#exception") + expect(transport.events.last.transaction_info).to eq({ source: :view }) get "/posts" expect(transport.events.last.transaction).to eq("PostsController#index") + expect(transport.events.last.transaction_info).to eq({ source: :view }) end it "sets correct request url" do @@ -196,11 +198,13 @@ expect(transport.events.count).to eq(1) last_event = transport.events.last expect(last_event.transaction).to eq("HelloController#exception") + expect(transport.events.last.transaction_info).to eq({ source: :view }) expect(response.body).to match(last_event.event_id) get "/posts" expect(transport.events.last.transaction).to eq("PostsController#index") + expect(transport.events.last.transaction_info).to eq({ source: :view }) end it "sets correct request url" do diff --git a/sentry-ruby/lib/sentry/scope.rb b/sentry-ruby/lib/sentry/scope.rb index d352a682d..acb1ac092 100644 --- a/sentry-ruby/lib/sentry/scope.rb +++ b/sentry-ruby/lib/sentry/scope.rb @@ -46,12 +46,8 @@ def apply_to_event(event, hint = nil) event.user = user.merge(event.user) event.extra = extra.merge(event.extra) event.contexts = contexts.merge(event.contexts) - event.transaction ||= transaction_name if transaction_name - - if transaction_sources - event.transaction_info ||= {} - event.transaction_info = { source: transaction_source }.merge(event.transaction_info) - end + event.transaction = transaction_name if transaction_name + event.transaction_info = { source: transaction_source } if transaction_source if span event.contexts[:trace] = span.get_trace_context diff --git a/sentry-ruby/spec/sentry/scope_spec.rb b/sentry-ruby/spec/sentry/scope_spec.rb index 45dc97e51..43b825058 100644 --- a/sentry-ruby/spec/sentry/scope_spec.rb +++ b/sentry-ruby/spec/sentry/scope_spec.rb @@ -219,16 +219,12 @@ event.user = {id: 2} event.extra = {additional_info: "nothing"} event.contexts = {os: nil} - event.transaction = "/some/url" - event.transaction_info = { source: :url } subject.apply_to_event(event) expect(event.tags).to eq({foo: "baz"}) expect(event.user).to eq({id: 2}) expect(event.extra[:additional_info]).to eq("nothing") expect(event.contexts[:os]).to eq(nil) - expect(event.transaction).to eq("/some/url") - expect(event.transaction_info).to eq({ source: :url }) end it "applies event processors to the event" do From 251ff20118f5a8519b1a71c226114a4d5772935d Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 27 Sep 2022 18:41:23 +0200 Subject: [PATCH 17/18] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2df74f2fe..9947dff13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - in all outgoing requests in our net/http patch - in Sentry transactions as [Dynamic Sampling Context](https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/) - Create new Baggage entries as Head SDK (originator of trace) [#1898](https://github.com/getsentry/sentry-ruby/pull/1898) + - Add Transaction source annotations to classify low quality (high cardinality) transaction names [#1902](https://github.com/getsentry/sentry-ruby/pull/1902) ### Bug Fixes From 327b19bc88f1acb7e6690d8abbe54cf15d3b379d Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Thu, 29 Sep 2022 14:56:04 +0200 Subject: [PATCH 18/18] Review fixes --- sentry-rails/lib/sentry/rails/action_cable.rb | 6 +++--- sentry-ruby/lib/sentry/scope.rb | 4 ++-- sentry-ruby/lib/sentry/transaction.rb | 6 +++--- sentry-ruby/spec/sentry/transport_spec.rb | 2 +- .../lib/sentry/sidekiq/sentry_context_middleware.rb | 6 +++--- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/action_cable.rb b/sentry-rails/lib/sentry/rails/action_cable.rb index fa06e870e..397a5a755 100644 --- a/sentry-rails/lib/sentry/rails/action_cable.rb +++ b/sentry-rails/lib/sentry/rails/action_cable.rb @@ -14,7 +14,7 @@ def capture(connection, transaction_name:, extra_context: nil, &block) scope.set_rack_env(env) scope.set_context("action_cable", extra_context) if extra_context scope.set_transaction_name(transaction_name, source: :view) - transaction = start_transaction(env, scope.transaction_name, scope.transaction_source) + transaction = start_transaction(env, scope) scope.set_span(transaction) if transaction begin @@ -29,11 +29,11 @@ def capture(connection, transaction_name:, extra_context: nil, &block) end end - def start_transaction(env, transaction_name, transaction_source) + def start_transaction(env, scope) sentry_trace = env["HTTP_SENTRY_TRACE"] baggage = env["HTTP_BAGGAGE"] - options = { name: transaction_name, source: transaction_source, op: "rails.action_cable".freeze } + options = { name: scope.transaction_name, source: scope.transaction_source, op: "rails.action_cable".freeze } transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, baggage: baggage, **options) if sentry_trace Sentry.start_transaction(transaction: transaction, **options) end diff --git a/sentry-ruby/lib/sentry/scope.rb b/sentry-ruby/lib/sentry/scope.rb index acb1ac092..09c4b7cde 100644 --- a/sentry-ruby/lib/sentry/scope.rb +++ b/sentry-ruby/lib/sentry/scope.rb @@ -88,8 +88,8 @@ def dup copy.extra = extra.deep_dup copy.tags = tags.deep_dup copy.user = user.deep_dup - copy.transaction_names = transaction_names.deep_dup - copy.transaction_sources = transaction_sources.deep_dup + copy.transaction_names = transaction_names.dup + copy.transaction_sources = transaction_sources.dup copy.fingerprint = fingerprint.deep_dup copy.span = span.deep_dup copy.session = session.deep_dup diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index 28ce97c72..179d5db5b 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -24,7 +24,7 @@ class Transaction < Span attr_reader :name # The source of the transaction name. - # @return [String] + # @return [Symbol] attr_reader :source # The sampling decision of the parent transaction, which will be considered when making the current transaction's sampling decision. @@ -61,7 +61,7 @@ def initialize( super(**options) @name = name - @source = SOURCES.include?(source) ? source : :custom + @source = SOURCES.include?(source) ? source.to_sym : :custom @parent_sampled = parent_sampled @transaction = self @hub = hub @@ -272,7 +272,7 @@ def populate_head_baggage items["transaction"] = name unless source_low_quality? - user = Sentry.get_current_scope&.user + user = @hub.current_scope&.user items["user_segment"] = user["segment"] if user && user["segment"] items.compact! diff --git a/sentry-ruby/spec/sentry/transport_spec.rb b/sentry-ruby/spec/sentry/transport_spec.rb index 334effb5e..a1c5f885b 100644 --- a/sentry-ruby/spec/sentry/transport_spec.rb +++ b/sentry-ruby/spec/sentry/transport_spec.rb @@ -13,7 +13,7 @@ let(:client) { Sentry::Client.new(configuration) } let(:hub) do - Sentry::Hub.new(client, subject) + Sentry::Hub.new(client, Sentry::Scope.new) end subject { client.transport } diff --git a/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb b/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb index 2c1f9e084..0619eb8a5 100644 --- a/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb +++ b/sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb @@ -17,7 +17,7 @@ def call(_worker, job, queue) scope.set_tags(build_tags(job["tags"])) scope.set_contexts(sidekiq: job.merge("queue" => queue)) scope.set_transaction_name(context_filter.transaction_name, source: :task) - transaction = start_transaction(scope.transaction_name, scope.transaction_source, job["sentry_trace"]) + transaction = start_transaction(scope, job["sentry_trace"]) scope.set_span(transaction) if transaction begin @@ -37,8 +37,8 @@ def build_tags(tags) Array(tags).each_with_object({}) { |name, tags_hash| tags_hash[:"sidekiq.#{name}"] = true } end - def start_transaction(transaction_name, transaction_source, sentry_trace) - options = { name: transaction_name, source: transaction_source, op: "sidekiq" } + def start_transaction(scope, sentry_trace) + options = { name: scope.transaction_name, source: scope.transaction_source, op: "sidekiq" } transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, **options) if sentry_trace Sentry.start_transaction(transaction: transaction, **options) end