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

szokeasaurusrex · 2024-08-08T15:30:55Z

Make several changes to prepare for fully removing Hubs in the next major:

Deprecate the Hub-based auto_session_tracking function, replacing it with a new Scope-based function called track_session
Deprecate the scope-based auto_session_tracking_scope in favor of the new track_session function
Change usages of auto_session_tracking_scope to track_sessions. There are no usages of auto_session_tracking outside of tests.
Run all tests that were previously run against auto_session_tracking also against the new track_session. Previously, auto_session_tracking_scope was completely untested.

szokeasaurusrex · 2024-08-08T15:33:48Z

sentry_sdk/sessions.py

        should_track = client_options.get("auto_session_tracking", False)

    return should_track


+@contextmanager
+def track_session(scope, session_mode="application"):


I originally tried to make the auto_session_tracking function use the scope based API by default but still be backwards compatible with hub-based API, but implementing this was going to be extremely overcomplicated.

I decided it would be much simpler to just implement a new function to replace auto_session_tracking instead, hence why I have proposed fully deprecating auto_session_tracking and auto_session_tracking_scope and replacing both with the new track_session.

codecov · 2024-08-08T16:19:50Z

Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time.

❌ Failed Test Results:

Completed 12419 tests with 1 failed, 10789 passed and 1629 skipped.

View the full list of failed tests

py3.7-falcon-v1.4

Class name: pytest
Test name: internal
```
No failure message available
```

antonpirker · 2024-08-09T11:19:39Z

sentry_sdk/sessions.py

-    # TODO: add deprecation warning
-
-    if hub is None:
-        hub = sentry_sdk.Hub.current


Imo to deprecate the hub based session tracking we could add a deprecation warning here if hub is not None and explain that in the next major they can not use the hub as a parameter in the next major, but can pass a scope parameter.

When we do the next major we do what is written in the comment in sessions.py:

TODO: This (is_auto_session_tracking_enabled_scope) uses the new scopes. When the Hub is removed, the function is_auto_session_tracking_enabled should be removed and this function should be renamed to is_auto_session_tracking_enabled.

Discussed offline. The is_auto_session_tracking_enabled deprecation is now in a separate PR (#3428). After clarifying the reasoning for introducing a new function was that it would allow us in our own code to avoid using deprecated APIs, @antonpirker agreed that introducing track_sessions was a good idea

Make several changes to prepare for fully removing Hubs in the next major: - Deprecate the Hub-based `auto_session_tracking` function, replacing it with a new Scope-based function called `track_session` - Deprecate the scope-based `auto_session_tracking_scope` in favor of the new `track_session` function - Change usages of `auto_session_tracking_scope` to `track_sessions`. There are no usages of `auto_session_tracking` outside of tests. - Run all tests that were previously run against `auto_session_tracking` also against the new `track_session`. Previously, `auto_session_tracking_scope` was completely untested. Fixes #3417

antonpirker

Looks good!

I left a few comments, please address them before merging.

antonpirker · 2024-08-09T13:08:08Z

tests/test_sessions.py

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


Suggested change

sentry_sdk.flush()

sentry_sdk.flush()

# If we reach this point without error, the test is successful.

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

Accidentally merged before fixed, opened #3430 with this change

antonpirker · 2024-08-09T13:10:41Z

tests/test_sessions.py

+        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()


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.

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.

Implement suggestion from #3419 (comment).

Make several changes to prepare for fully removing Hubs in the next major: - Deprecate the Hub-based `auto_session_tracking` function, replacing it with a new Scope-based function called `track_session` - Deprecate the scope-based `auto_session_tracking_scope` in favor of the new `track_session` function - Change usages of `auto_session_tracking_scope` to `track_sessions`. There are no usages of `auto_session_tracking` outside of tests. - Run all tests that were previously run against `auto_session_tracking` also against the new `track_session`. Previously, `auto_session_tracking_scope` was completely untested. Fixes #3417

Implement suggestion from #3419 (comment). Co-authored-by: Anton Pirker <anton.pirker@sentry.io>

Make several changes to prepare for fully removing Hubs in the next major: - Deprecate the Hub-based `auto_session_tracking` function, replacing it with a new Scope-based function called `track_session` - Deprecate the scope-based `auto_session_tracking_scope` in favor of the new `track_session` function - Change usages of `auto_session_tracking_scope` to `track_sessions`. There are no usages of `auto_session_tracking` outside of tests. - Run all tests that were previously run against `auto_session_tracking` also against the new `track_session`. Previously, `auto_session_tracking_scope` was completely untested. Fixes getsentry#3417

Implement suggestion from getsentry#3419 (comment). Co-authored-by: Anton Pirker <anton.pirker@sentry.io>

szokeasaurusrex requested review from antonpirker and sentrivana August 8, 2024 15:30

szokeasaurusrex commented Aug 8, 2024

View reviewed changes

szokeasaurusrex force-pushed the szokeasaurusrex/session-tracking-hub-deprecation branch 2 times, most recently from f946729 to c0e5fad Compare August 8, 2024 15:41

szokeasaurusrex mentioned this pull request Aug 9, 2024

feat(sessions): Remove deprecated functions #3424

Closed

antonpirker reviewed Aug 9, 2024

View reviewed changes

szokeasaurusrex marked this pull request as draft August 9, 2024 11:48

szokeasaurusrex force-pushed the szokeasaurusrex/session-tracking-hub-deprecation branch from c0e5fad to 6111f9c Compare August 9, 2024 12:50

szokeasaurusrex requested a review from antonpirker August 9, 2024 12:50

szokeasaurusrex marked this pull request as ready for review August 9, 2024 12:50

antonpirker approved these changes Aug 9, 2024

View reviewed changes

szokeasaurusrex merged commit 275c63e into master Aug 9, 2024
123 of 124 checks passed

szokeasaurusrex deleted the szokeasaurusrex/session-tracking-hub-deprecation branch August 9, 2024 14:18

szokeasaurusrex mentioned this pull request Aug 9, 2024

Document test_sessions #3429

Open

szokeasaurusrex added a commit that referenced this pull request Aug 9, 2024

test(sessions): Add comments to explain test

e9adc9d

Implement suggestion from #3419 (comment).

szokeasaurusrex mentioned this pull request Aug 9, 2024

test(sessions): Add comments to explain test #3430

Merged

antonpirker added a commit that referenced this pull request Aug 28, 2024

test(sessions): Add comments to explain test (#3430)

2e991c7

Implement suggestion from #3419 (comment). Co-authored-by: Anton Pirker <anton.pirker@sentry.io>

arjennienhuis pushed a commit to arjennienhuis/sentry-python that referenced this pull request Sep 30, 2024

test(sessions): Add comments to explain test (getsentry#3430)

da2d51c

Implement suggestion from getsentry#3419 (comment). Co-authored-by: Anton Pirker <anton.pirker@sentry.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

szokeasaurusrex commented Aug 8, 2024 •

edited

Loading

szokeasaurusrex Aug 8, 2024

codecov bot commented Aug 8, 2024 •

edited

Loading

py3.7-falcon-v1.4

antonpirker Aug 9, 2024

szokeasaurusrex Aug 9, 2024

antonpirker left a comment

antonpirker Aug 9, 2024

antonpirker Aug 9, 2024

szokeasaurusrex Aug 9, 2024

antonpirker Aug 9, 2024

szokeasaurusrex Aug 9, 2024

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

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

Conversation

szokeasaurusrex commented Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2024 • edited Loading

❌ Failed Test Results:

py3.7-falcon-v1.4

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonpirker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

szokeasaurusrex commented Aug 8, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading