-
Notifications
You must be signed in to change notification settings - Fork 131
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: different default and max idle period #1558
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size Change: +2.26 kB (+0.07%) Total Size: 3.15 MB
ℹ️ View Unchanged
|
@@ -427,7 +427,7 @@ export class PostHog { | |||
this._retryQueue = new RetryQueue(this) | |||
this.__request_queue = [] | |||
|
|||
this.sessionManager = new SessionIdManager(this.config, this.persistence) | |||
this.sessionManager = new SessionIdManager(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chose to pass in the posthog instance rather than pass in this.register
export const DEFAULT_SESSION_IDLE_TIMEOUT_SECONDS = 30 * 60 // 30 minutes | ||
export const MAX_SESSION_IDLE_TIMEOUT_SECONDS = 10 * 60 * 60 // 10 hours | ||
const MIN_SESSION_IDLE_TIMEOUT_SECONDS = 60 // 1 minute | ||
const SESSION_LENGTH_LIMIT_MILLISECONDS = 24 * 3600 * 1000 // 24 hours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed these since they have different units
added DEFAULT and MAX as separate values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
picked ten hours as the new max but 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine w/ me, the sessions table queries assume 24 hours and some queries might break with sessions longer than that, but anything up to and including 24 hours is fine.
MIN_SESSION_IDLE_TIMEOUT_SECONDS, | ||
MAX_SESSION_IDLE_TIMEOUT_SECONDS, | ||
'session_idle_timeout_seconds', | ||
DEFAULT_SESSION_IDLE_TIMEOUT_SECONDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now the clampToRange
function needs to take an arg that tells it what value to use if the provided value is invalid, previously it would use its max, but in this case we want DEFAULT not MAX
so,
- desired > max -> max
- desired < min -> min
- desired in range -> desired
- desired is invalid -> default (not max)
tagging @robbie-c too in case this would be bad for sessions table |
allow configuring an idle timeout larger than the default
see https://posthog.slack.com/archives/C07RUA1TBT3/p1732619524510359
the motivation for the user here is that their product might be used by someone before they leave for the day, and then once they return, and those two interactions are logically one session. the second might follow-on from the first.