Avoid duplicated sampling on Transaction events (#1601)

* 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`.

* Remove unused @configuration from Transport

* Remove sampling from general sending restriction logic

* Check if the event is sampled in Client#capture_event

* Update changelog

* Update sentry-ruby/lib/sentry/client.rb

Co-authored-by: Neel Shah <neelshah.sa@gmail.com>

Co-authored-by: Neel Shah <neelshah.sa@gmail.com>

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
 

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 event.is_a?(TransactionEvent) || configuration.sample_allowed?
 
       event = scope.apply_to_event(event, hint)
 

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

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
@@ -27,12 +25,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")
 
@@ -97,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

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|

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
 

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)