-
Notifications
You must be signed in to change notification settings - Fork 614
test(flask): Add span streaming test coverage #6264
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
Open
ericapisani
wants to merge
10
commits into
master
Choose a base branch
from
py-2323-migrate-flask
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+166
−51
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c74acb1
feat(flask): Add span streaming support and request body capture
ericapisani c557b26
move common function between flask and starlette to more central place
ericapisani 2df2cd3
add comment back in that was accidentally removed, small cleanups
ericapisani c90dc55
Read the request body before sending the response to the user instead…
ericapisani 984a63a
Merge branch 'master' into py-2323-migrate-flask
ericapisani b7c0ec6
Improve handling of situation where a request body is never read in t…
ericapisani 3b71c16
fix for unbounded consumption of request wtihout a content-header spe…
ericapisani 234e667
Merge branch 'master' into py-2323-migrate-flask
ericapisani 8b28bef
Revert changes as they were centred around adding the request body, w…
ericapisani 855a9bb
remove assertion as its failure is due to testing conditions - in pro…
ericapisani File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertion
http.response.status_code == 500will always fail when Flask TESTING propagates exceptionsWith
app.config["TESTING"] = True, Flask re-raises unhandled exceptions instead of returning a 500 response, so_sentry_start_responseis never called andhttp.response.status_codeis never set on the span; accessingsegment["attributes"]["http.response.status_code"]will raiseKeyError.Verification
Traced the full execution path: (1)
app.config["TESTING"] = Trueis set in theappfixture (test_flask.py:40). (2) With TESTING=True, Flask'shandle_exceptionre-raises unhandled exceptions beforereturn response(environ, start_response)is reached in Flask'swsgi_app. (3) The test wraps the request inwith pytest.raises(ZeroDivisionError):(line 864), confirming no HTTP response is returned. (4) InSentryWsgiMiddleware.__call__(wsgi.py),_sentry_start_responseis passed as thestart_responsecallback, but it is only called by Flask when a response is actually being sent. Because Flask propagates the exception without callingstart_response,_sentry_start_responseis never invoked and thespan.set_attribute("http.response.status_code", status_int)line in_sentry_start_responseis never reached. (5) Theexcept BaseException: reraise(...)block and thefinallyblock in wsgi.py do not sethttp.response.status_codeeither. (6)StreamedSpan.__exit__only setsself.status = SpanStatus.ERROR.value, not anyhttp.response.status_codeattribute. (7)conftest.py:capture_itemsconstructssegment["attributes"]from the span's attribute dict; a missing key causesKeyErrorwhen accessed. The assertionsegment["status"] == SpanStatus.ERROR(line 882) would pass, but thehttp.response.status_codeassertion will fail.Suggested fix: Remove the assertion for
http.response.status_codein the error-propagation case, since no response is sent when Flask testing mode propagates the exception.Identified by Warden find-bugs · S66-Z95
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warden is correct on this one, and it's why the test is failing. I'm going to remove the assertion because, based on my understanding of this behaviour, Flask's error handlers are never called due to this snippet of code, and this only happens when Flask is set to "testing" or "debug" mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in 855a9bb