Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

Guidelines for sessions in SDKs - #323

Merged
HazAT merged 3 commits into
masterfrom
rhcarvalho/sessions-sdk-guidelines
May 17, 2021
Merged

Guidelines for sessions in SDKs#323
HazAT merged 3 commits into
masterfrom
rhcarvalho/sessions-sdk-guidelines

Conversation

@rhcarvalho

@rhcarvalho rhcarvalho commented Apr 30, 2021

Copy link
Copy Markdown
Contributor

@vercel

vercel Bot commented Apr 30, 2021

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sentry/develop/4e1Rx1Ehamnj6cPUb3jkYMb1Xa1y
✅ Preview: https://develop-git-rhcarvalho-sessions-sdk-guidelines.sentry.dev

Comment thread src/docs/sdk/sessions.mdx
The Hub.endSession() method is explicitly called; or
The program terminates without errors; or
The program terminates with an unhandled exception; or
The program terminates with an unhandled promise rejection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is Node specific.... unhandled promise rejections do not exist in Python for example...

Comment thread src/docs/sdk/sessions.mdx

#### Lifetime of a Session

Sessions are never tracked nor sent individually, instead they are aggregated and the aggregates are sent every 30s and a final time when the web server is terminating.

@ahmedetefy ahmedetefy May 3, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

every 60 seconds*

Comment thread src/docs/sdk/sessions.mdx
#### Lifetime of a Session

Sessions are never tracked nor sent individually, instead they are aggregated and the aggregates are sent every 30s and a final time when the web server is terminating.
As an implementation hint to the point above, when a "Client" is closed or flushed the associated "Session Flusher" shall also be flushed and submit the current aggregates the the transport, before the transport is flushed/closed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am not sure if SessionFlusher exists in sdks other than Python and JS, do they?

Comment thread src/docs/sdk/sessions.mdx
## SDK Implementation Guideline

We track the health of each release of projects in Sentry by sending session payloads from SDKs.
The session payload provide data such as session duration and the presence or absence of errors/crashes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure this is entirely a correct statement because we do not send duration in SessionAggregates

Comment thread src/docs/sdk/sessions.mdx
- user interacting with a mobile app
- user loading a web site with their favorite browser

Session aggregates are used when sending individual sessions would be undesirable or unpractical. To constrain resource usage (namely memory and network), SDKs keep track of summary information about a batch of sessions that occured in the recent past, never actually having to deal with session objects representing the individual sessions that make up the aggregate. This mode is the choice for applications that run for an arbitrarily long time and handle larger throughputs for potentially multiple users, such as web servers, background job workers, etc. Note that for those types of application, a better definition of session matches the execution of a single HTTP request or task, instead of a single execution of the whole application process.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

never actually having to deal with session objects representing the individual sessions that make up the aggregate

This is only valid for Node afaik
Python does use the Session object up until before it is added to the Transport
Not sure that is what your intention was with this comment but I guess it could be true if you are referring to how its finally sent to the server

Comment thread src/docs/sdk/sessions.mdx

In either case, SDKs should create and report sessions by default, choosing to report them individually or as aggregates depending on the type of application.

If an SDK can detect that an application is better served by session aggregates, then it must not report an application-wide session. The application-wide session may still be created during SDK initialization but must be aborted and never sent to Sentry. As an example, in the Node.js SDK, we can detect an application is probably a web server if it uses the `requestHandler` integration that is provided.

@ahmedetefy ahmedetefy May 6, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

application-wide session

Would rename this to Single session to have consistent naming across the doc

Comment thread src/docs/sdk/sessions.mdx

In either case, SDKs should create and report sessions by default, choosing to report them individually or as aggregates depending on the type of application.

If an SDK can detect that an application is better served by session aggregates, then it must not report an application-wide session. The application-wide session may still be created during SDK initialization but must be aborted and never sent to Sentry. As an example, in the Node.js SDK, we can detect an application is probably a web server if it uses the `requestHandler` integration that is provided.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

in the Node.js SDK, we can detect an application is probably a web server if it uses the requestHandler integration that is provided

I would also add

And in python we detect that an application is probably a web server through the WSGI integration

Just for better clarification

@ahmedetefy ahmedetefy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Another pass at this

Comment thread src/docs/sdk/sessions.mdx

Sessions should be enabled by default only for the global hub/client that is initialized by Sentry.init, and disabled by default for any other manually created client.
A session is started when the SDK is initialized (ideally when the default client is bound to the global hub) and ended when one of these conditions happen:
The Hub.endSession() method is explicitly called; or

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also it would be ended if a user starts a session
which is probably why we need to document that if a user wants to explicitly start and end their own sessions they should disable autoSessionTracking?

Comment thread src/docs/sdk/sessions.mdx
The program terminates with an unhandled exception; or
The program terminates with an unhandled promise rejection.

Care must be taken to never attempt to send new session payloads to Sentry for a session that is already ended. For example, if the user manually ends the session with Hub.endSession(), there should not be any new updates to the session when the program terminates.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would clarify this a bit better by saying that we should not send sessions for sessions that are in terminal states
Ref: https://develop.sentry.dev/sdk/sessions/

String, optional, default is ok. The current status of the session. A session can only be in two states effectively: ok which means the session is alive or one of the terminal states. When a session is moved away from ok it must not be updated anymore.

Comment thread src/docs/sdk/sessions.mdx

#### Sending Session to Sentry

Session is sent initially after a certain (initially hard-coded, less config is better) delay (something between 1s to 30s TBD), and then updated with the duration and final status and error count when the program terminates. Note that, as an optimization, short lived programs will not send 2 session requests to Relay, but only the final one with status and duration.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It is not so much a delay as an interval every 1 min

Comment thread src/docs/sdk/sessions.mdx

#### Sending Session to Sentry

Session is sent initially after a certain (initially hard-coded, less config is better) delay (something between 1s to 30s TBD), and then updated with the duration and final status and error count when the program terminates. Note that, as an optimization, short lived programs will not send 2 session requests to Relay, but only the final one with status and duration.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note that, as an optimization, short lived programs will not send 2 session requests to Relay, but only the final one with status and duration.

I am not sure what that means?

Comment thread src/docs/sdk/sessions.mdx

Sessions are never tracked nor sent individually, instead they are aggregated and the aggregates are sent every 30s and a final time when the web server is terminating.
As an implementation hint to the point above, when a "Client" is closed or flushed the associated "Session Flusher" shall also be flushed and submit the current aggregates the the transport, before the transport is flushed/closed.
Make sure this works reasonably for Serverless — there we shall not use "request mode" and SessionFlusher because we cannot have any work that happens outside of the request-response flow.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SessionFlusher exists for both Single Sessions and SessionAggregates in Python
SessionFlusher that only serves SessionAggregates is just an implementation detail in Node

Comment thread src/docs/sdk/sessions.mdx
Sessions are never tracked nor sent individually, instead they are aggregated and the aggregates are sent every 30s and a final time when the web server is terminating.
As an implementation hint to the point above, when a "Client" is closed or flushed the associated "Session Flusher" shall also be flushed and submit the current aggregates the the transport, before the transport is flushed/closed.
Make sure this works reasonably for Serverless — there we shall not use "request mode" and SessionFlusher because we cannot have any work that happens outside of the request-response flow.
Provide an easy way to integrate with existing Node frameworks (Express, Next.js, Koa).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would also add explanation on how the requestHandler is used for that in Node frameworks and WSGI is used for Python frameworks

@ahmedetefy

Copy link
Copy Markdown

Note: We should also add instructions on how you could manually start your process too in terms of

  • Disabling autoSessionTracking
  • Starting a session at the beginning of the process and ending a session at the end of the process

@HazAT HazAT changed the title [DO NOT MERGE] draft guidelines for sessions in SDKs Guidelines for sessions in SDKs May 17, 2021
@HazAT
HazAT marked this pull request as ready for review May 17, 2021 13:37
@HazAT

HazAT commented May 17, 2021

Copy link
Copy Markdown
Member

This is good enough to be merged and we can update this over time. Given this is only a guideline and not defacto the truth. Not merging this will do more harm than merging it, let's keep iterating if necessary.

@HazAT
HazAT merged commit 67d895e into master May 17, 2021
@HazAT
HazAT deleted the rhcarvalho/sessions-sdk-guidelines branch May 17, 2021 13:43
@robblovell

This comment has been minimized.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants