Skip to content
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.

Sign up for GitHub

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

fix(sqlalchemy): Use context instead of connection in sqlalchemy integration #1388

Merged
merged 3 commits into from
Apr 11, 2022
Merged

fix(sqlalchemy): Use context instead of connection in sqlalchemy integration #1388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27abc12
Revert "fix(sqlalchemy): Change context manager type to avoid race in…
sl0thentr0py Apr 8, 2022
951e293
Use context instead of conn in sqlalchemy hooks
sl0thentr0py Apr 8, 2022
055aeb2
mypy
sl0thentr0py Apr 8, 2022
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
6 changes: 3 additions & 3 deletions sentry_sdk/integrations/django/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from sentry_sdk.hub import Hub, _should_send_default_pii
from sentry_sdk.scope import add_global_event_processor
from sentry_sdk.serializer import add_global_repr_processor
from sentry_sdk.tracing_utils import RecordSqlQueries
from sentry_sdk.tracing_utils import record_sql_queries
from sentry_sdk.utils import (
HAS_REAL_CONTEXTVARS,
CONTEXTVARS_ERROR_MESSAGE,
Expand Down Expand Up @@ -538,7 +538,7 @@ def execute(self, sql, params=None):
if hub.get_integration(DjangoIntegration) is None:
return real_execute(self, sql, params)

with RecordSqlQueries(
with record_sql_queries(
hub, self.cursor, sql, params, paramstyle="format", executemany=False
):
return real_execute(self, sql, params)
Expand All @@ -549,7 +549,7 @@ def executemany(self, sql, param_list):
if hub.get_integration(DjangoIntegration) is None:
return real_executemany(self, sql, param_list)

with RecordSqlQueries(
with record_sql_queries(
hub, self.cursor, sql, param_list, paramstyle="format", executemany=True
):
return real_executemany(self, sql, param_list)
Expand Down
27 changes: 15 additions & 12 deletions sentry_sdk/integrations/sqlalchemy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from sentry_sdk._types import MYPY
from sentry_sdk.hub import Hub
from sentry_sdk.integrations import Integration, DidNotEnable
from sentry_sdk.tracing_utils import RecordSqlQueries
from sentry_sdk.tracing_utils import record_sql_queries

try:
from sqlalchemy.engine import Engine # type: ignore
Expand Down Expand Up @@ -50,37 +50,40 @@ def _before_cursor_execute(
if hub.get_integration(SqlalchemyIntegration) is None:
return

ctx_mgr = RecordSqlQueries(
ctx_mgr = record_sql_queries(
hub,
cursor,
statement,
parameters,
paramstyle=context and context.dialect and context.dialect.paramstyle or None,
executemany=executemany,
)
conn._sentry_sql_span_manager = ctx_mgr
context._sentry_sql_span_manager = ctx_mgr

span = ctx_mgr.__enter__()

if span is not None:
conn._sentry_sql_span = span
context._sentry_sql_span = span


def _after_cursor_execute(conn, cursor, statement, *args):
# type: (Any, Any, Any, *Any) -> None
def _after_cursor_execute(conn, cursor, statement, parameters, context, *args):
# type: (Any, Any, Any, Any, Any, *Any) -> None
ctx_mgr = getattr(
conn, "_sentry_sql_span_manager", None
context, "_sentry_sql_span_manager", None
) # type: ContextManager[Any]

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


def _handle_error(context, *args):
# type: (Any, *Any) -> None
conn = context.connection
span = getattr(conn, "_sentry_sql_span", None) # type: Optional[Span]
execution_context = context.execution_context
if execution_context is None:
return

span = getattr(execution_context, "_sentry_sql_span", None) # type: Optional[Span]
Copy link
Member

Choose a reason for hiding this comment

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

Below the execution_context._sentry_sql_span_manager is set back to None but _sentry_sql_span is never set to None. Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

the span is only used for set_status so I guess it doesn't require cleanup like the context manager where lifecycle has to be managed properly.

Copy link
Member

Choose a reason for hiding this comment

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

ok


if span is not None:
span.set_status("internal_error")
Expand All @@ -89,9 +92,9 @@ def _handle_error(context, *args):
# from SQLAlchemy codebase it does seem like any error coming into this
# handler is going to be fatal.
ctx_mgr = getattr(
conn, "_sentry_sql_span_manager", None
execution_context, "_sentry_sql_span_manager", None
) # type: ContextManager[Any]

if ctx_mgr is not None:
conn._sentry_sql_span_manager = None
execution_context._sentry_sql_span_manager = None
ctx_mgr.__exit__(None, None, None)
96 changes: 44 additions & 52 deletions sentry_sdk/tracing_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import re
import contextlib
import json
import math

Expand Down Expand Up @@ -105,58 +106,6 @@ def __iter__(self):
yield k[len(self.prefix) :]


class RecordSqlQueries:
def __init__(
self,
hub, # type: sentry_sdk.Hub
cursor, # type: Any
query, # type: Any
params_list, # type: Any
paramstyle, # type: Optional[str]
executemany, # type: bool
):
# type: (...) -> None
# TODO: Bring back capturing of params by default
self._hub = hub
if self._hub.client and self._hub.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

self._query = _format_sql(cursor, query)

self._data = {}
if params_list is not None:
self._data["db.params"] = params_list
if paramstyle is not None:
self._data["db.paramstyle"] = paramstyle
if executemany:
self._data["db.executemany"] = True

def __enter__(self):
# type: () -> Span
with capture_internal_exceptions():
self._hub.add_breadcrumb(
message=self._query, category="query", data=self._data
)

with self._hub.start_span(op="db", description=self._query) as span:
for k, v in self._data.items():
span.set_data(k, v)
return span

def __exit__(self, exc_type, exc_val, exc_tb):
# type: (Any, Any, Any) -> None
pass


def has_tracing_enabled(options):
# type: (Dict[str, Any]) -> bool
"""
Expand Down Expand Up @@ -201,6 +150,49 @@ def is_valid_sample_rate(rate):
return True


@contextlib.contextmanager
def record_sql_queries(
hub, # type: sentry_sdk.Hub
cursor, # type: Any
query, # type: Any
params_list, # type: Any
paramstyle, # type: Optional[str]
executemany, # type: bool
):
# type: (...) -> Generator[Span, None, None]

# TODO: Bring back capturing of params by default
if hub.client and hub.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

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

with hub.start_span(op="db", description=query) as span:
for k, v in data.items():
span.set_data(k, v)
yield span


def maybe_create_breadcrumbs_from_span(hub, span):
# type: (sentry_sdk.Hub, Span) -> None
if span.op == "redis":
Expand Down