🐛 fix session management and event attribution issues#79
Conversation
It caused some issues if the app name is containing some non ascii characters. As we don't really need to capture it here, let's drop it.
- discard events generated after session expiry (gap → DISCARDED) - document the Infinity↔null disk format contract - add e2e scenarios for session attribution and crash recovery
fix unclosed history entry accumulation across restarts
Previous app has been deleted
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8f1ef1140
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 022a43103e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function getUserAgent(): Promise<string> { | ||
| return getOSUserAgentPart().then((osPart) => | ||
| [ | ||
| `${app.getName()}/${app.getVersion()}`, |
There was a problem hiding this comment.
Should we really remove it instead of sanitizing it ?
There was a problem hiding this comment.
This part in the user agent is not used and only visible when looking at the useragent attribute in the event JSON.
Since app name will be injected by backend from the created rum app and version needs to be configured at sdk init to be added properly into the events, I don't think it is really useful.
| const hookResult = this.hooks.triggerRum({ | ||
| eventType: event.data.type, | ||
| startTime: event.startTime ?? timeStampNow(), | ||
| startTime: event.data.date as TimeStamp, |
| if (startTime <= entry.endTime) { | ||
| return entry.value; | ||
| } | ||
| break; |
There was a problem hiding this comment.
Why do we need this, are we expecting gaps between sessions ? I expected that if some even happens in-between 2 sessions it would be added to the older session. If we're expecting that some event could occur between a timeframe where a session ended and before another one started, this means the issue is actually that we are ending sessions incorrectly no ?
There was a problem hiding this comment.
are we expecting gaps between sessions ?
yes, we could have a session expiring due to end user inactivity during 15 min and a new one starting only when the end user come back and interact again with the application.
I expected that if some even happens in-between 2 sessions it would be added to the older session.
We shouldn't collect the events happening outside of an active session.
For example, we could have events generated regularly, like requests from a regular poll to the backend.
The session should still expire if there is no end user activity and the requests happening when no session is active should not be collected.
this means the issue is actually that we are ending sessions incorrectly no ?
Let's chat to see if there are some missing case
2 participants
Motivation
Several bugs were identified from an internal user report. This PR addresses session management, event attribution, and user activity detection issues.
Changes
TimeStampValueHistory.find()to requirestartTime <= entry.endTime, preventing stale session/view attribution for events arriving after a session endedAssemblyto useevent.data.date(renderer event timestamp) for hook context resolution instead of the arrival timeUserActivityTracker— registers a handler that listens for renderer RUM events and emitsEND_USER_ACTIVITYwhen a click action is detected, keeping sessions alive from real user interactionsTest instructions
On the playground:
lastActivitytimestampChecklist