feat: add a session length maximum time limit

[pauldambra]

Changes

Enforces a maximum age for session ids. Regardless of activity in the session.

Takes advantage of the existing mechanism to rotate session id after SESSION_CHANGE_THRESHOLD milliseconds of inactivity.

Alongside that "last session activity timestamp" stores a "session start timestamp"

And checks a SESSION_LENGTH_LIMIT from that new "session start timestamp". Rolling the session and window id if that limit has passed

Those values are stored as an array: [sessionActivityTimestamp, sessionId]. The new value is added to the end of the array: [sessionActivityTimestamp, sessionId, sessionStartTimestamp].

This allows existing sessions to add a session start timestamp the next time the session is read. Without losing data if the version of posthog-js has to be rolled back

---

see PostHog/posthog#2142 or PostHog/product-internal#316 for context

Checklist

• Tests for new code (see advice on the tests we use)
• Accounted for the impact of any changes across different browsers
• TypeScript definitions (module.d.ts) updated and in sync with library exports (if applicable)

[benjackwhite]

Code-wise looks good. Relatively complex logic but I guess its a complex case.

Can't speak to how functionally-good it is as I'm not super deep into the js tracker...

[hazzadous]

Tests look sensible. Do we need to test that the new session will have a full snapshot taken as the first rrweb event or is that a guarantee that rrweb gives?

[pauldambra]

@hazzadous I think that https://github.com/PostHog/posthog-js/blob/master/src/extensions/sessionrecording.js#L110 means we take a full snapshot if the window or session id changes

[mariusandra]

Is there any way to mock/test that a full snapshot is taken in this case? Just to protect ourselves from ourselves in the future.

[pauldambra]

@mariusandra there is!

I split it into multiple tests because it felt unclear what was being tested when it was smooshed into one

[rcmarron]

A couple thoughts on this:

1. I'm not sure this robustly gets us what we want for limiting recording length. We still need to handle old clients and clients that are late in uploading data. So we're still going to have to handle long recordings on the server - although, this will definitely make that case happen less often.
2. The session_id that we're rotating here, is the same session_id that's used in session analysis. While it's defined as a 'series of events with less than a 30 min gap between them' today, looking at our competitor's docs, we will inevitably get requests to make this configurable. I worry that this 24-hour limit is going to cause some issues for some customers down the road who have unique session definitions.

It'd be great if we had a solution that didn't lean on client side validation. With that said, I get that this is probably the simplest approach, so I don't want to be a blocker here. We can always update things down the road if needed.

[pauldambra]

What we're aiming towards is that we can know when a recording is finished. In order to avoid paging across ClickHouse and Object Storage.

I worry that this 24-hour limit is going to cause some issues for some customers down the road who have unique session definitions.

I guess the question is: what is someone doing that their product has a continuous > 24 hour session?

Really, the decision we're making is that (even if configurable) PostHog doesn't support continuous sessions of more than 24 hours.

[pauldambra]

handle old clients

Here the old client would continue to send a >24h recording. They'd be able to watch it in total for one to four days (roughly) because it would be in ClickHouse.

And then we'd ship the first 24 hours to object storage and they'd only be able to watch that.

They report the bug. We tell them it's not a bug and they need to upgrade

[pauldambra]

we're still going to have to handle long recordings on the server - although, this will definitely make that case happen less often

Whether or not a team can configure a maximum session length...

On the server we can read the max timestamp for a session and assert it is further in the past than that limit . That session is implicitly finished and can be shipped to object storage.

That makes this change _almost _redundant. There's a long tail of very long sessions sent by a very small proportion of clients. This should massively shorten that tail.

[pauldambra]

There's a risk of bugs 24 hours after deploy of this PR (as sessions start to be rotated)

I'm normally pretty YOLO about Friday deploys but I don't want to be monitoring this at the weekend so will wait till Monday morning at the earliest to merge