Skip to content
Open
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
45 changes: 32 additions & 13 deletions sentry_sdk/integrations/sqlalchemy.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
from sentry_sdk.consts import SPANSTATUS, SPANDATA
from sentry_sdk.integrations import _check_minimum_version, Integration, DidNotEnable
from sentry_sdk.tracing_utils import add_query_source, record_sql_queries
from sentry_sdk.tracing_utils import (
add_query_source,
record_sql_queries_supporting_streaming,
)
from sentry_sdk.utils import (
capture_internal_exceptions,
ensure_integration_enabled,
parse_version,
)
from sentry_sdk.traces import StreamedSpan, SpanStatus
from sentry_sdk.tracing import Span

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

from sentry_sdk.tracing import Span
from typing import Union


class SqlalchemyIntegration(Integration):
Expand All @@ -48,7 +52,7 @@ def _before_cursor_execute(
executemany: bool,
*args: "Any",
) -> None:
ctx_mgr = record_sql_queries(
ctx_mgr = record_sql_queries_supporting_streaming(
cursor,
statement,
parameters,
Expand Down Expand Up @@ -78,12 +82,19 @@ def _after_cursor_execute(
context, "_sentry_sql_span_manager", None
)

# Record query source immediately before span is finished: accurate end timestamp and before the span is flushed.
span: "Optional[Union[Span, StreamedSpan]]" = getattr(
context, "_sentry_sql_span", None
)
if isinstance(span, StreamedSpan):
with capture_internal_exceptions():
add_query_source(span)

if ctx_mgr is not None:
context._sentry_sql_span_manager = None
ctx_mgr.__exit__(None, None, None)

span: "Optional[Span]" = getattr(context, "_sentry_sql_span", None)
if span is not None:
if isinstance(span, Span):
with capture_internal_exceptions():
add_query_source(span)

Expand All @@ -96,7 +107,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,15 +146,20 @@ 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:
if isinstance(span, StreamedSpan):
set_on_span = span.set_attribute
else:
set_on_span = span.set_data

db_system = _get_db_system(conn.engine.name)
if db_system is not None:
span.set_data(SPANDATA.DB_SYSTEM, db_system)
set_on_span(SPANDATA.DB_SYSTEM, db_system)
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.

db.name was deprecated in favor of db.system.name, so we should use that in streaming mode


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

Expand All @@ -149,12 +168,12 @@ def _set_db_data(span: "Span", conn: "Any") -> None:

db_name = conn.engine.url.database
if db_name is not None:
span.set_data(SPANDATA.DB_NAME, db_name)
set_on_span(SPANDATA.DB_NAME, db_name)
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.

This should be db.namespace in streaming mode


server_address = conn.engine.url.host
if server_address is not None:
span.set_data(SPANDATA.SERVER_ADDRESS, server_address)
set_on_span(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)
set_on_span(SPANDATA.SERVER_PORT, server_port)
83 changes: 78 additions & 5 deletions sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,65 @@ def record_sql_queries(
yield span


@contextlib.contextmanager
def record_sql_queries_supporting_streaming(
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.

Any reason not to adapt the existing record_sql_queries instead? Why a new func, if we still support both streaming and legacy mode in it?

cursor: "Any",
query: "Any",
params_list: "Any",
paramstyle: "Optional[str]",
executemany: bool,
record_cursor_repr: bool = False,
span_origin: str = "manual",
) -> "Generator[Union[sentry_sdk.tracing.Span, sentry_sdk.traces.StreamedSpan], None, None]":
# TODO: Bring back capturing of params by default
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

if paramstyle == "pyformat":
paramstyle = "format"
else:
params_list = None
paramstyle = None

query = _format_sql(cursor, query)

data = {}
if params_list is not None:
data["db.params"] = params_list
if paramstyle is not None:
data["db.paramstyle"] = paramstyle
if executemany:
data["db.executemany"] = True
if record_cursor_repr and cursor is not None:
data["db.cursor"] = cursor
Comment on lines +198 to +205
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.

We're setting these as attributes couple lines later, but none of them seems to be in conventions?


with capture_internal_exceptions():
sentry_sdk.add_breadcrumb(message=query, category="query", data=data)

if has_span_streaming_enabled(client.options):
with sentry_sdk.traces.start_span(
name="<unknown SQL query>" if query is None else 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
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(
scope: "sentry_sdk.Scope", span: "sentry_sdk.tracing.Span"
) -> None:
Expand Down Expand Up @@ -316,22 +375,36 @@ def add_source(
span.set_attribute(SPANDATA.CODE_FUNCTION, frame.f_code.co_name)


def add_query_source(span: "sentry_sdk.tracing.Span") -> None:
def add_query_source(
span: "Union[sentry_sdk.tracing.Span, sentry_sdk.traces.StreamedSpan]",
) -> None:
"""
Adds OTel compatible source code information to a database query span
"""
client = sentry_sdk.get_client()
if not client.is_active():
return

if span.timestamp is None or span.start_timestamp is None:
if isinstance(span, LegacySpan):
if not client.is_active():
return
Comment on lines +387 to +388
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.

I believe this check should apply to both streaming and non streaming modes, so I'd move it outside of the isinstance branch


# In the StreamedSpan case, we need to add the extra span information before
# the span finishes, so it's expected that this will be None. In the LegacySpan case,
# it should already be finished.
if span.timestamp is None:
return

if span.start_timestamp is None:
return

should_add_query_source = client.options.get("enable_db_query_source", True)
if not should_add_query_source:
return

duration = span.timestamp - span.start_timestamp
end_timestamp = (
datetime.now(timezone.utc) if span.timestamp is None else span.timestamp
)

duration = end_timestamp - span.start_timestamp
threshold = client.options.get("db_query_source_threshold_ms", 0)
slow_query = duration / timedelta(milliseconds=1) > threshold

Expand Down
27 changes: 19 additions & 8 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,23 +476,34 @@ def maybe_monkeypatched_threading(request):

@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"]:
for span in spans:
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
33 changes: 25 additions & 8 deletions tests/integrations/django/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ def test_response_trace(sentry_init, client, capture_events, render_span_tree):

assert (
'- op="view.response.render": description="serialize response"'
in render_span_tree(events[0])
in render_span_tree(events[0]["spans"], events[0]["contexts"]["trace"])
)


Expand Down Expand Up @@ -596,7 +596,9 @@ def test_django_connect_trace(sentry_init, client, capture_events, render_span_t
data = span.get("data")
assert data.get(SPANDATA.DB_SYSTEM) == "postgresql"

assert '- op="db": description="connect"' in render_span_tree(event)
assert '- op="db": description="connect"' in render_span_tree(
event["spans"], event["contexts"]["trace"]
)


@pytest.mark.forked
Expand Down Expand Up @@ -954,7 +956,9 @@ def test_render_spans(sentry_init, client, capture_events, render_span_tree):
events = capture_events()
client.get(url)
transaction = events[0]
assert expected_line in render_span_tree(transaction)
assert expected_line in render_span_tree(
transaction["spans"], transaction["contexts"]["trace"]
)


@pytest.mark.skipif(DJANGO_VERSION < (1, 9), reason="Requires Django >= 1.9")
Expand Down Expand Up @@ -1034,7 +1038,10 @@ def test_middleware_spans(sentry_init, client, capture_events, render_span_tree)
message, transaction = events

assert message["message"] == "hi"
assert render_span_tree(transaction) == EXPECTED_MIDDLEWARE_SPANS
assert (
render_span_tree(transaction["spans"], transaction["contexts"]["trace"])
== EXPECTED_MIDDLEWARE_SPANS
)


def test_middleware_spans_disabled(sentry_init, client, capture_events):
Expand Down Expand Up @@ -1075,7 +1082,10 @@ def test_signals_spans(sentry_init, client, capture_events, render_span_tree):
message, transaction = events

assert message["message"] == "hi"
assert render_span_tree(transaction) == EXPECTED_SIGNALS_SPANS
assert (
render_span_tree(transaction["spans"], transaction["contexts"]["trace"])
== EXPECTED_SIGNALS_SPANS
)

assert transaction["spans"][0]["op"] == "event.django"
assert transaction["spans"][0]["description"] == "django.db.reset_queries"
Expand Down Expand Up @@ -1127,7 +1137,10 @@ def test_signals_spans_filtering(sentry_init, client, capture_events, render_spa

(transaction,) = events

assert render_span_tree(transaction) == EXPECTED_SIGNALS_SPANS_FILTERED
assert (
render_span_tree(transaction["spans"], transaction["contexts"]["trace"])
== EXPECTED_SIGNALS_SPANS_FILTERED
)

assert transaction["spans"][0]["op"] == "event.django"
assert transaction["spans"][0]["description"] == "django.db.reset_queries"
Expand Down Expand Up @@ -1206,7 +1219,9 @@ def test_custom_urlconf_middleware(
event = events.pop(0)
assert event["transaction"] == "/custom/ok"
if middleware_spans:
assert "custom_urlconf_middleware" in render_span_tree(event)
assert "custom_urlconf_middleware" in render_span_tree(
event["spans"], event["contexts"]["trace"]
)

_content, status, _headers = unpack_werkzeug_response(client.get("/custom/exc"))
assert status.lower() == "500 internal server error"
Expand All @@ -1216,7 +1231,9 @@ def test_custom_urlconf_middleware(
assert error_event["exception"]["values"][-1]["mechanism"]["type"] == "django"
assert transaction_event["transaction"] == "/custom/exc"
if middleware_spans:
assert "custom_urlconf_middleware" in render_span_tree(transaction_event)
assert "custom_urlconf_middleware" in render_span_tree(
transaction_event["spans"], transaction_event["contexts"]["trace"]
)

settings.MIDDLEWARE.pop(0)

Expand Down
Loading
Loading