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

feat(release-health): Enable session tracking by default #994

Merged
merged 20 commits into from
Feb 22, 2021

Conversation

ahmedetefy
Copy link
Contributor

@ahmedetefy ahmedetefy commented Feb 5, 2021

This PR adds session tracking by default, through removing auto_session_tracking from experimental options to proper ones with session_mode defaulting to application but then switches to request in the middleware

Changes:

  • Moved session_mode from SessionFlusher (sessions.py) to Session (session.py)
  • Hub.start_session takes a session_mode that defaults to application
  • auto_session_tracking context manager takes an optional session_mode
  • Moved auto_session_tracking experimental flag to a proper option that defaults to True
  • Removed session_mode experimental option

sentry_sdk/consts.py Show resolved Hide resolved
sentry_sdk/sessions.py Outdated Show resolved Hide resolved
sentry_sdk/consts.py Outdated Show resolved Hide resolved
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I do have one concern, since we default to application mode, which means no pre-aggregation, currently the auto_session_tracking resource manager only checks for the auto_session_tracking flag, but not for the mode.

Why is this relevant: The wsgi integration does auto_session_tracking, so if someone has not correctly configured the SDK, that integration will generate tons of individual session updates.

@Swatinem
Copy link
Member

Swatinem commented Feb 8, 2021

second concern is around application mode sessions: currently there is no code that starts/stops one. what I think is missing is actually calling start_session in the sdk init.

@mitsuhiko
Copy link
Member

Do we need this flag? I thought we might be able to auto detect within the integration. Is that not the case?

@ahmedetefy
Copy link
Contributor Author

Do we need this flag? I thought we might be able to auto-detect within the integration. Is that not the case?

I am assuming you are talking about the auto_session_tracking flag, right?

And in that case, I am not entirely sure but it seems like it relies on the options passed on init?
https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/sessions.py#L21-L47

and in regards to the session_mode that @Swatinem is referring to, afaik it is auto-detected in Rust but doesn't seem like it is in Python
https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/client.py#L108

Am I missing something here?

@ahmedetefy ahmedetefy changed the title feat: Enabled auto session tracking by default + Turned auto_session_tracking and session_mode into proper options feat(release-health): Enable session tracking by default Feb 22, 2021
@ahmedetefy ahmedetefy requested a review from Swatinem February 22, 2021 12:11
Copy link
Member

@mitsuhiko mitsuhiko left a comment

Choose a reason for hiding this comment

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

This looks good.

@ahmedetefy ahmedetefy merged commit 1279eec into master Feb 22, 2021
@ahmedetefy ahmedetefy deleted the auto-enable-session-tracking branch February 22, 2021 14:40
alexmv added a commit to CAVaccineInventory/vial that referenced this pull request Mar 5, 2021
Interestingly, sentry has just hit 1.0.0!  None of the nominally
breaking changes[1] look relevant to our minimal usage:

> - Feat: Moved auto_session_tracking experimental flag to a proper
> option and removed session_mode, hence enabling release health by
> default getsentry/sentry-python#994
>
> - Fixed Django transaction name by setting the name to
> request.path_info rather than request.path
>
> - Fix for tracing by getting HTTP headers from span rather than
> transaction when possible getsentry/sentry-python#1035
>
> - Fix for Flask transactions missing request body in non errored
> transactions getsentry/sentry-python#1034
>
> - Fix for honoring the X-Forwarded-For header getsentry/sentry-python#1037
>
> - Fix for worker that logs data dropping of events with level error
> getsentry/sentry-python#1032

[1] https://github.com/getsentry/sentry-python/blob/master/CHANGELOG.md
@levrik
Copy link

levrik commented May 5, 2021

Sadly I couldn't find any documentation about what this auto session tracking actually is so I'm a bit afraid of upgrading to the latest Sentry SDK. What exactly is this feature?

@rhcarvalho
Copy link
Contributor

What exactly is this feature?

@levrik this feature tracks Release Health by sending session updates with healthy/unhealthy counts.

@anderoonies
Copy link

anderoonies commented Mar 12, 2022

Hello!

We recently made a big upgrade from <1.0 to 1.5.7 and I believe we ran into an issue related to this—from APM, it looks like sessions were being updated frequently, which was causing some performance issues downstream, I believe due to user_agent reads on cached sessions (https://docs.djangoproject.com/en/2.2/topics/http/sessions/#using-cached-sessions).

You mentioned:

Why is this relevant: The wsgi integration does auto_session_tracking, so if someone has not correctly configured the SDK, that integration will generate tons of individual session updates.

Can you clarify the misconfiguration here? Is it an incorrect session_mode?
We've tried to disable auto_session_tracking, but we're still seeing a high # of cache gets and the sentry_patched_wsgi_handler is a big % of wall time

Thank you!

UPDATE:
Sorry for any alarm, it looks like session tracking wasn't causing our issue. Looks like a bad interaction with Cachalot, I'll see if I can isolate what's wrong but I don't believe it's because of this. Again, sorry for alarm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants