From 9af3670f26bb32b9fae91ea2c6444a58199e4aae Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 21 Oct 2022 11:06:28 +0100 Subject: [PATCH] Make transaction a required argument of Span According to the specification, a Span must has a Transaction, whether it's directly assigned to it or inherited from its parent Span. So this commit enforces that by requiring the `transaction:` argument on Span's constructor. Although it makes test setup a bit more complicated, it does make tests reflect our real-world expectation more accurately. --- sentry-ruby/lib/sentry/span.rb | 11 ++--- sentry-ruby/lib/sentry/transaction.rb | 3 +- sentry-ruby/spec/sentry/client_spec.rb | 11 ++++- sentry-ruby/spec/sentry/scope/setters_spec.rb | 2 +- sentry-ruby/spec/sentry/scope_spec.rb | 23 ++++------ sentry-ruby/spec/sentry/span_spec.rb | 42 +++++++++++-------- sentry-ruby/spec/sentry_spec.rb | 5 ++- 7 files changed, 56 insertions(+), 41 deletions(-) diff --git a/sentry-ruby/lib/sentry/span.rb b/sentry-ruby/lib/sentry/span.rb index 94c276b92..fc41c4d37 100644 --- a/sentry-ruby/lib/sentry/span.rb +++ b/sentry-ruby/lib/sentry/span.rb @@ -60,9 +60,10 @@ class Span # The Transaction object the Span belongs to. # Every span needs to be attached to a Transaction and their child spans will also inherit the same transaction. # @return [Transaction] - attr_accessor :transaction + attr_reader :transaction def initialize( + transaction:, description: nil, op: nil, status: nil, @@ -79,6 +80,7 @@ def initialize( @start_timestamp = start_timestamp || Sentry.utc_now.to_f @timestamp = timestamp @description = description + @transaction = transaction @op = op @status = status @data = {} @@ -105,10 +107,10 @@ def to_sentry_trace end # Generates a W3C Baggage header string for distributed tracing - # from the incoming baggage stored on the transation. + # from the incoming baggage stored on the transaction. # @return [String, nil] def to_baggage - transaction&.get_baggage&.serialize + transaction.get_baggage&.serialize end # @return [Hash] @@ -143,9 +145,8 @@ def get_trace_context # Starts a child span with given attributes. # @param attributes [Hash] the attributes for the child span. def start_child(**attributes) - attributes = attributes.dup.merge(trace_id: @trace_id, parent_span_id: @span_id, sampled: @sampled) + attributes = attributes.dup.merge(transaction: @transaction, trace_id: @trace_id, parent_span_id: @span_id, sampled: @sampled) new_span = Span.new(**attributes) - new_span.transaction = transaction new_span.span_recorder = span_recorder if span_recorder diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index 179d5db5b..2a5ab0f0c 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -58,12 +58,11 @@ def initialize( baggage: nil, **options ) - super(**options) + super(transaction: self, **options) @name = name @source = SOURCES.include?(source) ? source.to_sym : :custom @parent_sampled = parent_sampled - @transaction = self @hub = hub @baggage = baggage @configuration = hub.configuration # to be removed diff --git a/sentry-ruby/spec/sentry/client_spec.rb b/sentry-ruby/spec/sentry/client_spec.rb index 6283b7a92..d8d03f2a7 100644 --- a/sentry-ruby/spec/sentry/client_spec.rb +++ b/sentry-ruby/spec/sentry/client_spec.rb @@ -17,6 +17,15 @@ def sentry_context end subject { Sentry::Client.new(configuration) } + let(:transaction) do + hub = Sentry::Hub.new(subject, Sentry::Scope.new) + Sentry::Transaction.new( + name: "test transaction", + hub: hub, + sampled: true + ) + end + let(:fake_time) { Time.now } before do @@ -406,7 +415,7 @@ module ExcTag; end configuration.logger = logger end - let(:span) { Sentry::Span.new } + let(:span) { Sentry::Span.new(transaction: transaction) } it "generates the trace with given span and logs correct message" do expect(subject.generate_sentry_trace(span)).to eq(span.to_sentry_trace) diff --git a/sentry-ruby/spec/sentry/scope/setters_spec.rb b/sentry-ruby/spec/sentry/scope/setters_spec.rb index 3b2c23129..acbb922b3 100644 --- a/sentry-ruby/spec/sentry/scope/setters_spec.rb +++ b/sentry-ruby/spec/sentry/scope/setters_spec.rb @@ -24,7 +24,7 @@ end describe "#set_span" do - let(:span) { Sentry::Span.new(op: "foo") } + let(:span) { Sentry::Span.new(op: "foo", transaction: nil) } it "sets the Span" do subject.set_span(span) diff --git a/sentry-ruby/spec/sentry/scope_spec.rb b/sentry-ruby/spec/sentry/scope_spec.rb index 43b825058..98b1b6d3f 100644 --- a/sentry-ruby/spec/sentry/scope_spec.rb +++ b/sentry-ruby/spec/sentry/scope_spec.rb @@ -111,18 +111,13 @@ end describe "#clear" do - subject do - scope = described_class.new - scope.set_tags({foo: "bar"}) - scope.set_extras({additional_info: "hello"}) - scope.set_user({id: 1}) - scope.set_transaction_name("WelcomeController#index") - scope.set_span(Sentry::Span.new) - scope.set_fingerprint(["foo"]) - scope - end - it "resets the scope's data" do + subject.set_tags({foo: "bar"}) + subject.set_extras({additional_info: "hello"}) + subject.set_user({id: 1}) + subject.set_transaction_name("WelcomeController#index") + subject.set_span(Sentry::Transaction.new(op: "foo", hub: hub)) + subject.set_fingerprint(["foo"]) scope_id = subject.object_id subject.clear @@ -241,12 +236,12 @@ end it "sets trace context if there's a span" do - span = Sentry::Span.new(op: "foo") - subject.set_span(span) + transaction = Sentry::Transaction.new(op: "foo", hub: hub) + subject.set_span(transaction) subject.apply_to_event(event) - expect(event.contexts[:trace]).to eq(span.get_trace_context) + expect(event.contexts[:trace]).to eq(transaction.get_trace_context) expect(event.contexts.dig(:trace, :op)).to eq("foo") end diff --git a/sentry-ruby/spec/sentry/span_spec.rb b/sentry-ruby/spec/sentry/span_spec.rb index 78fe9596c..6758ed67e 100644 --- a/sentry-ruby/spec/sentry/span_spec.rb +++ b/sentry-ruby/spec/sentry/span_spec.rb @@ -1,8 +1,22 @@ require "spec_helper" RSpec.describe Sentry::Span do + let(:hub) do + client = Sentry::Client.new(Sentry::Configuration.new) + Sentry::Hub.new(client, Sentry::Scope.new) + end + + let(:transaction) do + Sentry::Transaction.new( + name: "test transaction", + hub: hub, + sampled: true + ) + end + subject do described_class.new( + transaction: transaction, op: "sql.query", description: "SELECT * FROM users;", status: "ok", @@ -67,7 +81,7 @@ end context "without sampled value" do - subject { described_class.new } + subject { described_class.new(transaction: transaction) } it "doesn't contain the sampled flag" do sentry_trace = subject.to_sentry_trace @@ -129,6 +143,16 @@ expect(new_span.sampled).to eq(true) end + it "gives the child span its transaction" do + span_1 = subject.start_child + + expect(span_1.transaction).to eq(subject.transaction) + + span_2 = span_1.start_child + + expect(span_2.transaction).to eq(subject.transaction) + end + context "when the parent span has a span_recorder" do subject do # inherits the span recorder from the transaction @@ -150,22 +174,6 @@ expect(subject.span_recorder.spans.count).to eq(4) end end - - context "when the parent span has a transaction" do - before do - subject.transaction = Sentry::Transaction.new(hub: Sentry.get_current_hub) - end - - it "gives the child span its transaction" do - span_1 = subject.start_child - - expect(span_1.transaction).to eq(subject.transaction) - - span_2 = span_1.start_child - - expect(span_2.transaction).to eq(subject.transaction) - end - end end describe "#with_child_span" do diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index 1fac6ad99..94ea1a948 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -494,7 +494,10 @@ end context "when the current span is present" do - let(:parent_span) { Sentry::Span.new(op: "parent") } + let(:parent_span) do + transaction = Sentry::Transaction.new(op: "foo", hub: Sentry.get_current_hub) + Sentry::Span.new(op: "parent", transaction: transaction) + end before do described_class.get_current_scope.set_span(parent_span)