Skip to content

fix(tracing): Fix span nesting concurrency issues #4409

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

Closed
wants to merge 3 commits into from
Closed
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
7 changes: 7 additions & 0 deletions sentry_sdk/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,13 @@ def start_span(
**kwargs, # type: Any
):
# type: (...) -> Span
if kwargs.pop("scope", None) is not None:
warnings.warn(
"The `scope` parameter is deprecated, and its value is ignored.",
DeprecationWarning,
stacklevel=2,
)

return get_current_scope().start_span(**kwargs)


Expand Down
2 changes: 1 addition & 1 deletion sentry_sdk/integrations/langchain.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def _handle_error(self, run_id, error):
span_data = self.span_map[run_id]
if not span_data:
return
sentry_sdk.capture_exception(error, span_data.span.scope)
sentry_sdk.capture_exception(error)
span_data.span.__exit__(None, None, None)
del self.span_map[run_id]

Expand Down
42 changes: 23 additions & 19 deletions sentry_sdk/scope.py
Original file line number Diff line number Diff line change
Expand Up @@ -1122,32 +1122,36 @@ def start_span(self, instrumenter=INSTRUMENTER.SENTRY, **kwargs):
stacklevel=2,
)

with new_scope():
kwargs.setdefault("scope", self)
if kwargs.pop("scope", None) is not None:
warnings.warn(
"The `scope` parameter is deprecated, and its value is ignored.",
DeprecationWarning,
stacklevel=2,
)

client = self.get_client()
client = self.get_client()

configuration_instrumenter = client.options["instrumenter"]
configuration_instrumenter = client.options["instrumenter"]

if instrumenter != configuration_instrumenter:
return NoOpSpan()
if instrumenter != configuration_instrumenter:
return NoOpSpan()

# get current span or transaction
span = self.span or self.get_isolation_scope().span
# get current span or transaction
span = self.span or self.get_isolation_scope().span

if span is None:
# New spans get the `trace_id` from the scope
if "trace_id" not in kwargs:
propagation_context = self.get_active_propagation_context()
if propagation_context is not None:
kwargs["trace_id"] = propagation_context.trace_id
if span is None:
# New spans get the `trace_id` from the scope
if "trace_id" not in kwargs:
propagation_context = self.get_active_propagation_context()
if propagation_context is not None:
kwargs["trace_id"] = propagation_context.trace_id

span = Span(**kwargs)
else:
# Children take `trace_id`` from the parent span.
span = span.start_child(**kwargs)
span = Span(**kwargs)
else:
# Children take `trace_id`` from the parent span.
span = span.start_child(**kwargs)

return span
return span

def continue_trace(
self, environ_or_headers, op=None, name=None, source=None, origin="manual"
Expand Down
129 changes: 65 additions & 64 deletions sentry_sdk/tracing.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ class Span:
"name",
"_flags",
"_flags_capacity",
"scope_manager",
)

