Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable back pressure monitor in Potel #3602

Open
wants to merge 3 commits into
base: potel-base
Choose a base branch
from
Open

Enable back pressure monitor in Potel #3602

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0aba891
Enable back pressure monitor in Potel
antonpirker Oct 3, 2024
662deba
Merge branch 'potel-base' into antonpirker/potel/back-pressure
antonpirker Oct 7, 2024
f325411
Count outcomes only for root-spans
antonpirker Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions sentry_sdk/integrations/opentelemetry/sampler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from random import random
import random

from opentelemetry import trace

Expand Down Expand Up @@ -45,6 +45,18 @@ def dropped_result(span_context):
# type: (SpanContext) -> SamplingResult
trace_state = span_context.trace_state.update(SENTRY_TRACE_STATE_DROPPED, "true")

# Tell Sentry why we dropped the transaction/span
client = sentry_sdk.get_client()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to match the logic in the earlier tracing impl, we should rather do this at the end of the transaction flow, so I'd not do this here but in span processor on_end and only for the root span case. Here it will apply to every span and the numbers will be wrong.

if client.monitor and client.monitor.downsample_factor > 0:
reason = "backpressure"
else:
reason = "sample_rate"

client.transport.record_lost_event(reason, data_category="transaction")

# Only one span (the transaction itself) is discarded, since we did not record any spans here.
client.transport.record_lost_event(reason, data_category="span")

return SamplingResult(
Decision.DROP,
trace_state=trace_state,
Expand Down Expand Up @@ -107,6 +119,11 @@ def should_sample(
# Check if there is a traces_sample_rate
sample_rate = client.options.get("traces_sample_rate")

# Down-sample in case of back pressure monitor says so
# TODO: this should only be done for transactions (aka root spans)
if client.monitor:
sample_rate /= 2**client.monitor.downsample_factor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this below the is_valid_sample_rate block, just before the actual random call is made


# If the sample rate is invalid, drop the span
if not is_valid_sample_rate(sample_rate, source=self.__class__.__name__):
logger.warning(
Expand All @@ -115,7 +132,7 @@ def should_sample(
return dropped_result(parent_span_context)

# Roll the dice on sample rate
sampled = random() < float(sample_rate)
sampled = random.random() < float(sample_rate)

# TODO-neel-potel set sample rate as attribute for DSC
if sampled:
Expand Down
4 changes: 3 additions & 1 deletion tests/test_monitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ def test_transaction_uses_downsampled_rate(

with sentry_sdk.start_transaction(name="foobar") as transaction:
assert transaction.sampled is False
assert transaction.sample_rate == 0.5
assert (
transaction.sample_rate == 0.5
) # TODO: this fails until we put the sample_rate in the POTelSpan

assert Counter(record_lost_event_calls) == Counter(
[
Expand Down
Loading