Logs are misleading when transaction is sampled due to parent being sampled

[rmsy]

Issue Description

Hello 👋

If a transaction has a parent which is sampled, the tracing debug logs are slightly misleading. Due to the precedence of the conditionals below, the sample_rate may be false or 0.0 if a sampler proc is defined and returns one of them:

sentry-ruby/sentry-ruby/lib/sentry/transaction.rb

Lines 108 to 115 in 8538717

	sample_rate =
	if @traces_sampler.is_a?(Proc)
	@traces_sampler.call(sampling_context)
	elsif !sampling_context[:parent_sampled].nil?
	sampling_context[:parent_sampled]
	else
	@traces_sample_rate
	end

This results in the following log message being logged, indicating the transaction will be discarded:

sentry-ruby/sentry-ruby/lib/sentry/transaction.rb

Lines 125 to 129 in 8538717

	if sample_rate == 0.0 || sample_rate == false
	@sampled = false
	log_debug("#{MESSAGE_PREFIX} Discarding #{transaction_description} because traces_sampler returned 0 or false")
	return
	end

However, if @parent_sampled is true for the transaction, it is still sent in finish:

sentry-ruby/sentry-ruby/lib/sentry/transaction.rb

Lines 167 to 173 in 8538717

	unless @sampled || @parent_sampled
	hub.current_client.transport.record_lost_event(:sample_rate, 'transaction')
	return
	end
	event = hub.current_client.event_from_transaction(self)
	hub.capture_event(event)

I assume the first two conditionals in the sample_rate assignment can just be reversed, but I wasn't 100% sure if doing that would have any other side-effects, so I opened this issue instead of a PR.

Reproduction Steps

1. Configure Sentry to have a traces_sampler proc which returns false or 0.0
2. Send a request to Rails with a Sentry-Trace header
3. Observe the debug log message indicating the transaction will be discarded
4. Observe that the transaction is still sent to Sentry

Expected Behavior

The log message should not indicate that the transaction will be discarded.

Actual Behavior

The log message indicates that the transaction will be discarded.

Ruby Version

2.7.4

SDK Version

4.8.2

Integration and Its Version

Rails

Sentry Config

No response

[st0012]

After digging into this a bit, I think this there's a conflict between 2 tracing requirements or my understanding on them.

1. We should prioritize traces_sampler's decision over inherited decision. This follows sentry-python's implementation.
2. Always send the transaction if its parent is sampled, which is the @parent_sampled in this line

sentry-ruby/sentry-ruby/lib/sentry/transaction.rb

Line 167 in 8538717

unless @sampled || @parent_sampled

I think 2 is probably wrong as sentry-python doesn't implement this and it kind of disobeys the decision of set_initial_sample_decision. So removing the @parent_sampled part should be the right thing to do.

@sl0thentr0py Can you help me check if the above is true?

[sl0thentr0py]

@st0012, the official precedence order requirement is written down here, so I believe yes, 1. is correct - traces_sampler takes priority.

[st0012]

@sl0thentr0py sorry that I wasn't clear about what "the above" meant. I meant that my 2nd point was incorrect and the condition should be changed.

[sl0thentr0py]

yes, 2. is incorrect and the condition should be changed. The transaction should be sent based on the sampling decision in 1. where traces_sampler takes priority over the parent.

[st0012]

@sl0thentr0py thanks for the confirmation!

@rmsy in this case, the logging logic is fine. it's the transaction should not be sent, which I'll fix shortly.

if you want to keep the current behavior (send unsampled transaction based on its parent's decision), you need prioritize the inherited sampling decision in your traces_sampler:

config.traces_sampler = lambda do |sampling_context|
  parent_sampled = sampling_context[:parent_sampled]

  if !parent_sampled.nil?
    parent_sampled
  else
    # the rest of sampling logic
  end
end