Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 28 additions & 7 deletions sentry_sdk/scope.py
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,7 @@ def get_traceparent(self, *args: "Any", **kwargs: "Any") -> "Optional[str]":

span_streaming = has_span_streaming_enabled(client.options)
# If we have an active span, return traceparent from there
if span_streaming and type(self.streamed_span) is StreamedSpan:
if span_streaming and self.streamed_span is not None:
return self.streamed_span._to_traceparent()
elif not span_streaming and self.span is not None:
return self.span._to_traceparent()
Expand All @@ -617,7 +617,7 @@ def get_baggage(self, *args: "Any", **kwargs: "Any") -> "Optional[Baggage]":

span_streaming = has_span_streaming_enabled(client.options)
# If we have an active span, return baggage from there
if span_streaming and type(self.streamed_span) is StreamedSpan:
if span_streaming and self.streamed_span is not None:
return self.streamed_span._to_baggage()
elif not span_streaming and self.span is not None:
return self.span._to_baggage()
Expand All @@ -632,7 +632,7 @@ def get_trace_context(self) -> "Dict[str, Any]":
if (
has_tracing_enabled(self.get_client().options)
and self._span is not None
and not isinstance(self._span, (NoOpStreamedSpan, NoOpSpan))
and not isinstance(self._span, NoOpSpan)
Comment thread
sentry-warden[bot] marked this conversation as resolved.
):
return self._span._get_trace_context()

