Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
73 changes: 51 additions & 22 deletions sentry_sdk/integrations/sqlalchemy.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
ensure_integration_enabled,
parse_version,
)
from sentry_sdk.traces import StreamedSpan, SpanStatus

try:
from sqlalchemy.engine import Engine # type: ignore
Expand All @@ -20,6 +21,7 @@
from typing import Any
from typing import ContextManager
from typing import Optional
from typing import Union

from sentry_sdk.tracing import Span

Expand Down Expand Up @@ -96,7 +98,10 @@ def _handle_error(context: "Any", *args: "Any") -> None:
span: "Optional[Span]" = getattr(execution_context, "_sentry_sql_span", None)

if span is not None:
span.set_status(SPANSTATUS.INTERNAL_ERROR)
if isinstance(span, StreamedSpan):
span.status = SpanStatus.ERROR
else:
span.set_status(SPANSTATUS.INTERNAL_ERROR)
Comment thread
alexander-alderman-webb marked this conversation as resolved.

# _after_cursor_execute does not get called for crashing SQL stmts. Judging
# from SQLAlchemy codebase it does seem like any error coming into this
Comment thread
alexander-alderman-webb marked this conversation as resolved.
Expand Down Expand Up @@ -132,29 +137,53 @@ def _get_db_system(name: str) -> "Optional[str]":
return None


def _set_db_data(span: "Span", conn: "Any") -> None:
def _set_db_data(span: "Union[Span, StreamedSpan]", conn: "Any") -> None:
db_system = _get_db_system(conn.engine.name)
if db_system is not None:
span.set_data(SPANDATA.DB_SYSTEM, db_system)

try:
driver = conn.dialect.driver
if driver:
span.set_data(SPANDATA.DB_DRIVER_NAME, driver)
except Exception:
pass
if isinstance(span, StreamedSpan):
if db_system is not None:
span.set_attribute(SPANDATA.DB_SYSTEM, db_system)

try:
driver = conn.dialect.driver
if driver:
span.set_attribute(SPANDATA.DB_DRIVER_NAME, driver)
except Exception:
pass
else:
if db_system is not None:
span.set_data(SPANDATA.DB_SYSTEM, db_system)

try:
driver = conn.dialect.driver
if driver:
span.set_data(SPANDATA.DB_DRIVER_NAME, driver)
except Exception:
pass

if conn.engine.url is None:
return

db_name = conn.engine.url.database
if db_name is not None:
span.set_data(SPANDATA.DB_NAME, db_name)

server_address = conn.engine.url.host
if server_address is not None:
span.set_data(SPANDATA.SERVER_ADDRESS, server_address)

server_port = conn.engine.url.port
if server_port is not None:
span.set_data(SPANDATA.SERVER_PORT, server_port)
if isinstance(span, StreamedSpan):
db_name = conn.engine.url.database
if db_name is not None:
span.set_attribute(SPANDATA.DB_NAME, db_name)

server_address = conn.engine.url.host
if server_address is not None:
span.set_attribute(SPANDATA.SERVER_ADDRESS, server_address)

server_port = conn.engine.url.port
if server_port is not None:
span.set_attribute(SPANDATA.SERVER_PORT, server_port)
else:
db_name = conn.engine.url.database
if db_name is not None:
span.set_data(SPANDATA.DB_NAME, db_name)

server_address = conn.engine.url.host
if server_address is not None:
span.set_data(SPANDATA.SERVER_ADDRESS, server_address)

server_port = conn.engine.url.port
if server_port is not None:
span.set_data(SPANDATA.SERVER_PORT, server_port)
31 changes: 22 additions & 9 deletions sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@
span_origin: str = "manual",
) -> "Generator[sentry_sdk.tracing.Span, None, None]":
# TODO: Bring back capturing of params by default
if sentry_sdk.get_client().options["_experiments"].get("record_sql_params", False):
client = sentry_sdk.get_client()
if client.options["_experiments"].get("record_sql_params", False):
if not params_list or params_list == [None]:
params_list = None

