Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sentry_sdk/integrations/aiohttp.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ async def sentry_app_handle(self, request, *args, **kwargs):

weak_request = weakref.ref(request)

with Hub(Hub.current) as hub:
with Hub(hub) as hub:
# Scope data will not leak between requests because aiohttp
# create a task to wrap each request.
with hub.configure_scope() as scope:
Expand Down
64 changes: 39 additions & 25 deletions sentry_sdk/integrations/tornado.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import weakref
import contextlib
from inspect import iscoroutinefunction

from sentry_sdk.hub import Hub, _should_send_default_pii
from sentry_sdk.tracing import Transaction
from sentry_sdk.utils import (
HAS_REAL_CONTEXTVARS,
CONTEXTVARS_ERROR_MESSAGE,
Expand Down Expand Up @@ -32,6 +34,7 @@
from typing import Optional
from typing import Dict
from typing import Callable
from typing import Generator

from sentry_sdk._types import EventProcessor

Expand Down Expand Up @@ -63,38 +66,16 @@ def setup_once():
# Starting Tornado 6 RequestHandler._execute method is a standard Python coroutine (async/await)
# In that case our method should be a coroutine function too
async def sentry_execute_request_handler(self, *args, **kwargs):
# type: (Any, *Any, **Any) -> Any
hub = Hub.current
integration = hub.get_integration(TornadoIntegration)
if integration is None:
return await old_execute(self, *args, **kwargs)

weak_handler = weakref.ref(self)

with Hub(hub) as hub:
with hub.configure_scope() as scope:
scope.clear_breadcrumbs()
processor = _make_event_processor(weak_handler) # type: ignore
scope.add_event_processor(processor)
# type: (RequestHandler, *Any, **Any) -> Any
with _handle_request_impl(self):
return await old_execute(self, *args, **kwargs)

else:

@coroutine # type: ignore
def sentry_execute_request_handler(self, *args, **kwargs):
# type: (RequestHandler, *Any, **Any) -> Any
hub = Hub.current
integration = hub.get_integration(TornadoIntegration)
if integration is None:
return old_execute(self, *args, **kwargs)

weak_handler = weakref.ref(self)

with Hub(hub) as hub:
with hub.configure_scope() as scope:
scope.clear_breadcrumbs()
processor = _make_event_processor(weak_handler) # type: ignore
scope.add_event_processor(processor)
with _handle_request_impl(self):
result = yield from old_execute(self, *args, **kwargs)
return result

Expand All @@ -110,6 +91,39 @@ def sentry_log_exception(self, ty, value, tb, *args, **kwargs):
RequestHandler.log_exception = sentry_log_exception # type: ignore


@contextlib.contextmanager
def _handle_request_impl(self):
# type: (RequestHandler) -> Generator[None, None, None]
hub = Hub.current
integration = hub.get_integration(TornadoIntegration)

if integration is None:
yield

weak_handler = weakref.ref(self)

with Hub(hub) as hub:
with hub.configure_scope() as scope:
scope.clear_breadcrumbs()
processor = _make_event_processor(weak_handler) # type: ignore
Copy link
Contributor

@ahmedetefy ahmedetefy Mar 19, 2021

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

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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.

scope.add_event_processor(processor)

transaction = Transaction.continue_from_headers(
self.request.headers,
op="http.server",
# Like with all other integrations, this is our
# fallback transaction in case there is no route.
# sentry_urldispatcher_resolve is responsible for
# setting a transaction name later.
name="generic Tornado request",
)

with hub.start_transaction(
transaction, custom_sampling_context={"tornado_request": self.request}
):
yield


def _capture_exception(ty, value, tb):
# type: (type, BaseException, Any) -> None
hub = Hub.current
Expand Down
97 changes: 96 additions & 1 deletion tests/integrations/tornado/test_tornado.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import pytest

from sentry_sdk import configure_scope
from sentry_sdk import configure_scope, start_transaction
from sentry_sdk.integrations.tornado import TornadoIntegration

from tornado.web import RequestHandler, Application, HTTPError
Expand Down Expand Up @@ -40,6 +40,25 @@ def get(self):
scope.set_tag("foo", "42")
1 / 0

def post(self):
with configure_scope() as scope:
scope.set_tag("foo", "43")
1 / 0


class HelloHandler(RequestHandler):
async def get(self):
with configure_scope() as scope:
scope.set_tag("foo", "42")

return b"hello"

async def post(self):
with configure_scope() as scope:
scope.set_tag("foo", "43")

return b"hello"


def test_basic(tornado_testcase, sentry_init, capture_events):
sentry_init(integrations=[TornadoIntegration()], send_default_pii=True)
Expand Down Expand Up @@ -82,6 +101,82 @@ def test_basic(tornado_testcase, sentry_init, capture_events):
assert not scope._tags


@pytest.mark.parametrize(
"handler,code",
[
(CrashingHandler, 500),
(HelloHandler, 200),
],
)
def test_transactions(tornado_testcase, sentry_init, capture_events, handler, code):
sentry_init(integrations=[TornadoIntegration()], traces_sample_rate=1.0, debug=True)
events = capture_events()
client = tornado_testcase(Application([(r"/hi", handler)]))

with start_transaction(name="client") as span:
pass

response = client.fetch(
"/hi", method="POST", body=b"heyoo", headers=dict(span.iter_headers())
)
assert response.code == code

if code == 200:
client_tx, server_tx = events
server_error = None
else:
client_tx, server_error, server_tx = events

assert client_tx["type"] == "transaction"
assert client_tx["transaction"] == "client"

if server_error is not None:
assert server_error["exception"]["values"][0]["type"] == "ZeroDivisionError"
assert (
server_error["transaction"]
== "tests.integrations.tornado.test_tornado.CrashingHandler.post"
)

if code == 200:
assert (
server_tx["transaction"]
== "tests.integrations.tornado.test_tornado.HelloHandler.post"
)
else:
assert (
server_tx["transaction"]
== "tests.integrations.tornado.test_tornado.CrashingHandler.post"
)

assert server_tx["type"] == "transaction"

request = server_tx["request"]
host = request["headers"]["Host"]
assert server_tx["request"] == {
"env": {"REMOTE_ADDR": "127.0.0.1"},
"headers": {
"Accept-Encoding": "gzip",
"Connection": "close",
**request["headers"],
},
"method": "POST",
"query_string": "",
"data": {"heyoo": [""]},
"url": "http://{host}/hi".format(host=host),
}

assert (
client_tx["contexts"]["trace"]["trace_id"]
== server_tx["contexts"]["trace"]["trace_id"]
)

if server_error is not None:
assert (
server_error["contexts"]["trace"]["trace_id"]
== server_tx["contexts"]["trace"]["trace_id"]
)


def test_400_not_logged(tornado_testcase, sentry_init, capture_events):
sentry_init(integrations=[TornadoIntegration()])
events = capture_events()
Expand Down