fix: Do not trigger session on meaningless navigation #3608
Conversation
AbhiPrasad
left a comment
AbhiPrasad
left a comment
There was a problem hiding this comment.
I would double check with Daniel to confirm, but this LGTM
size-limit report
|
Co-authored-by: Abhijeet Prasad <[email protected]>
packages/hub/src/session.ts
Outdated
| // Otherwise: | ||
| // hub.startSession(); | ||
| // hub.captureSession(); | ||
| // can produce either 0 or 1, depending on the mood | ||
| this.duration = Math.floor(this.timestamp - this.started); |
There was a problem hiding this comment.
https://develop.sentry.dev/sdk/sessions/#session-update-payload documents duration to be a float
What exactly was the problem here? If float is not being ingested correctly then we might need changes elsewhere.
There was a problem hiding this comment.
‘Date.now() - Date.now()’ which is what we have here will always produce int.
There was a problem hiding this comment.
If both are ints, what's the deal with rounding? This doesn't make sense.
If we want to always report zero then Math.floor is not part of the solution.
There was a problem hiding this comment.
New session is initially at 0, but captureSession is passing its own Date.now() as endTimestamp. Its possible that 2 calls followed after each other (as in code comment) will have 2 differents Date.now (by 1ms), which will produce a duration of 1.
There was a problem hiding this comment.
Team talked about this on a call, decided that we:
- Do not send duration for JS Browser sessions.
- Fix duration calculation to report seconds instead of milliseconds and to use the same clock as we use for errors and transactions. See
timestampInSeconds. - For this PR, revert the above changes to
packages/hub/src/session.tsand merge the changes topackages/browser/src/sdk.ts.
We'll fix the reporting of session durations separately later.
rhcarvalho
changed the title
rhcarvalho
changed the title
Bug found when doing end-to-end tests for the
@sentry/nextjsSDK.