Skip to content

BugFix: Always close segment in teardown_request handler #272

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.

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

Merged
merged 2 commits into from
Feb 8, 2021

Conversation

srprash
Copy link
Contributor

@srprash srprash commented Feb 3, 2021

Issue #, if available: #269

Description of changes:
The previous PR which intended to fix the #269 introduced a bug where if the flask application does not pass an exception in case of response code >= 500, the segment will remain unclosed. This may be an edge case scenario that application faults do not pass exception to teardown_request handler but it is still a valid case of responses.
Thus, irrespective of exception being present or not, the middleware should always end the segment if its present.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@srprash srprash requested a review from willarmiros February 3, 2021 23:01
@srprash
Copy link
Contributor Author

srprash commented Feb 3, 2021

@ianmetcalf Feel free to take a look at this.

@ianmetcalf
Copy link

@srprash This should take care of it.

With this change in place could you skip closing the segment in after_request for all requests. Having teardown_request close all segments would make things easier to reason about.

It might also be a good idea to rename the _handle_exception method since it's not only for exception handling now.

@srprash
Copy link
Contributor Author

srprash commented Feb 4, 2021

@ianmetcalf That's a fair call out since the teardown_request is bound to be called at the end of each request, we can avoid some redundant code for closing the segment. And +1 for renaming the method.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

LGTM

@srprash srprash merged commit 508f929 into aws:master Feb 8, 2021
@srprash srprash deleted the flask_bug_fix branch February 8, 2021 18:45
Hargrav3s pushed a commit to Gavant/aws-xray-sdk-python that referenced this pull request Mar 22, 2022
* always closing segment if present in teardown_request handler

* close segment in teardown_request only. rename teardown method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants