From 248ad250c483bf59f87b91a8dbdda847e61b305e Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 22 Nov 2022 14:24:37 +0100 Subject: [PATCH 1/5] Add config.instrumenter to switch between sentry and otel instrumentation --- CHANGELOG.md | 7 +++ sentry-rails/lib/sentry/rails/railtie.rb | 2 +- .../spec/sentry/rails/tracing_spec.rb | 17 ++++++ sentry-ruby/lib/sentry/configuration.rb | 11 ++++ sentry-ruby/lib/sentry/hub.rb | 7 ++- sentry-ruby/spec/sentry/configuration_spec.rb | 21 +++++++ sentry-ruby/spec/sentry_spec.rb | 57 +++++++++++++++++++ 7 files changed, 119 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52fc0ad32..9462778ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## Unreleased + +### Features + +- Add OpenTelemetry support with new `sentry-opentelemetry` gem + - Add `config.instrumenter` to switch between sentry and otel instrumentation [#1944](https://github.com/getsentry/sentry-ruby/pull/1944) + ## 5.6.0 ### Features diff --git a/sentry-rails/lib/sentry/rails/railtie.rb b/sentry-rails/lib/sentry/rails/railtie.rb index 8e21931ca..63f929b1c 100644 --- a/sentry-rails/lib/sentry/rails/railtie.rb +++ b/sentry-rails/lib/sentry/rails/railtie.rb @@ -115,7 +115,7 @@ def override_streaming_reporter end def activate_tracing - if Sentry.configuration.tracing_enabled? + if Sentry.configuration.tracing_enabled? && Sentry.configuration.instrumenter == :sentry subscribers = Sentry.configuration.rails.tracing_subscribers Sentry::Rails::Tracing.register_subscribers(subscribers) Sentry::Rails::Tracing.subscribe_tracing_events diff --git a/sentry-rails/spec/sentry/rails/tracing_spec.rb b/sentry-rails/spec/sentry/rails/tracing_spec.rb index 8d8b9da54..346471f9c 100644 --- a/sentry-rails/spec/sentry/rails/tracing_spec.rb +++ b/sentry-rails/spec/sentry/rails/tracing_spec.rb @@ -91,6 +91,23 @@ end end + context "with instrumenter :otel" do + before do + make_basic_app do |config| + config.traces_sample_rate = 1.0 + config.instrumenter = :otel + end + end + + it "doesn't do any tracing" do + p = Post.create! + get "/posts/#{p.id}" + + expect(response).to have_http_status(:ok) + expect(transport.events.count).to eq(0) + end + end + context "with sprockets-rails" do let(:string_io) { StringIO.new } let(:logger) do diff --git a/sentry-ruby/lib/sentry/configuration.rb b/sentry-ruby/lib/sentry/configuration.rb index 5cbc08355..9c6eebc41 100644 --- a/sentry-ruby/lib/sentry/configuration.rb +++ b/sentry-ruby/lib/sentry/configuration.rb @@ -211,6 +211,10 @@ class Configuration # @return [Boolean] attr_accessor :auto_session_tracking + # The instrumenter to use, :sentry or :otel + # @return [Symbol] + attr_reader :instrumenter + # these are not config options # @!visibility private attr_reader :errors, :gem_specs @@ -237,6 +241,8 @@ class Configuration MODULE_SEPARATOR = "::".freeze SKIP_INSPECTION_ATTRIBUTES = [:@linecache, :@stacktrace_builder] + INSTRUMENTERS = [:sentry, :otel] + # Post initialization callbacks are called at the end of initialization process # allowing extending the configuration of sentry-ruby by multiple extensions @@post_initialization_callbacks = [] @@ -269,6 +275,7 @@ def initialize self.trusted_proxies = [] self.dsn = ENV['SENTRY_DSN'] self.server_name = server_name_from_env + self.instrumenter = :sentry self.before_send = nil self.rack_env_whitelist = RACK_ENV_WHITELIST_DEFAULT @@ -332,6 +339,10 @@ def environment=(environment) @environment = environment.to_s end + def instrumenter=(instrumenter) + @instrumenter = INSTRUMENTERS.include?(instrumenter) ? instrumenter : :sentry + end + def sending_allowed? @errors = [] diff --git a/sentry-ruby/lib/sentry/hub.rb b/sentry-ruby/lib/sentry/hub.rb index 582855a30..8e58c60fc 100644 --- a/sentry-ruby/lib/sentry/hub.rb +++ b/sentry-ruby/lib/sentry/hub.rb @@ -76,8 +76,9 @@ def pop_scope @stack.pop end - def start_transaction(transaction: nil, custom_sampling_context: {}, **options) + def start_transaction(transaction: nil, custom_sampling_context: {}, instrumenter: :sentry, **options) return unless configuration.tracing_enabled? + return unless instrumenter == configuration.instrumenter transaction ||= Transaction.new(**options.merge(hub: self)) @@ -92,7 +93,9 @@ def start_transaction(transaction: nil, custom_sampling_context: {}, **options) transaction end - def with_child_span(**attributes, &block) + def with_child_span(instrumenter: :sentry, **attributes, &block) + return yield(nil) unless instrumenter == configuration.instrumenter + current_span = current_scope.get_span return yield(nil) unless current_span diff --git a/sentry-ruby/spec/sentry/configuration_spec.rb b/sentry-ruby/spec/sentry/configuration_spec.rb index 6f73bfb77..e164f9313 100644 --- a/sentry-ruby/spec/sentry/configuration_spec.rb +++ b/sentry-ruby/spec/sentry/configuration_spec.rb @@ -351,4 +351,25 @@ class SentryConfigurationSample < Sentry::Configuration expect(subject.auto_session_tracking).to eq(false) end end + + describe "#instrumenter" do + it "returns :sentry by default" do + expect(subject.instrumenter).to eq(:sentry) + end + + it "can be set to :sentry" do + subject.instrumenter = :sentry + expect(subject.instrumenter).to eq(:sentry) + end + + it "can be set to :otel" do + subject.instrumenter = :otel + expect(subject.instrumenter).to eq(:otel) + end + + it "defaults to :sentry if invalid" do + subject.instrumenter = :foo + expect(subject.instrumenter).to eq(:sentry) + end + end end diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index 94ea1a948..c8b7cb3b3 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -469,6 +469,24 @@ expect(described_class.start_transaction(op: "foo")).to eq(nil) end end + + context "when instrumenter is not :sentry" do + before do + perform_basic_setup do |config| + config.traces_sample_rate = 1.0 + config.instrumenter = :otel + end + end + + it "noops without explicit instrumenter" do + expect(described_class.start_transaction(op: "foo")).to eq(nil) + end + + it "creates transaction with explicit instrumenter" do + transaction = described_class.start_transaction(op: "foo", instrumenter: :otel) + expect(transaction).to be_a(Sentry::Transaction) + end + end end describe ".with_child_span" do @@ -515,6 +533,45 @@ expect(child_span.parent_span_id).to eq(parent_span.span_id) expect(child_span.timestamp).to be_a(Float) end + + context "when instrumenter is not :sentry" do + before do + perform_basic_setup do |config| + config.traces_sample_rate = 1.0 + config.instrumenter = :otel + end + + described_class.get_current_scope.set_span(parent_span) + end + + it "yields block with nil without explicit instrumenter" do + span = nil + executed = false + + result = described_class.with_child_span do |child_span| + span = child_span + executed = true + "foobar" + end + + expect(result).to eq("foobar") + expect(span).to eq(nil) + expect(executed).to eq(true) + end + + it "records the child span with explicit instrumenter" do + child_span = nil + + result = described_class.with_child_span(instrumenter: :otel, op: "child") do |span| + child_span = span + "foobar" + end + + expect(result).to eq("foobar") + expect(child_span.parent_span_id).to eq(parent_span.span_id) + expect(child_span.timestamp).to be_a(Float) + end + end end end From 954d6a2d0921489e239f9c2af141df8b4803e8a9 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 22 Nov 2022 16:35:17 +0100 Subject: [PATCH 2/5] Expose span_id in Span constructor --- CHANGELOG.md | 1 + sentry-ruby/lib/sentry/span.rb | 3 ++- sentry-ruby/spec/sentry/span_spec.rb | 8 ++++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9462778ef..a1c80f376 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Add OpenTelemetry support with new `sentry-opentelemetry` gem - Add `config.instrumenter` to switch between sentry and otel instrumentation [#1944](https://github.com/getsentry/sentry-ruby/pull/1944) + - Expose `span_id` in Span constructor [#1945](https://github.com/getsentry/sentry-ruby/pull/1945) ## 5.6.0 diff --git a/sentry-ruby/lib/sentry/span.rb b/sentry-ruby/lib/sentry/span.rb index fc41c4d37..9ca450e16 100644 --- a/sentry-ruby/lib/sentry/span.rb +++ b/sentry-ruby/lib/sentry/span.rb @@ -68,13 +68,14 @@ def initialize( op: nil, status: nil, trace_id: nil, + span_id: nil, parent_span_id: nil, sampled: nil, start_timestamp: nil, timestamp: nil ) @trace_id = trace_id || SecureRandom.uuid.delete("-") - @span_id = SecureRandom.hex(8) + @span_id = span_id || SecureRandom.hex(8) @parent_span_id = parent_span_id @sampled = sampled @start_timestamp = start_timestamp || Sentry.utc_now.to_f diff --git a/sentry-ruby/spec/sentry/span_spec.rb b/sentry-ruby/spec/sentry/span_spec.rb index 6758ed67e..b9518a570 100644 --- a/sentry-ruby/spec/sentry/span_spec.rb +++ b/sentry-ruby/spec/sentry/span_spec.rb @@ -153,6 +153,14 @@ expect(span_2.transaction).to eq(subject.transaction) end + it "initializes a new child Span with explicit span id" do + span_id = SecureRandom.hex(8) + new_span = subject.start_child(op: "foo", span_id: span_id) + + expect(new_span.op).to eq("foo") + expect(new_span.span_id).to eq(span_id) + end + context "when the parent span has a span_recorder" do subject do # inherits the span recorder from the transaction From f2d6d717a124d07be45601bc36d3ad2f577ed98c Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 22 Nov 2022 16:40:58 +0100 Subject: [PATCH 3/5] Expose optional end_timestamp argument in Span#finish and Transaction#finish --- CHANGELOG.md | 3 ++- sentry-ruby/lib/sentry/span.rb | 4 ++-- sentry-ruby/lib/sentry/transaction.rb | 4 ++-- sentry-ruby/spec/sentry/transaction_spec.rb | 10 ++++++++++ 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1c80f376..3005db183 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ - Add OpenTelemetry support with new `sentry-opentelemetry` gem - Add `config.instrumenter` to switch between sentry and otel instrumentation [#1944](https://github.com/getsentry/sentry-ruby/pull/1944) - - Expose `span_id` in Span constructor [#1945](https://github.com/getsentry/sentry-ruby/pull/1945) + - Expose `span_id` in `Span` constructor [#1945](https://github.com/getsentry/sentry-ruby/pull/1945) + - Expose `end_timestamp` in `Span#finish` and `Transaction#finish` [#1946](https://github.com/getsentry/sentry-ruby/pull/1946) ## 5.6.0 diff --git a/sentry-ruby/lib/sentry/span.rb b/sentry-ruby/lib/sentry/span.rb index 9ca450e16..f7d16cb83 100644 --- a/sentry-ruby/lib/sentry/span.rb +++ b/sentry-ruby/lib/sentry/span.rb @@ -90,11 +90,11 @@ def initialize( # Finishes the span by adding a timestamp. # @return [self] - def finish + def finish(end_timestamp: nil) # already finished return if @timestamp - @timestamp = Sentry.utc_now.to_f + @timestamp = end_timestamp || Sentry.utc_now.to_f self end diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index 2a5ab0f0c..e237b5a9b 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -210,7 +210,7 @@ def set_initial_sample_decision(sampling_context:) # Finishes the transaction's recording and send it to Sentry. # @param hub [Hub] the hub that'll send this transaction. (Deprecated) # @return [TransactionEvent] - def finish(hub: nil) + def finish(hub: nil, end_timestamp: nil) if hub log_warn( <<~MSG @@ -222,7 +222,7 @@ def finish(hub: nil) hub ||= @hub - super() # Span#finish doesn't take arguments + super(end_timestamp: end_timestamp) if @name.nil? @name = UNLABELD_NAME diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index 2d4716f66..da7f54a68 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -391,6 +391,16 @@ expect(event[:transaction]).to eq("foo") end + it "finishes the transaction with explicit timestamp" do + timestamp = Sentry.utc_now.to_f + subject.finish(end_timestamp: timestamp) + + expect(events.count).to eq(1) + event = events.last.to_hash + + expect(event[:timestamp]).to eq(timestamp) + end + it "assigns the transaction's tags" do Sentry.set_tags(name: "apple") From 9ed5e426846dcdeef27597d9d0b51fec5f98a732 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 22 Nov 2022 16:48:06 +0100 Subject: [PATCH 4/5] Add Transaction#set_context api --- CHANGELOG.md | 1 + sentry-ruby/lib/sentry/transaction.rb | 52 ++++++++++++++++----- sentry-ruby/lib/sentry/transaction_event.rb | 1 + sentry-ruby/spec/sentry/client_spec.rb | 6 +++ sentry-ruby/spec/sentry/hub_spec.rb | 19 ++++++++ sentry-ruby/spec/sentry/transaction_spec.rb | 15 ++++++ 6 files changed, 83 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3005db183..c11b93a24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Add `config.instrumenter` to switch between sentry and otel instrumentation [#1944](https://github.com/getsentry/sentry-ruby/pull/1944) - Expose `span_id` in `Span` constructor [#1945](https://github.com/getsentry/sentry-ruby/pull/1945) - Expose `end_timestamp` in `Span#finish` and `Transaction#finish` [#1946](https://github.com/getsentry/sentry-ruby/pull/1946) + - Add `Transaction#set_context` api [#1947](https://github.com/getsentry/sentry-ruby/pull/1947) ## 5.6.0 diff --git a/sentry-ruby/lib/sentry/transaction.rb b/sentry-ruby/lib/sentry/transaction.rb index e237b5a9b..55a4b9e81 100644 --- a/sentry-ruby/lib/sentry/transaction.rb +++ b/sentry-ruby/lib/sentry/transaction.rb @@ -50,6 +50,10 @@ class Transaction < Span # @return [Float, nil] attr_reader :effective_sample_rate + # Additional contexts stored directly on the transaction object. + # @return [Hash] + attr_reader :contexts + def initialize( hub:, name: nil, @@ -60,8 +64,7 @@ def initialize( ) super(transaction: self, **options) - @name = name - @source = SOURCES.include?(source) ? source.to_sym : :custom + set_name(name, source: source) @parent_sampled = parent_sampled @hub = hub @baggage = baggage @@ -74,6 +77,7 @@ def initialize( @environment = hub.configuration.environment @dsn = hub.configuration.dsn @effective_sample_rate = nil + @contexts = {} init_span_recorder end @@ -91,16 +95,10 @@ def self.from_sentry_trace(sentry_trace, baggage: nil, hub: Sentry.get_current_h return unless hub.configuration.tracing_enabled? return unless sentry_trace - match = SENTRY_TRACE_REGEXP.match(sentry_trace) - return if match.nil? - trace_id, parent_span_id, sampled_flag = match[1..3] + sentry_trace_data = extract_sentry_trace(sentry_trace) + return unless sentry_trace_data - parent_sampled = - if sampled_flag.nil? - nil - else - sampled_flag != "0" - end + trace_id, parent_span_id, parent_sampled = sentry_trace_data baggage = if baggage && !baggage.empty? Baggage.from_incoming_header(baggage) @@ -123,6 +121,20 @@ def self.from_sentry_trace(sentry_trace, baggage: nil, hub: Sentry.get_current_h ) end + # Extract the trace_id, parent_span_id and parent_sampled values from a sentry-trace header. + # + # @param sentry_trace [String] the sentry-trace header value from the previous transaction. + # @return [Array, nil] + def self.extract_sentry_trace(sentry_trace) + match = SENTRY_TRACE_REGEXP.match(sentry_trace) + return nil if match.nil? + + trace_id, parent_span_id, sampled_flag = match[1..3] + parent_sampled = sampled_flag.nil? ? nil : sampled_flag != "0" + + [trace_id, parent_span_id, parent_sampled] + end + # @return [Hash] def to_hash hash = super @@ -244,6 +256,24 @@ def get_baggage @baggage end + # Set the transaction name directly. + # Considered internal api since it bypasses the usual scope logic. + # @param name [String] + # @param source [Symbol] + # @return [void] + def set_name(name, source: :custom) + @name = name + @source = SOURCES.include?(source) ? source.to_sym : :custom + end + + # Set contexts directly on the transaction. + # @param key [String, Symbol] + # @param value [Object] + # @return [void] + def set_context(key, value) + @contexts[key] = value + end + protected def init_span_recorder(limit = 1000) diff --git a/sentry-ruby/lib/sentry/transaction_event.rb b/sentry-ruby/lib/sentry/transaction_event.rb index 06f89488f..389ca45b0 100644 --- a/sentry-ruby/lib/sentry/transaction_event.rb +++ b/sentry-ruby/lib/sentry/transaction_event.rb @@ -19,6 +19,7 @@ def initialize(transaction:, **options) self.transaction = transaction.name self.transaction_info = { source: transaction.source } + self.contexts.merge!(transaction.contexts) self.contexts.merge!(trace: transaction.get_trace_context) self.timestamp = transaction.timestamp self.start_timestamp = transaction.start_timestamp diff --git a/sentry-ruby/spec/sentry/client_spec.rb b/sentry-ruby/spec/sentry/client_spec.rb index 44a3bda0f..39a04d53e 100644 --- a/sentry-ruby/spec/sentry/client_spec.rb +++ b/sentry-ruby/spec/sentry/client_spec.rb @@ -160,6 +160,12 @@ def sentry_context "trace_id" => transaction.trace_id }) end + + it "adds explicitly added contexts to event" do + transaction.set_context(:foo, { bar: 42 }) + event = subject.event_from_transaction(transaction) + expect(event.contexts).to include({ foo: { bar: 42 } }) + end end describe "#event_from_exception" do diff --git a/sentry-ruby/spec/sentry/hub_spec.rb b/sentry-ruby/spec/sentry/hub_spec.rb index f73852a05..e1ceedf43 100644 --- a/sentry-ruby/spec/sentry/hub_spec.rb +++ b/sentry-ruby/spec/sentry/hub_spec.rb @@ -241,6 +241,25 @@ end end + context "when event is a transaction" do + it "scope.set_context merges and takes precedence over transaction.set_context" do + scope.set_context(:foo, { val: 42 }) + scope.set_context(:bar, { val: 43 }) + + transaction = Sentry::Transaction.new(hub: subject, name: 'test') + transaction.set_context(:foo, { val: 44 }) + transaction.set_context(:baz, { val: 45 }) + + transaction_event = subject.current_client.event_from_transaction(transaction) + subject.capture_event(transaction_event) + + event = transport.events.last + expect(event.contexts[:foo]). to eq({ val: 44 }) + expect(event.contexts[:bar]). to eq({ val: 43 }) + expect(event.contexts[:baz]). to eq({ val: 45 }) + end + end + it_behaves_like "capture_helper" do let(:capture_helper) { :capture_event } let(:capture_subject) { event } diff --git a/sentry-ruby/spec/sentry/transaction_spec.rb b/sentry-ruby/spec/sentry/transaction_spec.rb index da7f54a68..665095aa1 100644 --- a/sentry-ruby/spec/sentry/transaction_spec.rb +++ b/sentry-ruby/spec/sentry/transaction_spec.rb @@ -559,4 +559,19 @@ end end end + + describe "#set_name" do + it "sets name and source directly" do + subject.set_name("bar", source: :url) + expect(subject.name).to eq("bar") + expect(subject.source).to eq(:url) + end + end + + describe "#set_context" do + it "sets arbitrary context" do + subject.set_context(:foo, { bar: 42 }) + expect(subject.contexts).to eq({ foo: { bar: 42 } }) + end + end end From d8a3d5e1ebf3e66ed6539f93eca60b426eb53355 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Tue, 29 Nov 2022 14:11:58 +0100 Subject: [PATCH 5/5] Fix spec name --- sentry-ruby/spec/sentry/hub_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry-ruby/spec/sentry/hub_spec.rb b/sentry-ruby/spec/sentry/hub_spec.rb index e1ceedf43..3b43000a2 100644 --- a/sentry-ruby/spec/sentry/hub_spec.rb +++ b/sentry-ruby/spec/sentry/hub_spec.rb @@ -242,7 +242,7 @@ end context "when event is a transaction" do - it "scope.set_context merges and takes precedence over transaction.set_context" do + it "transaction.set_context merges and takes precedence over scope.set_context" do scope.set_context(:foo, { val: 42 }) scope.set_context(:bar, { val: 43 })