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

ref(sessions): Deprecate hub-based sessions.py logic #3419

Merged
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
4 changes: 2 additions & 2 deletions sentry_sdk/integrations/aiohttp.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from sentry_sdk.consts import OP, SPANSTATUS, SPANDATA
from sentry_sdk.integrations import Integration, DidNotEnable
from sentry_sdk.integrations.logging import ignore_logger
from sentry_sdk.sessions import auto_session_tracking_scope
from sentry_sdk.sessions import track_session
from sentry_sdk.integrations._wsgi_common import (
_filter_headers,
request_body_within_bounds,
Expand Down Expand Up @@ -105,7 +105,7 @@ async def sentry_app_handle(self, request, *args, **kwargs):
weak_request = weakref.ref(request)

with sentry_sdk.isolation_scope() as scope:
with auto_session_tracking_scope(scope, session_mode="request"):
with track_session(scope, session_mode="request"):
# Scope data will not leak between requests because aiohttp
# create a task to wrap each request.
scope.generate_propagation_context()
Expand Down
4 changes: 2 additions & 2 deletions sentry_sdk/integrations/asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
_get_request_data,
_get_url,
)
from sentry_sdk.sessions import auto_session_tracking_scope
from sentry_sdk.sessions import track_session
from sentry_sdk.tracing import (
SOURCE_FOR_STYLE,
TRANSACTION_SOURCE_ROUTE,
Expand Down Expand Up @@ -169,7 +169,7 @@ async def _run_app(self, scope, receive, send, asgi_version):
_asgi_middleware_applied.set(True)
try:
with sentry_sdk.isolation_scope() as sentry_scope:
with auto_session_tracking_scope(sentry_scope, session_mode="request"):
with track_session(sentry_scope, session_mode="request"):
sentry_scope.clear_breadcrumbs()
sentry_scope._name = "asgi"
processor = partial(self.event_processor, asgi_scope=scope)
Expand Down
6 changes: 2 additions & 4 deletions sentry_sdk/integrations/wsgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@
from sentry_sdk.consts import OP
from sentry_sdk.scope import should_send_default_pii
from sentry_sdk.integrations._wsgi_common import _filter_headers
from sentry_sdk.sessions import (
auto_session_tracking_scope as auto_session_tracking,
) # When the Hub is removed, this should be renamed (see comment in sentry_sdk/sessions.py)
from sentry_sdk.sessions import track_session
from sentry_sdk.scope import use_isolation_scope
from sentry_sdk.tracing import Transaction, TRANSACTION_SOURCE_ROUTE
from sentry_sdk.utils import (
Expand Down Expand Up @@ -83,7 +81,7 @@ def __call__(self, environ, start_response):
_wsgi_middleware_applied.set(True)
try:
with sentry_sdk.isolation_scope() as scope:
with auto_session_tracking(scope, session_mode="request"):
with track_session(scope, session_mode="request"):
with capture_internal_exceptions():
scope.clear_breadcrumbs()
scope._name = "wsgi"
Expand Down
34 changes: 28 additions & 6 deletions sentry_sdk/sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,15 @@ def is_auto_session_tracking_enabled(hub=None):
@contextmanager
def auto_session_tracking(hub=None, session_mode="application"):
# type: (Optional[sentry_sdk.Hub], str) -> Generator[None, None, None]
"""Starts and stops a session automatically around a block."""
# TODO: add deprecation warning
"""DEPRECATED: Use track_session instead
Starts and stops a session automatically around a block.
"""
warnings.warn(
"This function is deprecated and will be removed in the next major release. "
"Use track_session instead.",
DeprecationWarning,
stacklevel=2,
)

if hub is None:
hub = sentry_sdk.Hub.current
Expand Down Expand Up @@ -98,13 +105,28 @@ def _is_auto_session_tracking_enabled(scope):
@contextmanager
def auto_session_tracking_scope(scope, session_mode="application"):
# type: (sentry_sdk.Scope, str) -> Generator[None, None, None]
"""
"""DEPRECATED: This function is a deprecated alias for track_session.
Starts and stops a session automatically around a block.
"""

TODO: This uses the new scopes. When the Hub is removed, the function
auto_session_tracking should be removed and this function
should be renamed to auto_session_tracking.
warnings.warn(
"This function is a deprecated alias for track_session and will be removed in the next major release.",
DeprecationWarning,
stacklevel=2,
)

with track_session(scope, session_mode=session_mode):
yield


@contextmanager
def track_session(scope, session_mode="application"):
# type: (sentry_sdk.Scope, str) -> Generator[None, None, None]
"""
Start a new session in the provided scope, assuming session tracking is enabled.
This is a no-op context manager if session tracking is not enabled.
"""

should_track = _is_auto_session_tracking_enabled(scope)
if should_track:
scope.start_session(session_mode=session_mode)
Expand Down
106 changes: 105 additions & 1 deletion tests/test_sessions.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from unittest import mock

import sentry_sdk
from sentry_sdk.sessions import auto_session_tracking
from sentry_sdk.sessions import auto_session_tracking, track_session


def sorted_aggregates(item):
Expand Down Expand Up @@ -50,6 +50,48 @@ def test_aggregates(sentry_init, capture_envelopes):
)
envelopes = capture_envelopes()

with sentry_sdk.isolation_scope() as scope:
with track_session(scope, session_mode="request"):
try:
scope.set_user({"id": "42"})
raise Exception("all is wrong")
except Exception:
sentry_sdk.capture_exception()

with sentry_sdk.isolation_scope() as scope:
with track_session(scope, session_mode="request"):
pass

sentry_sdk.get_isolation_scope().start_session(session_mode="request")
sentry_sdk.get_isolation_scope().end_session()
sentry_sdk.flush()
Comment on lines +54 to +67
Copy link
Member

Choose a reason for hiding this comment

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

Tests are always also a kind of documentation on how to use the code.

Could you add comments to the three sessions to mark what aggregate status is expected for each one of them?

Please add those comments to all similar tests in this file. This will help us in the future (or others) to understand what is going on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied these tests from the existing tests, and modified them to call the new API. To be completely honest, I don't really understand these tests or know what aggregate status is expected from them.

Since all I am doing is modifying tests to call the new API (and keeping around a copy to still test the old deprecated API), I think it is out of scope to add this documentation here. But, I think this is a good idea, so I will open a new issue.


assert len(envelopes) == 2
assert envelopes[0].get_event() is not None

sess = envelopes[1]
assert len(sess.items) == 1
sess_event = sess.items[0].payload.json
assert sess_event["attrs"] == {
"release": "fun-release",
"environment": "not-fun-env",
}

aggregates = sorted_aggregates(sess_event)
assert len(aggregates) == 1
assert aggregates[0]["exited"] == 2
assert aggregates[0]["errored"] == 1


def test_aggregates_deprecated(
sentry_init, capture_envelopes, suppress_deprecation_warnings
):
sentry_init(
release="fun-release",
environment="not-fun-env",
)
envelopes = capture_envelopes()

with auto_session_tracking(session_mode="request"):
with sentry_sdk.new_scope() as scope:
try:
Expand Down Expand Up @@ -90,6 +132,39 @@ def test_aggregates_explicitly_disabled_session_tracking_request_mode(
)
envelopes = capture_envelopes()

with sentry_sdk.isolation_scope() as scope:
with track_session(scope, session_mode="request"):
try:
raise Exception("all is wrong")
except Exception:
sentry_sdk.capture_exception()

with sentry_sdk.isolation_scope() as scope:
with track_session(scope, session_mode="request"):
pass

sentry_sdk.get_isolation_scope().start_session(session_mode="request")
sentry_sdk.get_isolation_scope().end_session()
sentry_sdk.flush()

sess = envelopes[1]
assert len(sess.items) == 1
sess_event = sess.items[0].payload.json

aggregates = sorted_aggregates(sess_event)
assert len(aggregates) == 1
assert aggregates[0]["exited"] == 1
assert "errored" not in aggregates[0]


def test_aggregates_explicitly_disabled_session_tracking_request_mode_deprecated(
sentry_init, capture_envelopes, suppress_deprecation_warnings
):
sentry_init(
release="fun-release", environment="not-fun-env", auto_session_tracking=False
)
envelopes = capture_envelopes()

with auto_session_tracking(session_mode="request"):
with sentry_sdk.new_scope():
try:
Expand Down Expand Up @@ -120,6 +195,35 @@ def test_no_thread_on_shutdown_no_errors(sentry_init):
environment="not-fun-env",
)

# make it seem like the interpreter is shutting down
with mock.patch(
"threading.Thread.start",
side_effect=RuntimeError("can't create new thread at interpreter shutdown"),
):
with sentry_sdk.isolation_scope() as scope:
with track_session(scope, session_mode="request"):
try:
raise Exception("all is wrong")
except Exception:
sentry_sdk.capture_exception()

with sentry_sdk.isolation_scope() as scope:
with track_session(scope, session_mode="request"):
pass

sentry_sdk.get_isolation_scope().start_session(session_mode="request")
sentry_sdk.get_isolation_scope().end_session()
sentry_sdk.flush()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sentry_sdk.flush()
sentry_sdk.flush()
# If we reach this point without error, the test is successful.

Copy link
Member

Choose a reason for hiding this comment

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

Can you also add this comment to the next test? (can not suggest this change from the github ui)

Copy link
Member Author

Choose a reason for hiding this comment

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

Accidentally merged before fixed, opened #3430 with this change



def test_no_thread_on_shutdown_no_errors_deprecated(
sentry_init, suppress_deprecation_warnings
):
sentry_init(
release="fun-release",
environment="not-fun-env",
)

# make it seem like the interpreter is shutting down
with mock.patch(
"threading.Thread.start",
Expand Down
Loading