Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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 @@
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)

Check warning on line 104 in sentry_sdk/integrations/sqlalchemy.py

View check run for this annotation

@sentry/warden / warden: find-bugs

StreamedSpan handling code is unreachable - spans are never StreamedSpan instances

The `isinstance(span, StreamedSpan)` check on line 101 will never be true. The span comes from `record_sql_queries()` which internally calls `sentry_sdk.start_span()`. When span streaming is enabled, `sentry_sdk.start_span()` returns `NoOpSpan` (not `StreamedSpan`). When streaming is disabled, it returns `Span`. Neither is a `StreamedSpan`, making this error handling code unreachable.
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 @@
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)

Check warning on line 177 in sentry_sdk/integrations/sqlalchemy.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[FY5-ZHS] StreamedSpan handling code is unreachable - spans are never StreamedSpan instances (additional location)

The `isinstance(span, StreamedSpan)` check on line 101 will never be true. The span comes from `record_sql_queries()` which internally calls `sentry_sdk.start_span()`. When span streaming is enabled, `sentry_sdk.start_span()` returns `NoOpSpan` (not `StreamedSpan`). When streaming is disabled, it returns `Span`. Neither is a `StreamedSpan`, making this error handling code unreachable.
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)
29 changes: 21 additions & 8 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,23 +476,36 @@

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

by_parent = {}
for span in event["spans"]:
print("spans are", spans)
for span in spans:
print(span)

Check warning on line 487 in tests/conftest.py

View check run for this annotation

@sentry/warden / warden: code-review

[B9C-6RJ] Potential runtime error if no root span found in streamed spans (additional location)

When `root_span=None` (streaming mode), the code attempts to find a root span by looking for a span without `parent_span_id`. If no such span exists in the list, `root_span` remains `None`, and line 509 will raise a runtime error when calling `render_span(root_span)`. This could cause cryptic test failures.
Comment thread
alexander-alderman-webb marked this conversation as resolved.
Outdated
if "parent_span_id" not in span:
root_span = span
continue

Check warning on line 490 in tests/conftest.py

View check run for this annotation

@sentry/warden / warden: code-review

Potential runtime error if no root span found in streamed spans

When `root_span=None` (streaming mode), the code attempts to find a root span by looking for a span without `parent_span_id`. If no such span exists in the list, `root_span` remains `None`, and line 509 will raise a runtime error when calling `render_span(root_span)`. This could cause cryptic test failures.
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