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

Jump to bottom

bug(flask): Transactions missing body #1034

Merged
merged 4 commits into from
Mar 2, 2021
Merged

bug(flask): Transactions missing body #1034

merged 4 commits into from
Mar 2, 2021

Conversation

ahmedetefy
Copy link
Contributor

This PR fixes the bug that transactions were missing data/body because weakref to the request object was garbage collected before the request object made into the event_processor

  • Removed weakref to request object when passed to the event_processor_factory

Ref: https://app.asana.com/0/1142928854421177/1199663160066590

@ahmedetefy ahmedetefy requested a review from untitaker February 26, 2021 10:29
untitaker
untitaker reviewed Feb 26, 2021
View reviewed changes
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

please verify manually that the test fails if the fix is not applied

@ahmedetefy
Copy link
Contributor Author

please verify manually that the test fails if the fix is not applied

Already did .. fails without the code changes

@ahmedetefy
Copy link
Contributor Author

ahmedetefy commented Feb 26, 2021 •
edited
Loading

There is no particular need for the request to be passed on to the event processor factory
https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/flask.py#L117-L119

A discussed solution would be to just create an instance of FlaskRequestExtractor when the request is started but that poses two problems
1- It adds overhead for users that do not have tracing enabled
Possible solution: Have a different code path if you do not have tracing enabled
2- (More serious) Calling request.data eagerly loads to memory before the user does in his code is not ideal

@untitaker is that everything we discussed or would you like to add something?

@untitaker
Copy link
Member

nah sounds good to me

@ahmedetefy ahmedetefy merged commit ed7d722 into master Mar 2, 2021
@ahmedetefy ahmedetefy deleted the bug/flask_transaction_missing_body branch March 2, 2021 08:28
alexmv added a commit to CAVaccineInventory/vial that referenced this pull request Mar 5, 2021
@alexmv
requirements: Upgrade via ./scripts/upgrade-requirements
2fdbb1d 
Interestingly, sentry has just hit 1.0.0!  None of the nominally
breaking changes[1] look relevant to our minimal usage:

> - Feat: Moved auto_session_tracking experimental flag to a proper
> option and removed session_mode, hence enabling release health by
> default getsentry/sentry-python#994
>
> - Fixed Django transaction name by setting the name to
> request.path_info rather than request.path
>
> - Fix for tracing by getting HTTP headers from span rather than
> transaction when possible getsentry/sentry-python#1035
>
> - Fix for Flask transactions missing request body in non errored
> transactions getsentry/sentry-python#1034
>
> - Fix for honoring the X-Forwarded-For header getsentry/sentry-python#1037
>
> - Fix for worker that logs data dropping of events with level error
> getsentry/sentry-python#1032

[1] https://github.com/getsentry/sentry-python/blob/master/CHANGELOG.md
This was referenced Mar 8, 2021
Merged
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Merged
This was referenced Mar 16, 2021
Closed
Merged
Merged
Closed
Merged
@ahmedetefy ahmedetefy mentioned this pull request Mar 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@untitaker untitaker untitaker left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ahmedetefy @untitaker