Expand All @@ -160,14 +161,26 @@
with capture_internal_exceptions():
sentry_sdk.add_breadcrumb(message=query, category="query", data=data)

with sentry_sdk.start_span(
op=OP.DB,
name=query,
origin=span_origin,
) as span:
for k, v in data.items():
span.set_data(k, v)
yield span
if has_span_streaming_enabled(client.options):
with sentry_sdk.traces.start_span(
name=query,
attributes={
"sentry.origin": span_origin,
"sentry.op": OP.DB,
},
) as span:
for k, v in data.items():
span.set_attribute(k, v)
yield span

Check warning on line 174 in sentry_sdk/tracing_utils.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Query source information lost when span streaming is enabled

When span streaming is enabled, `record_sql_queries` yields a `StreamedSpan`. The span's `_end()` method (called via `__exit__`) captures and sends the span to Sentry immediately. However, `add_query_source()` is called AFTER the span context manager exits in the SQLAlchemy integration. This means any attributes set by `add_query_source()` (like `code.line.number`, `code.file.path`, `code.function.name`) will be lost because the span has already been transmitted.
Comment thread
alexander-alderman-webb marked this conversation as resolved.
else:
with sentry_sdk.start_span(
op=OP.DB,
name=query,
Comment thread
alexander-alderman-webb marked this conversation as resolved.
origin=span_origin,
) as span:
for k, v in data.items():
span.set_data(k, v)
yield span


def maybe_create_breadcrumbs_from_span(
Expand Down
28 changes: 20 additions & 8 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,23 +476,35 @@

@pytest.fixture
def render_span_tree():
def inner(event):
assert event["type"] == "transaction"
def inner(spans, root_span=None):

Check failure on line 479 in tests/conftest.py

View check run for this annotation

@sentry/warden / warden: code-review

Breaking API change: render_span_tree signature change breaks existing callers

The `render_span_tree` fixture signature changed from `inner(event)` (accepting a full transaction event dict) to `inner(spans, root_span=None)` (accepting a spans list). Existing callers in `test_basic.py` (lines 460, 599, 957, 1037, etc.) pass a full event dict like `render_span_tree(transaction)` which will now be incorrectly interpreted as a spans list, causing iteration over dict keys instead of spans and runtime errors.
streamed_spans = False
if root_span is None:
streamed_spans = True

by_parent = {}
for span in event["spans"]:
for span in spans:
print(span)
Comment thread
alexander-alderman-webb marked this conversation as resolved.
Outdated
if "parent_span_id" not in span:
root_span = span
continue
Comment thread
alexander-alderman-webb marked this conversation as resolved.

Comment thread
alexander-alderman-webb marked this conversation as resolved.
by_parent.setdefault(span["parent_span_id"], []).append(span)

def render_span(span):
yield "- op={}: description={}".format(
json.dumps(span.get("op")), json.dumps(span.get("description"))
)
if streamed_spans:
yield "- sentry.op={}: name={}".format(
json.dumps(span["attributes"].get("sentry.op")),
json.dumps(span["name"]),
)
else:
yield "- op={}: description={}".format(
json.dumps(span.get("op")), json.dumps(span.get("description"))
)

for subspan in by_parent.get(span["span_id"]) or ():
for line in render_span(subspan):
yield " {}".format(line)

root_span = event["contexts"]["trace"]

return "\n".join(render_span(root_span))

return inner
Expand Down
3 changes: 2 additions & 1 deletion tests/integrations/django/asgi/test_asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,9 @@ async def test_async_middleware_spans(

(transaction,) = events

assert transaction["type"] == "transaction"
assert (
render_span_tree(transaction)
render_span_tree(transaction["spans"], transaction["contexts"]["trace"])
== """\
- op="http.server": description=null
- op="event.django": description="django.db.reset_queries"
Expand Down
Loading
Loading