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.

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

Enable back pressure monitor in Potel #3602

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

Conversation

antonpirker
Copy link
Member

This enables back pressure management in POtel.

It down samples in case of the transport not being healthy and also sends outcomes to Sentry.

Right now the sampling is done on span level, but should be done on transaction level, which is not possible right now, because we do not have a reference to the transaction (root span) in the sampler right now. This will be done in a separate PR.

The test that is updated in this PR still fails, because it needs the sample_rate to be somewhere on the Span, but this will be done in a separate PR.

Copy link

codecov bot commented Oct 3, 2024

❌ 3340 Tests Failed:

Tests completed Failed Passed Skipped
17799 3340 14459 1709
View the top 3 failed tests by shortest run time
tests.integrations.clickhouse_driver.test_clickhouse_driver test_clickhouse_client_spans
Stack Traces | 0s run time
No failure message available
tests.integrations.clickhouse_driver.test_clickhouse_driver test_clickhouse_dbapi_breadcrumbs_with_pii
Stack Traces | 0s run time
No failure message available
tests.integrations.litestar.test_litestar test_middleware_partial_receive_send
Stack Traces | 0s run time
No failure message available

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@@ -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
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.

@antonpirker
Copy link
Member Author

If the transaction is not sampled, the span_processor.on_end() is not run, so we need to do this in the sampler.
I have now updated the logic, to only emit the outcomes for transactions/root-spans.

@antonpirker antonpirker self-assigned this Oct 7, 2024
if span.parent and not span.parent.is_remote:
self._children_spans[span.parent.span_id].append(span)
else:
is_root_span = not (span.parent and not span.parent.is_remote)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
is_root_span = not (span.parent and not span.parent.is_remote)
is_root_span = not span.parent or span.parent.is_remote

@@ -54,6 +54,20 @@ def dropped_result(span_context, attributes, sample_rate=None):
if sample_rate:
trace_state = trace_state.add(TRACESTATE_SAMPLE_RATE_KEY, str(sample_rate))

is_root_span = not (span_context.is_valid and not span_context.is_remote)
Copy link
Member

Choose a reason for hiding this comment

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

if span_context is not valid, why is it a root span?

@@ -124,6 +138,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:
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants