From 644c5e7eeedc3806e71a0bf02071d4a6b98b5e0b Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 17 Sep 2021 22:32:52 +0800 Subject: [PATCH 1/3] Require debugger in specs --- sentry-rails/spec/sentry/rails/tracing_spec.rb | 13 +++++++++++++ sentry-rails/spec/spec_helper.rb | 1 + 2 files changed, 14 insertions(+) diff --git a/sentry-rails/spec/sentry/rails/tracing_spec.rb b/sentry-rails/spec/sentry/rails/tracing_spec.rb index 1225b5514..6063dba6b 100644 --- a/sentry-rails/spec/sentry/rails/tracing_spec.rb +++ b/sentry-rails/spec/sentry/rails/tracing_spec.rb @@ -21,6 +21,7 @@ expect(described_class).to receive(:subscribe_tracing_events).and_call_original make_basic_app do |config| + config.breadcrumbs_logger = [:active_support_logger] config.traces_sample_rate = 1.0 end end @@ -87,6 +88,18 @@ expect(last_span[:description]).to eq("PostsController#show") expect(last_span[:parent_span_id]).to eq(parent_span_id) end + + it "doesn't add internal start timestamp payload to breadcrumbs data" do + p = Post.create! + + get "/posts/#{p.id}" + + expect(transport.events.count).to eq(1) + + transaction = transport.events.last.to_hash + breadcrumbs = transaction[:breadcrumbs][:values] + expect(breadcrumbs.last[:data].has_key?(described_class::START_TIMESTAMP_NAME)).to eq(false) + end end context "with sprockets-rails" do diff --git a/sentry-rails/spec/spec_helper.rb b/sentry-rails/spec/spec_helper.rb index b3837d232..d19f9257a 100644 --- a/sentry-rails/spec/spec_helper.rb +++ b/sentry-rails/spec/spec_helper.rb @@ -1,4 +1,5 @@ require "bundler/setup" +require "debug" require "pry" require "sentry-ruby" From b0d34201e668a5914e6dd3da8280ac0ae0c8dc5b Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 17 Sep 2021 22:33:11 +0800 Subject: [PATCH 2/3] Don't record 'start_timestamp' attribute in breadcrumbs --- sentry-rails/lib/sentry/rails.rb | 2 +- .../rails/instrument_payload_cleanup_helper.rb | 2 +- sentry-rails/lib/sentry/rails/tracing.rb | 4 +++- .../rails/tracing/action_controller_subscriber.rb | 2 +- .../sentry/rails/tracing/action_view_subscriber.rb | 2 +- .../rails/tracing/active_record_subscriber.rb | 2 +- .../breadcrumbs/active_support_breadcrumbs_spec.rb | 14 ++++++++++++++ sentry-rails/spec/sentry/rails/tracing_spec.rb | 13 ------------- 8 files changed, 22 insertions(+), 19 deletions(-) diff --git a/sentry-rails/lib/sentry/rails.rb b/sentry-rails/lib/sentry/rails.rb index a5b895e02..14c8e1b80 100644 --- a/sentry-rails/lib/sentry/rails.rb +++ b/sentry-rails/lib/sentry/rails.rb @@ -1,10 +1,10 @@ require "rails" require "sentry-ruby" require "sentry/integrable" +require "sentry/rails/tracing" require "sentry/rails/configuration" require "sentry/rails/engine" require "sentry/rails/railtie" -require "sentry/rails/tracing" module Sentry module Rails diff --git a/sentry-rails/lib/sentry/rails/instrument_payload_cleanup_helper.rb b/sentry-rails/lib/sentry/rails/instrument_payload_cleanup_helper.rb index c4ce8833b..343d2d41c 100644 --- a/sentry-rails/lib/sentry/rails/instrument_payload_cleanup_helper.rb +++ b/sentry-rails/lib/sentry/rails/instrument_payload_cleanup_helper.rb @@ -1,7 +1,7 @@ module Sentry module Rails module InstrumentPayloadCleanupHelper - IGNORED_DATA_TYPES = [:request, :response, :headers, :exception, :exception_object] + IGNORED_DATA_TYPES = [:request, :response, :headers, :exception, :exception_object, Tracing::START_TIMESTAMP_NAME] def cleanup_data(data) IGNORED_DATA_TYPES.each do |key| diff --git a/sentry-rails/lib/sentry/rails/tracing.rb b/sentry-rails/lib/sentry/rails/tracing.rb index c1df3a113..e4bf52b7a 100644 --- a/sentry-rails/lib/sentry/rails/tracing.rb +++ b/sentry-rails/lib/sentry/rails/tracing.rb @@ -1,6 +1,8 @@ module Sentry module Rails module Tracing + START_TIMESTAMP_NAME = :sentry_start_timestamp + def self.register_subscribers(subscribers) @subscribers = subscribers end @@ -37,7 +39,7 @@ def self.patch_active_support_notifications def instrument(name, payload = {}, &block) is_public_event = name[0] != "!" - payload[:start_timestamp] = Time.now.utc.to_f if is_public_event + payload[START_TIMESTAMP_NAME] = Time.now.utc.to_f if is_public_event super(name, payload, &block) end diff --git a/sentry-rails/lib/sentry/rails/tracing/action_controller_subscriber.rb b/sentry-rails/lib/sentry/rails/tracing/action_controller_subscriber.rb index 31b3e0b74..3d69a0ac3 100644 --- a/sentry-rails/lib/sentry/rails/tracing/action_controller_subscriber.rb +++ b/sentry-rails/lib/sentry/rails/tracing/action_controller_subscriber.rb @@ -16,7 +16,7 @@ def self.subscribe! record_on_current_span( op: event_name, - start_timestamp: payload[:start_timestamp], + start_timestamp: payload[START_TIMESTAMP_NAME], description: "#{controller}##{action}", duration: duration ) do |span| diff --git a/sentry-rails/lib/sentry/rails/tracing/action_view_subscriber.rb b/sentry-rails/lib/sentry/rails/tracing/action_view_subscriber.rb index 1a31e3e37..5c0c429a2 100644 --- a/sentry-rails/lib/sentry/rails/tracing/action_view_subscriber.rb +++ b/sentry-rails/lib/sentry/rails/tracing/action_view_subscriber.rb @@ -8,7 +8,7 @@ class ActionViewSubscriber < AbstractSubscriber def self.subscribe! subscribe_to_event(EVENT_NAME) do |event_name, duration, payload| - record_on_current_span(op: event_name, start_timestamp: payload[:start_timestamp], description: payload[:identifier], duration: duration) + record_on_current_span(op: event_name, start_timestamp: payload[START_TIMESTAMP_NAME], description: payload[:identifier], duration: duration) end end end diff --git a/sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb b/sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb index d4486266b..262991736 100644 --- a/sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb +++ b/sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb @@ -11,7 +11,7 @@ def self.subscribe! subscribe_to_event(EVENT_NAME) do |event_name, duration, payload| next if EXCLUDED_EVENTS.include? payload[:name] - record_on_current_span(op: event_name, start_timestamp: payload[:start_timestamp], description: payload[:sql], duration: duration) do |span| + record_on_current_span(op: event_name, start_timestamp: payload[START_TIMESTAMP_NAME], description: payload[:sql], duration: duration) do |span| span.set_data(:connection_id, payload[:connection_id]) end end diff --git a/sentry-rails/spec/sentry/rails/breadcrumbs/active_support_breadcrumbs_spec.rb b/sentry-rails/spec/sentry/rails/breadcrumbs/active_support_breadcrumbs_spec.rb index 656d49cce..93914fe31 100644 --- a/sentry-rails/spec/sentry/rails/breadcrumbs/active_support_breadcrumbs_spec.rb +++ b/sentry-rails/spec/sentry/rails/breadcrumbs/active_support_breadcrumbs_spec.rb @@ -92,4 +92,18 @@ expect(breadcrumb_buffer.count).to be_zero end + + context "when used with tracing" do + it "doesn't add internal start timestamp payload to breadcrumbs data" do + p = Post.create! + + get "/posts/#{p.id}" + + expect(transport.events.count).to eq(1) + + transaction = transport.events.last.to_hash + breadcrumbs = transaction[:breadcrumbs][:values] + expect(breadcrumbs.last[:data].has_key?(Sentry::Rails::Tracing::START_TIMESTAMP_NAME)).to eq(false) + end + end end diff --git a/sentry-rails/spec/sentry/rails/tracing_spec.rb b/sentry-rails/spec/sentry/rails/tracing_spec.rb index 6063dba6b..1225b5514 100644 --- a/sentry-rails/spec/sentry/rails/tracing_spec.rb +++ b/sentry-rails/spec/sentry/rails/tracing_spec.rb @@ -21,7 +21,6 @@ expect(described_class).to receive(:subscribe_tracing_events).and_call_original make_basic_app do |config| - config.breadcrumbs_logger = [:active_support_logger] config.traces_sample_rate = 1.0 end end @@ -88,18 +87,6 @@ expect(last_span[:description]).to eq("PostsController#show") expect(last_span[:parent_span_id]).to eq(parent_span_id) end - - it "doesn't add internal start timestamp payload to breadcrumbs data" do - p = Post.create! - - get "/posts/#{p.id}" - - expect(transport.events.count).to eq(1) - - transaction = transport.events.last.to_hash - breadcrumbs = transaction[:breadcrumbs][:values] - expect(breadcrumbs.last[:data].has_key?(described_class::START_TIMESTAMP_NAME)).to eq(false) - end end context "with sprockets-rails" do From 9c553d49057a4ab51f9383bcca69172cf0acd56a Mon Sep 17 00:00:00 2001 From: st0012 Date: Fri, 17 Sep 2021 23:09:45 +0800 Subject: [PATCH 3/3] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e08709a7c..8a9e8941a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## Unreleased + +- Avoid leaking tracing timestamp to breadcrumbs [#1575](https://github.com/getsentry/sentry-ruby/pull/1575) + ## 4.7.2 - Change default environment to 'development' [#1565](https://github.com/getsentry/sentry-ruby/pull/1565)