Expand Down Expand Up @@ -703,7 +703,7 @@ def iter_trace_propagation_headers(
if (
has_tracing_enabled(client.options)
and span is not None
and not isinstance(span, (NoOpStreamedSpan, NoOpSpan))
and not isinstance(span, NoOpSpan)
):
for header in span._iter_headers():
yield header
Expand Down Expand Up @@ -1295,13 +1295,18 @@ def start_streamed_span(
parent_span = self.streamed_span

# If no eligible parent_span was provided and there is no currently
# active span, this is a segment
# active span, this is a new segment
if parent_span is None:
propagation_context = self.get_active_propagation_context()

if is_ignored_span(name, attributes):
return NoOpStreamedSpan(
scope=self,
segment=None,
trace_id=propagation_context.trace_id,
parent_span_id=propagation_context.parent_span_id,
parent_sampled=propagation_context.parent_sampled,
baggage=propagation_context.baggage,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ignored spans wrong trace propagation

Medium Severity

Segment-level ignored spans create an active NoOpStreamedSpan without a propagation sampled value, so it defaults to False. Outgoing headers now come from that noop via _to_traceparent, which uses sampled, not parent_sampled. Downstream traces can be marked unsampled even when the incoming trace was sampled or should stay deferred.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 46b410a. Configure here.

unsampled_reason="ignored",
)

Expand All @@ -1314,9 +1319,15 @@ def start_streamed_span(
if sample_rate is not None:
self._update_sample_rate(sample_rate)

if sampled is False:
if sampled is False or sampled is None:
return NoOpStreamedSpan(
scope=self,
segment=None,
trace_id=propagation_context.trace_id,
parent_span_id=propagation_context.parent_span_id,
parent_sampled=propagation_context.parent_sampled,
baggage=propagation_context.baggage,
sampled=sampled,
unsampled_reason=outcome,
)

Expand All @@ -1338,11 +1349,21 @@ def start_streamed_span(
with new_scope():
if is_ignored_span(name, attributes):
return NoOpStreamedSpan(
segment=parent_span._segment,
trace_id=parent_span.trace_id,
parent_span_id=parent_span.span_id,
parent_sampled=parent_span.sampled,
unsampled_reason="ignored",
)

if isinstance(parent_span, NoOpStreamedSpan):
return NoOpStreamedSpan(unsampled_reason=parent_span._unsampled_reason)
return NoOpStreamedSpan(
segment=parent_span._segment,
trace_id=parent_span.trace_id,
parent_span_id=parent_span.span_id,
parent_sampled=parent_span.sampled,
unsampled_reason=parent_span._unsampled_reason,
)

return StreamedSpan(
name=name,
Expand Down
43 changes: 31 additions & 12 deletions sentry_sdk/traces.py
Original file line number Diff line number Diff line change
Expand Up @@ -610,15 +610,37 @@ def _to_json(self) -> "SpanJSON":

class NoOpStreamedSpan(StreamedSpan):
__slots__ = (
"_trace_id",
"_span_id",
"_sampled",
"_segment",
"_finished",
"_unsampled_reason",
)

def __init__(
self,
segment: "Optional[StreamedSpan]" = None,
trace_id: "Optional[str]" = None,
parent_span_id: "Optional[str]" = None,
parent_sampled: "Optional[bool]" = None,
baggage: "Optional[Baggage]" = None,
sampled: "Optional[bool]" = False,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Default sampled=False produces contradictory trace propagation headers for ignored child spans of sampled parents

When an ignored child span (is_ignored_span=True) is created inside a sampled real StreamedSpan, sampled is not passed and defaults to False. The outgoing sentry-trace header then carries -0 (explicitly unsampled) while the baggage header, derived from _segment._get_baggage() on the real sampled segment, carries sentry-sampled=true. Downstream services receiving these contradictory headers will typically honor the traceparent's -0 and drop the trace, breaking continuity of a sampled trace.

Evidence
  • scope.py creates NoOpStreamedSpan(segment=parent_span._segment, ..., unsampled_reason="ignored") without passing sampled when is_ignored_span is true for a child of a real StreamedSpan (scope.py ~line 1350).
  • The default sampled=False is stored as self._sampled (line 634 of traces.py).
  • stdlib.py passes the ignored HTTP span explicitly to iter_trace_propagation_headers(span=span) (stdlib.py ~line 160).
  • iter_trace_propagation_headers calls span._iter_headers() because NoOpStreamedSpan is not an instance of NoOpSpan and has_tracing_enabled is True (scope.py ~line 703).
  • Inside _iter_headers(), self._to_traceparent() uses self.sampled = False → appends -0, but self._segment._get_baggage() uses the real sampled StreamedSpan segment whose sampled is True → baggage includes sentry-sampled=true.

Identified by Warden code-review · RXZ-PUR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will verify but we might need to take the segment's sampled flag into account here, instead of the child. It used to not make a difference, but now it does.

unsampled_reason: "Optional[str]" = None,
scope: "Optional[sentry_sdk.Scope]" = None,
) -> None:
self._span_id: "Optional[str]" = None

self._sampled = sampled
self._segment = segment or self

self._trace_id: "Optional[str]" = trace_id
self._parent_span_id = parent_span_id
self._parent_sampled = parent_sampled
self._baggage = baggage
self._sample_rand = None
self._sample_rate = None

self._scope = scope # type: ignore[assignment]
self._unsampled_reason = unsampled_reason

Expand Down Expand Up @@ -693,9 +715,6 @@ def set_attributes(self, attributes: "Attributes") -> None:
def remove_attribute(self, key: str) -> None:
pass

def _is_segment(self) -> bool:
return self._scope is not None

@property
def status(self) -> "str":
return SpanStatus.OK.value
Expand All @@ -716,17 +735,9 @@ def name(self, value: str) -> None:
def active(self) -> bool:
return True

@property
def span_id(self) -> str:
return "0000000000000000"

@property
def trace_id(self) -> str:
return "00000000000000000000000000000000"

@property
def sampled(self) -> "Optional[bool]":
return False
return self._sampled

@property
def start_timestamp(self) -> "Optional[datetime]":
Expand All @@ -736,6 +747,14 @@ def start_timestamp(self) -> "Optional[datetime]":
def end_timestamp(self) -> "Optional[datetime]":
return None

def _get_trace_context(self) -> "dict[str, Any]":
return {
"trace_id": self.trace_id,
"span_id": self.span_id,
"parent_span_id": self._parent_span_id,
"dynamic_sampling_context": self._dynamic_sampling_context(),
}


if TYPE_CHECKING:

Expand Down
33 changes: 22 additions & 11 deletions sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,7 @@ def _fill_sample_rand(self) -> None:

# Get the sample rate and compute the transformation that will map the random value
# to the desired range: [0, 1), [0, sample_rate), or [sample_rate, 1).
sample_rate = try_convert(float, self.baggage.sentry_items.get("sample_rate"))
sample_rate = self._sample_rate()
lower, upper = _sample_rand_range(self.parent_sampled, sample_rate)

try:
Expand All @@ -689,6 +689,13 @@ def _sample_rand(self) -> "Optional[str]":

return self.baggage.sentry_items.get("sample_rand")

def _sample_rate(self) -> "Optional[float]":
"""Convenience method to get the sample_ value from the baggage."""
if self.baggage is None:
return None

return try_convert(float, self.baggage.sentry_items.get("sample_rate"))


class Baggage:
"""
Expand Down Expand Up @@ -857,7 +864,8 @@ def populate_from_segment(cls, segment: "StreamedSpan") -> "Baggage":
options = client.options or {}

sentry_items["trace_id"] = segment.trace_id
sentry_items["sample_rand"] = f"{segment._sample_rand:.6f}" # noqa: E231
if segment._sample_rand is not None:
sentry_items["sample_rand"] = f"{segment._sample_rand:.6f}"

if options.get("environment"):
sentry_items["environment"] = options["environment"]
Expand All @@ -873,8 +881,8 @@ def populate_from_segment(cls, segment: "StreamedSpan") -> "Baggage":
if (
segment.get_attributes().get("sentry.span.source")
not in LOW_QUALITY_SEGMENT_SOURCES
) and segment._name:
sentry_items["transaction"] = segment._name
) and segment.name:
sentry_items["transaction"] = segment.name

if segment._sample_rate is not None:
sentry_items["sample_rate"] = str(segment._sample_rate)
Expand Down Expand Up @@ -1530,23 +1538,23 @@ def _make_sampling_decision(
name: str,
attributes: "Optional[Attributes]",
scope: "sentry_sdk.Scope",
) -> "tuple[bool, Optional[float], Optional[float], Optional[str]]":
) -> "tuple[Optional[bool], Optional[float], Optional[float], Optional[str]]":
"""
Decide whether a span should be sampled.

Returns a tuple with:
1. the sampling decision
1. the sampling decision (sampled, unsampled, deferred)
2. the effective sample rate
3. the sample rand
4. the reason for not sampling the span, if unsampled
"""
client = sentry_sdk.get_client()

if not has_tracing_enabled(client.options):
return False, None, None, None

propagation_context = scope.get_active_propagation_context()

if not has_tracing_enabled(client.options):
return propagation_context.parent_sampled, None, None, None

sample_rand = None
if propagation_context.baggage is not None:
sample_rand = propagation_context.baggage._sample_rand()
Expand All @@ -1572,7 +1580,10 @@ def _make_sampling_decision(
sample_rate = client.options["traces_sampler"](sampling_context)
else:
if propagation_context.parent_sampled is not None:
sample_rate = propagation_context.parent_sampled
if propagation_context._sample_rate() is not None:
sample_rate = propagation_context._sample_rate()
else:
sample_rate = propagation_context.parent_sampled
else:
sample_rate = client.options["traces_sample_rate"]

Expand All @@ -1582,7 +1593,7 @@ def _make_sampling_decision(
logger.warning(f"[Tracing] Discarding {name} because of invalid sample rate.")
return False, None, None, "sample_rate"

sample_rate = float(sample_rate)
sample_rate = float(sample_rate) # type: ignore[arg-type]
if not sample_rate:
if traces_sampler_defined:
reason = "traces_sampler returned 0 or False"
Expand Down
40 changes: 28 additions & 12 deletions tests/integrations/stdlib/test_httplib.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,14 @@ def getresponse(self, *args, **kwargs):
headers = {
"sentry-trace": "771a43a4192642f0b136d5159a501700-1234567890abcdef-1",
"baggage": (
"other-vendor-value-1=foo;bar;baz, sentry-trace_id=771a43a4192642f0b136d5159a501700, "
"sentry-public_key=49d0f7386ad645858ae85020e393bef3, sentry-sample_rate=0.01337, "
"sentry-user_id=Am%C3%A9lie, sentry-sample_rand=0.132521102938283, other-vendor-value-2=foo;bar;"
"other-vendor-value-1=foo;bar;baz,"
"sentry-trace_id=771a43a4192642f0b136d5159a501700,"
"sentry-public_key=49d0f7386ad645858ae85020e393bef3,"
"sentry-sample_rate=0.01337,"
"sentry-user_id=Am%C3%A9lie,"
"sentry-sample_rand=0.000005,"
"sentry-sampled=true,"
"other-vendor-value-2=foo;bar;"
),
}

Expand All @@ -309,6 +314,15 @@ def getresponse(self, *args, **kwargs):
parent_span_id=request_span["span_id"],
sampled=1,
)

expected_outgoing_baggage = (
"sentry-trace_id=771a43a4192642f0b136d5159a501700,"
"sentry-public_key=49d0f7386ad645858ae85020e393bef3,"
"sentry-sample_rate=0.01337,"
"sentry-user_id=Am%C3%A9lie,"
"sentry-sample_rand=0.000005,"
"sentry-sampled=true"
)
else:
events = capture_events()
transaction = continue_trace(headers)
Expand All @@ -331,16 +345,18 @@ def getresponse(self, *args, **kwargs):
sampled=1,
)

assert request_headers["sentry-trace"] == expected_sentry_trace

expected_outgoing_baggage = (
"sentry-trace_id=771a43a4192642f0b136d5159a501700,"
"sentry-public_key=49d0f7386ad645858ae85020e393bef3,"
"sentry-sample_rate=1.0,"
"sentry-user_id=Am%C3%A9lie,"
"sentry-sample_rand=0.132521102938283"
)
# Note: the sample rate here is actually wrong. It's fixed in the
# streaming path
expected_outgoing_baggage = (
"sentry-trace_id=771a43a4192642f0b136d5159a501700,"
"sentry-public_key=49d0f7386ad645858ae85020e393bef3,"
"sentry-sample_rate=1.0,"
"sentry-user_id=Am%C3%A9lie,"
"sentry-sample_rand=0.000005,"
"sentry-sampled=true"
)

assert request_headers["sentry-trace"] == expected_sentry_trace
assert request_headers["baggage"] == expected_outgoing_baggage


Expand Down
Loading
Loading