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

Create 0110-client-user-session.md #110

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

smeubank
Copy link
Member

@smeubank smeubank commented Aug 15, 2023

Feature RFC to define and decide on a user session concept for Sentry SDKs. This is focused on client side SDKs, and aims to solve the ability for SDKs to uniquely link all events associated with a unique user sessions. Which would allow to create a link between all events generated by the SDK during the session, to group them in the Sentry UI, and allow the ID to be considered during client and server side sampling to ensure the completeness of user session in sentry.

Rendered RFC

@smeubank smeubank changed the title Create 0109-client-user-session.md Create 0110-client-user-session.md Aug 15, 2023
Comment on lines +36 to +44
> save us a lot of time if we were able to identify evens from that user
>

Would the default `userId`  work in this case?

> If you don't provide an `id` the SDK falls back to `installationId`, which the SDK randomly generates once during an app's installation.
>

https://docs.sentry.io/platforms/android/enriching-events/identify-user/
Copy link
Member

@kahest kahest Aug 16, 2023

Choose a reason for hiding this comment

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

as mentioned below in the cons, installationId persists until the app is re-installed. this means it stays the same for all sessions of the user throughout one installation, which of course can be thousands across multiple weeks/months/years, and would therefore likely also contain multiple app starts

Copy link
Member

Choose a reason for hiding this comment

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

Another con: The default userId can be overwritten once Sentry.setUser() is called, thus loosing the connection before/after a user logged into an app.
I see we have Context->Device->Id on Android, but I'm not sure if this is provided by other platforms.

2. Longer term solution:
1. react-native SDK
1. SDK provides a way for users to generate a sessionID
1. could simple be a version of the initializationID
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
1. could simple be a version of the initializationID
1. could simply be a version of the `installationId`

5. existing APIs around session may lead to confusion
6. client side sampling: SDK should ensure that if an event as part of a session included then all other events within
that session are included as well
1. identifying a unique session default experience, identifying by a particular user requires the user to decide with
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand this - is this referring to the default experience when looking up sessions on the product?


2. Longer term solution:
1. react-native SDK
1. SDK provides a way for users to generate a sessionID
Copy link
Member

Choose a reason for hiding this comment

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

SDK should auto-generate sessions in a meaningful way to works for most apps (e.g. generate unique id on setUser and app start) and ideally provide an API to override this.

Comment on lines +63 to +65
4. consider a sampling option,
1. similar to SR that states if there is an error to have a higher sample rate for those user sessions
2. potentially long txn times as a decider in client for deciding
Copy link
Member

Choose a reason for hiding this comment

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

to clarify: does that mean that as soon as there is an error or a long txn (tbd how to determine this), the SDK starts to sample everything for that session? which also means that previous events are not available

Copy link
Member

Choose a reason for hiding this comment

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

+1 on this question
the problem is that error often happens later, and we have already made sampling decisions for the given trace/session on the server at that point

@marandaneto
Copy link
Contributor

Can we actually somehow reuse the release health feature for that? each session already has an ID, that ID should be set in the event, and maybe propagate this ID as well similarly to connected errors and distributed tracing.
Would that be a viable option?

1. SDK provides a way for users to generate a sessionID
1. could simple be a version of the initializationID
2. could be experimental flag for now
3. automatically propagated this in DSC
Copy link
Member

Choose a reason for hiding this comment

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

Dynamic sampling could use this to ensure that either all transactions in this session or no transactions in this session are sampled. Nowadays we do the same thing but based on trace id.

Comment on lines +63 to +65
4. consider a sampling option,
1. similar to SR that states if there is an error to have a higher sample rate for those user sessions
2. potentially long txn times as a decider in client for deciding
Copy link
Member

Choose a reason for hiding this comment

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

+1 on this question
the problem is that error often happens later, and we have already made sampling decisions for the given trace/session on the server at that point

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.

None yet

5 participants