def __init__(
Expand All @@ -301,6 +302,24 @@ def __init__(
name=None, # type: Optional[str]
):
# type: (...) -> None
if hub is not None:
warnings.warn(
"The `hub` parameter is deprecated, and its value is ignored.",
DeprecationWarning,
stacklevel=2,
)

del hub

if scope is not None:
warnings.warn(
"The `scope` parameter is deprecated, and its value is ignored.",
DeprecationWarning,
stacklevel=2,
)

del scope

self.trace_id = trace_id or uuid.uuid4().hex
self.span_id = span_id or uuid.uuid4().hex[16:]
self.parent_span_id = parent_span_id
Expand All @@ -309,8 +328,6 @@ def __init__(
self.op = op
self.description = name or description
self.status = status
self.hub = hub # backwards compatibility
self.scope = scope
self.origin = origin
self._measurements = {} # type: Dict[str, MeasurementValue]
self._tags = {} # type: MutableMapping[str, str]
Expand All @@ -319,14 +336,9 @@ def __init__(
self._flags = {} # type: Dict[str, bool]
self._flags_capacity = 10

if hub is not None:
warnings.warn(
"The `hub` parameter is deprecated. Please use `scope` instead.",
DeprecationWarning,
stacklevel=2,
)

self.scope = self.scope or hub.scope
# Backwards compatibility: allow these values to be get and set (but we don't use them)
self.hub = None # type: Optional[sentry_sdk.Hub]
self.scope = None # type: Optional[sentry_sdk.Scope]

if start_timestamp is None:
start_timestamp = datetime.now(timezone.utc)
Expand Down Expand Up @@ -381,21 +393,19 @@ def __repr__(self):

def __enter__(self):
# type: () -> Span
scope = self.scope or sentry_sdk.get_current_scope()
old_span = scope.span
self.scope_manager = sentry_sdk.new_scope()
scope = self.scope_manager.__enter__()
scope.span = self
self._context_manager_state = (scope, old_span)
return self

def __exit__(self, ty, value, tb):
# type: (Optional[Any], Optional[Any], Optional[Any]) -> None
if value is not None and should_be_treated_as_error(ty, value):
self.set_status(SPANSTATUS.INTERNAL_ERROR)

scope, old_span = self._context_manager_state
del self._context_manager_state
self.finish(scope)
scope.span = old_span
try:
if value is not None and should_be_treated_as_error(ty, value):
self.set_status(SPANSTATUS.INTERNAL_ERROR)
self.finish()
finally:
self.scope_manager.__exit__(ty, value, tb)

@property
def containing_transaction(self):
Expand Down Expand Up @@ -429,6 +439,13 @@ def start_child(self, instrumenter=INSTRUMENTER.SENTRY, **kwargs):
stacklevel=2,
)

if kwargs.pop("scope", None) is not None:
warnings.warn(
"The `scope` parameter is deprecated, and its value is ignored.",
DeprecationWarning,
stacklevel=2,
)

configuration_instrumenter = sentry_sdk.get_client().options["instrumenter"]

if instrumenter != configuration_instrumenter:
Expand Down Expand Up @@ -684,8 +701,7 @@ def finish(self, scope=None, end_timestamp=None):
except AttributeError:
self.timestamp = datetime.now(timezone.utc)

scope = scope or sentry_sdk.get_current_scope()
maybe_create_breadcrumbs_from_span(scope, self)
maybe_create_breadcrumbs_from_span(self)

return None

Expand Down Expand Up @@ -819,6 +835,13 @@ def __init__( # type: ignore[misc]
):
# type: (...) -> None

if kwargs.pop("scope", None) is not None:
warnings.warn(
"The `scope` parameter is deprecated, and its value is ignored.",
DeprecationWarning,
stacklevel=2,
)

super().__init__(**kwargs)

self.name = name
Expand Down Expand Up @@ -906,39 +929,6 @@ def containing_transaction(self):
# reference.
return self

def _get_scope_from_finish_args(
self,
scope_arg, # type: Optional[Union[sentry_sdk.Scope, sentry_sdk.Hub]]
hub_arg, # type: Optional[Union[sentry_sdk.Scope, sentry_sdk.Hub]]
):
# type: (...) -> Optional[sentry_sdk.Scope]
"""
Logic to get the scope from the arguments passed to finish. This
function exists for backwards compatibility with the old finish.

TODO: Remove this function in the next major version.
"""
scope_or_hub = scope_arg
if hub_arg is not None:
warnings.warn(
"The `hub` parameter is deprecated. Please use the `scope` parameter, instead.",
DeprecationWarning,
stacklevel=3,
)

scope_or_hub = hub_arg

if isinstance(scope_or_hub, sentry_sdk.Hub):
warnings.warn(
"Passing a Hub to finish is deprecated. Please pass a Scope, instead.",
DeprecationWarning,
stacklevel=3,
)

return scope_or_hub.scope

return scope_or_hub

def finish(
self,
scope=None, # type: Optional[sentry_sdk.Scope]
Expand All @@ -961,17 +951,28 @@ def finish(
:return: The event ID if the transaction was sent to Sentry,
otherwise None.
"""
if hub is not None:
warnings.warn(
"The `hub` parameter is deprecated, and its value is ignored.",
DeprecationWarning,
stacklevel=2,
)

del hub

if scope is not None:
warnings.warn(
"The `scope` parameter is deprecated, and its value is ignored.",
DeprecationWarning,
stacklevel=2,
)

del scope

if self.timestamp is not None:
# This transaction is already finished, ignore.
return None

# For backwards compatibility, we must handle the case where `scope`
# or `hub` could both either be a `Scope` or a `Hub`.
scope = self._get_scope_from_finish_args(
scope, hub
) # type: Optional[sentry_sdk.Scope]

scope = scope or self.scope or sentry_sdk.get_current_scope()
client = sentry_sdk.get_client()

if not client.is_active():
Expand Down Expand Up @@ -1008,7 +1009,7 @@ def finish(
)
self.name = "<unlabeled transaction>"

super().finish(scope, end_timestamp)
super().finish(end_timestamp=end_timestamp)

if not self.sampled:
# At this point a `sampled = None` should have already been resolved
Expand Down Expand Up @@ -1067,7 +1068,7 @@ def finish(
if metrics_summary:
event["_metrics_summary"] = metrics_summary

return scope.capture_event(event)
return sentry_sdk.capture_event(event)

def set_measurement(self, name, value, unit=""):
# type: (str, float, MeasurementUnit) -> None
Expand Down
12 changes: 6 additions & 6 deletions sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,10 @@ def record_sql_queries(
yield span


def maybe_create_breadcrumbs_from_span(scope, span):
# type: (sentry_sdk.Scope, sentry_sdk.tracing.Span) -> None
def maybe_create_breadcrumbs_from_span(span):
# type: (sentry_sdk.tracing.Span) -> None
if span.op == OP.DB_REDIS:
scope.add_breadcrumb(
sentry_sdk.add_breadcrumb(
message=span.description, type="redis", category="redis", data=span._tags
)

Expand All @@ -175,14 +175,14 @@ def maybe_create_breadcrumbs_from_span(scope, span):
level = "warning"

if level:
scope.add_breadcrumb(
sentry_sdk.add_breadcrumb(
type="http", category="httplib", data=span._data, level=level
)
else:
scope.add_breadcrumb(type="http", category="httplib", data=span._data)
sentry_sdk.add_breadcrumb(type="http", category="httplib", data=span._data)

elif span.op == "subprocess":
scope.add_breadcrumb(
sentry_sdk.add_breadcrumb(
type="subprocess",
category="subprocess",
message=span.description,
Expand Down
20 changes: 16 additions & 4 deletions tests/integrations/httpx/test_httpx.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,20 @@ def before_breadcrumb(crumb, hint):
events = capture_events()

if asyncio.iscoroutinefunction(httpx_client.get):

async def get_response_and_capture_message():
response = await httpx_client.get(url)
capture_message("Testing!")
return response

response = asyncio.get_event_loop().run_until_complete(
httpx_client.get(url)
get_response_and_capture_message()
)
else:
response = httpx_client.get(url)
capture_message("Testing!")

assert response.status_code == 200
capture_message("Testing!")

(event,) = events

Expand Down Expand Up @@ -84,14 +90,20 @@ def test_crumb_capture_client_error(
events = capture_events()

if asyncio.iscoroutinefunction(httpx_client.get):

async def get_response_and_capture_message():
response = await httpx_client.get(url)
capture_message("Testing!")
return response

response = asyncio.get_event_loop().run_until_complete(
httpx_client.get(url)
get_response_and_capture_message()
)
else:
response = httpx_client.get(url)
capture_message("Testing!")

assert response.status_code == status_code
capture_message("Testing!")

(event,) = events

Expand Down
Loading
Loading