From c2be79e41997052c0866b20f2b098ead7bed2a51 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 31 Oct 2021 08:09:24 +0000 Subject: [PATCH 1/6] Remove unnecessary sending check from Transport Transport should only care about sending the event object to the right destination with right format. It doesn't need to check if it's allowed to send an event, which should already be done by `Client#capture_event`. --- sentry-ruby/lib/sentry/transport.rb | 6 ------ sentry-ruby/spec/sentry/transport_spec.rb | 19 ------------------- 2 files changed, 25 deletions(-) diff --git a/sentry-ruby/lib/sentry/transport.rb b/sentry-ruby/lib/sentry/transport.rb index 05ceff34c..19ba4f0fd 100644 --- a/sentry-ruby/lib/sentry/transport.rb +++ b/sentry-ruby/lib/sentry/transport.rb @@ -27,12 +27,6 @@ def send_event(event) event_hash = event.to_hash item_type = get_item_type(event_hash) - unless configuration.sending_allowed? - log_debug("Envelope [#{item_type}] not sent: #{configuration.error_messages}") - - return - end - if is_rate_limited?(item_type) log_info("Envelope [#{item_type}] not sent: rate limiting") diff --git a/sentry-ruby/spec/sentry/transport_spec.rb b/sentry-ruby/spec/sentry/transport_spec.rb index a318487f3..8b70c5846 100644 --- a/sentry-ruby/spec/sentry/transport_spec.rb +++ b/sentry-ruby/spec/sentry/transport_spec.rb @@ -79,25 +79,6 @@ let(:client) { Sentry::Client.new(configuration) } let(:event) { client.event_from_exception(ZeroDivisionError.new("divided by 0")) } - context "when event is not allowed (by sampling)" do - let(:string_io) do - StringIO.new - end - - before do - configuration.logger = Logger.new(string_io) - configuration.sample_rate = 0.5 - allow(Random).to receive(:rand).and_return(0.6) - end - - it "logs correct message" do - subject.send_event(event) - - logs = string_io.string - expect(logs).to match(/Envelope \[event\] not sent: Excluded by random sample/) - end - end - context "when success" do before do allow(subject).to receive(:send_data) From 3dbda5c40d8245be0acfbe66c00d96368f3d1331 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 31 Oct 2021 08:19:07 +0000 Subject: [PATCH 2/6] Remove unused @configuration from Transport --- sentry-ruby/lib/sentry/transport.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sentry-ruby/lib/sentry/transport.rb b/sentry-ruby/lib/sentry/transport.rb index 19ba4f0fd..a4c70486f 100644 --- a/sentry-ruby/lib/sentry/transport.rb +++ b/sentry-ruby/lib/sentry/transport.rb @@ -8,11 +8,9 @@ class Transport include LoggingHelper - attr_accessor :configuration attr_reader :logger, :rate_limits def initialize(configuration) - @configuration = configuration @logger = configuration.logger @transport_configuration = configuration.transport @dsn = configuration.dsn @@ -91,7 +89,7 @@ def encode(event) item_type = get_item_type(event_hash) envelope = <<~ENVELOPE - {"event_id":"#{event_id}","dsn":"#{configuration.dsn.to_s}","sdk":#{Sentry.sdk_meta.to_json},"sent_at":"#{Sentry.utc_now.iso8601}"} + {"event_id":"#{event_id}","dsn":"#{@dsn.to_s}","sdk":#{Sentry.sdk_meta.to_json},"sent_at":"#{Sentry.utc_now.iso8601}"} {"type":"#{item_type}","content_type":"application/json"} #{JSON.generate(event_hash)} ENVELOPE From adc7685dff7544aac981b21a3958eee0c2e53e99 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 31 Oct 2021 08:28:37 +0000 Subject: [PATCH 3/6] Remove sampling from general sending restriction logic --- sentry-ruby/lib/sentry/configuration.rb | 21 +++++++------------ sentry-ruby/spec/sentry/configuration_spec.rb | 10 ++++----- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/sentry-ruby/lib/sentry/configuration.rb b/sentry-ruby/lib/sentry/configuration.rb index 9de0626cf..7ca79bac1 100644 --- a/sentry-ruby/lib/sentry/configuration.rb +++ b/sentry-ruby/lib/sentry/configuration.rb @@ -268,9 +268,13 @@ def environment=(environment) def sending_allowed? @errors = [] - valid? && - capture_in_environment? && - sample_allowed? + valid? && capture_in_environment? + end + + def sample_allowed? + return true if sample_rate == 1.0 + + Random.rand < sample_rate end def error_messages @@ -389,17 +393,6 @@ def valid? end end - def sample_allowed? - return true if sample_rate == 1.0 - - if Random.rand >= sample_rate - @errors << "Excluded by random sample" - false - else - true - end - end - def environment_from_env ENV['SENTRY_CURRENT_ENV'] || ENV['SENTRY_ENVIRONMENT'] || ENV['RAILS_ENV'] || ENV['RACK_ENV'] || 'development' end diff --git a/sentry-ruby/spec/sentry/configuration_spec.rb b/sentry-ruby/spec/sentry/configuration_spec.rb index 68d29589c..fc4ecd8b0 100644 --- a/sentry-ruby/spec/sentry/configuration_spec.rb +++ b/sentry-ruby/spec/sentry/configuration_spec.rb @@ -233,21 +233,19 @@ end end - context "with a sample rate" do - before(:each) do - subject.dsn = 'http://12345:67890@sentry.localdomain:3000/sentry/42' + describe "#sample_allowed?" do + before do subject.sample_rate = 0.75 end it 'captured_allowed false when sampled' do allow(Random).to receive(:rand).and_return(0.76) - expect(subject.sending_allowed?).to eq(false) - expect(subject.errors).to eq(["Excluded by random sample"]) + expect(subject.sample_allowed?).to eq(false) end it 'captured_allowed true when not sampled' do allow(Random).to receive(:rand).and_return(0.74) - expect(subject.sending_allowed?).to eq(true) + expect(subject.sample_allowed?).to eq(true) end end From 3d55ef2c8cd3b6f7c9756dfc5364960f7678ebbb Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 31 Oct 2021 09:11:39 +0000 Subject: [PATCH 4/6] Check if the event is sampled in Client#capture_event --- sentry-ruby/lib/sentry/client.rb | 1 + .../spec/sentry/client/event_sending_spec.rb | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index a27141d6e..8adc2bf75 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -25,6 +25,7 @@ def initialize(configuration) def capture_event(event, scope, hint = {}) return unless configuration.sending_allowed? + return unless configuration.sample_allowed? || event.is_a?(TransactionEvent) event = scope.apply_to_event(event, hint) diff --git a/sentry-ruby/spec/sentry/client/event_sending_spec.rb b/sentry-ruby/spec/sentry/client/event_sending_spec.rb index 3a22ef6b0..289f522ab 100644 --- a/sentry-ruby/spec/sentry/client/event_sending_spec.rb +++ b/sentry-ruby/spec/sentry/client/event_sending_spec.rb @@ -19,6 +19,39 @@ let(:scope) { Sentry::Scope.new } let(:event) { subject.event_from_message(message) } + context "with sample_rate set" do + before do + configuration.sample_rate = 0.5 + configuration.background_worker_threads = 0 + end + + context "with Event" do + it "sends the event when it's sampled" do + allow(Random).to receive(:rand).and_return(0.49) + subject.capture_event(event, scope) + expect(subject.transport.events.count).to eq(1) + end + + it "doesn't send the event when it's not sampled" do + allow(Random).to receive(:rand).and_return(0.51) + subject.capture_event(event, scope) + expect(subject.transport.events.count).to eq(0) + end + end + + context "with TransactionEvent" do + require "debug" + it "ignores the sampling" do + transaction_event = subject.event_from_transaction(Sentry::Transaction.new(hub: hub)) + allow(Random).to receive(:rand).and_return(0.51) + + subject.capture_event(transaction_event, scope) + + expect(subject.transport.events.count).to eq(1) + end + end + end + context 'with config.async set' do let(:async_block) do lambda do |event| From 915a1056b37bd48cfb8d2cadff57f99daa3af2dc Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 31 Oct 2021 09:16:32 +0000 Subject: [PATCH 5/6] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92eca8ea3..3aa5fc105 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ - Connect `Sidekiq`'s transaction with its parent when possible [#1590](https://github.com/getsentry/sentry-ruby/pull/1590) - Fixes [#1586](https://github.com/getsentry/sentry-ruby/issues/1586) - Use nil instead of false to disable callable settings [#1594](https://github.com/getsentry/sentry-ruby/pull/1594) +- Avoid duplicated sampling on Transaction events [#1601](https://github.com/getsentry/sentry-ruby/pull/1601) ## 4.7.3 From 51248ceea81509bfb3aa8169820a5d7b06b94bf1 Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 4 Nov 2021 17:53:48 +0000 Subject: [PATCH 6/6] Update sentry-ruby/lib/sentry/client.rb Co-authored-by: Neel Shah --- sentry-ruby/lib/sentry/client.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry-ruby/lib/sentry/client.rb b/sentry-ruby/lib/sentry/client.rb index 8adc2bf75..d55e49a73 100644 --- a/sentry-ruby/lib/sentry/client.rb +++ b/sentry-ruby/lib/sentry/client.rb @@ -25,7 +25,7 @@ def initialize(configuration) def capture_event(event, scope, hint = {}) return unless configuration.sending_allowed? - return unless configuration.sample_allowed? || event.is_a?(TransactionEvent) + return unless event.is_a?(TransactionEvent) || configuration.sample_allowed? event = scope.apply_to_event(event, hint)