Skip to content

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

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

Jump to bottom
Merged
antonpirker merged 3 commits into from
Apr 11, 2022
Merged

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

antonpirker merged 3 commits into from
Apr 11, 2022

Conversation

@sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Apr 8, 2022

@sl0thentr0py
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.
@sl0thentr0py
Use context instead of conn in sqlalchemy hooks
951e293
@sl0thentr0py sl0thentr0py requested a review from antonpirker April 8, 2022 12:51
@sl0thentr0py sl0thentr0py mentioned this pull request Apr 8, 2022
Merged
antonpirker
antonpirker reviewed Apr 8, 2022
View reviewed changes
sentry_sdk/integrations/sqlalchemy.py
if execution_context is None:
return

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

@antonpirker antonpirker Apr 8, 2022

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

@sl0thentr0py sl0thentr0py Apr 8, 2022

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
Contributor

@antonpirker antonpirker Apr 11, 2022

Choose a reason for hiding this comment

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

ok

antonpirker
antonpirker approved these changes Apr 11, 2022
View reviewed changes
sentry_sdk/integrations/sqlalchemy.py
if execution_context is None:
return

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

@antonpirker antonpirker Apr 11, 2022

Choose a reason for hiding this comment

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

ok

@antonpirker antonpirker merged commit 9a0c133 into master Apr 11, 2022
@antonpirker antonpirker deleted the neel/WEBBACKEND-138-sql-query-regression branch April 11, 2022 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@antonpirker antonpirker antonpirker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sl0thentr0py @antonpirker