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.

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

Close segment in only _handle_exception in case of Internal Server Error #271

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

srprash
Copy link
Contributor

@srprash srprash commented Feb 1, 2021

Issue #, if available: #269

Description of changes:

  • Return from _after_request when response code >= 500 so that the exception in recorded on the segment in the _handle_exception method.
  • Not enabling the testing mode so that the request is handled by the application in usual way during the exception

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

@codecov-io
Copy link

codecov-io commented Feb 1, 2021

Codecov Report

Merging #271 (1f04fd4) into master (5af2357) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
- Coverage   79.46%   79.38%   -0.08%     
==========================================
  Files          82       82              
  Lines        3253     3255       +2     
==========================================
- Hits         2585     2584       -1     
- Misses        668      671       +3     
Impacted Files Coverage Δ
aws_xray_sdk/ext/flask/middleware.py 88.75% <100.00%> (-3.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5af2357...1f04fd4. Read the comment docs.

@srprash srprash requested a review from willarmiros February 1, 2021 17:12
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 606631d into aws:master Feb 1, 2021
@ianmetcalf
Copy link

@srprash I think this introduces a subtle bug. A 500 response in after_request doesn't always result in an exception being passed to teardown_request. For instance the following would cause the segment not to be closed at all.

app.route('/error')
def error():
    return 'Server Error', 500

@srprash
Copy link
Contributor Author

srprash commented Feb 3, 2021

@ianmetcalf Ah! I see. I was kind of unaware of tuple as a flask application response. I just found out about it here.
Good catch! I guess we should always close the segment in teardown_request if one is still present. I'll open a PR for the fix shortly.

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.

4 participants