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

sl0thentr0py · 2022-04-08T12:51:35Z

Revert (fix) Change context manager type to avoid race in threads #1368 since it caused a regression for measuring db span times (Django database transaction duration has 0 duration #1385)
Use context instead of connection in sqlalchemy integration
- The connection is reused across threads while the context is specific to the query in question.
- Also fix handle_error to use the correct execution_context where we attach our span context manager.

… threads (#1368)" This reverts commit de0bc50. This caused a regression (#1385) since the span finishes immediately in __enter__ and so all db spans have wrong time durations.

antonpirker · 2022-04-08T13:33:18Z

sentry_sdk/integrations/sqlalchemy.py

+    if execution_context is None:
+        return
+
+    span = getattr(execution_context, "_sentry_sql_span", None)  # type: Optional[Span]


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?

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.

antonpirker · 2022-04-11T06:56:46Z

sentry_sdk/integrations/sqlalchemy.py

+    if execution_context is None:
+        return
+
+    span = getattr(execution_context, "_sentry_sql_span", None)  # type: Optional[Span]


sl0thentr0py added 2 commits April 8, 2022 13:13

Revert "fix(sqlalchemy): Change context manager type to avoid race in…

27abc12

… threads (#1368)" This reverts commit de0bc50. This caused a regression (#1385) since the span finishes immediately in __enter__ and so all db spans have wrong time durations.

Use context instead of conn in sqlalchemy hooks

951e293

sl0thentr0py requested a review from antonpirker April 8, 2022 12:51

mypy

055aeb2

sl0thentr0py mentioned this pull request Apr 8, 2022

(fix) Change context manager type to avoid race in threads #1368

Merged

antonpirker reviewed Apr 8, 2022

View reviewed changes

antonpirker approved these changes Apr 11, 2022

View reviewed changes

antonpirker merged commit 9a0c133 into master Apr 11, 2022

antonpirker deleted the neel/WEBBACKEND-138-sql-query-regression branch April 11, 2022 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sl0thentr0py commented Apr 8, 2022

antonpirker Apr 8, 2022

sl0thentr0py Apr 8, 2022

antonpirker Apr 11, 2022

antonpirker Apr 11, 2022

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

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

Conversation

sl0thentr0py commented Apr 8, 2022

antonpirker Apr 8, 2022

Choose a reason for hiding this comment

sl0thentr0py Apr 8, 2022

Choose a reason for hiding this comment

antonpirker Apr 11, 2022

Choose a reason for hiding this comment

antonpirker Apr 11, 2022

Choose a reason for hiding this comment