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
feat: Support tracing on Tornado #1060
Conversation
with Hub(hub) as hub: | ||
with hub.configure_scope() as scope: | ||
scope.clear_breadcrumbs() | ||
processor = _make_event_processor(weak_handler) # type: ignore |
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.
Does this not have the same issue as #1034 , the weak_handler
is not accessible within the tornado_processor
inner func https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/tornado.py#L135
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.
Good catch, but I believe self
will stick around for the duration of the request handler invocation, which sticks around for the duration of the response streaming, so it should be fine afaict
let me add a test
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.
I don't think the test you added covers that case since that request raises an exception and so triggers the error handler.
I would create another test to check if the request body exists when the request doesn't raise an exception and returns a success status code.
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.
Conceptually the exception won't make the request stay around for longer, but you're right it's not the same testcase so I added another one.
7ec583d
to
7f65e71
Compare
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.
LGTM. Can you please also add a tornado version of this?
Yup will do, thanks. |
Fix #1057