Skip to content

Commit d712e13

Browse files
authored
fix(sessions): Correctly compute session duration (#3616)
Closes #3615 * ref: Update the session sent in `@sentry/browser` to ignore duration. The duration is not relevant at the moment as it doesn't represent anything users can relate to. * ref: Leverage timestampInSeconds from @sentry/utils instead of Date.now() to make sure that durations are correctly reported to Sentry. * test: Add tests around creating, updating and closing sessions
1 parent ef508e6 commit d712e13

File tree

5 files changed

+149
-16
lines changed

5 files changed

+149
-16
lines changed

packages/browser/src/sdk.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,11 @@ function startSessionTracking(): void {
199199
return;
200200
}
201201

202-
hub.startSession();
202+
// The session duration for browser sessions does not track a meaningful
203+
// concept that can be used as a metric.
204+
// Automatically captured sessions are akin to page views, and thus we
205+
// discard their duration.
206+
hub.startSession({ ignoreDuration: true });
203207
hub.captureSession();
204208

205209
// We want to create a session for every navigation as well
@@ -209,7 +213,7 @@ function startSessionTracking(): void {
209213
if (from === undefined || from === to) {
210214
return;
211215
}
212-
hub.startSession();
216+
hub.startSession({ ignoreDuration: true });
213217
hub.captureSession();
214218
},
215219
type: 'history',

packages/hub/src/session.ts

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {
88
SessionStatus,
99
Transport,
1010
} from '@sentry/types';
11-
import { dropUndefinedKeys, logger, uuid4 } from '@sentry/utils';
11+
import { dropUndefinedKeys, logger, timestampInSeconds, uuid4 } from '@sentry/utils';
1212

1313
import { getCurrentHub } from './hub';
1414

@@ -21,15 +21,20 @@ export class Session implements SessionInterface {
2121
public release?: string;
2222
public sid: string = uuid4();
2323
public did?: string;
24-
public timestamp: number = Date.now();
25-
public started: number = Date.now();
26-
public duration: number = 0;
24+
public timestamp: number;
25+
public started: number;
26+
public duration?: number = 0;
2727
public status: SessionStatus = SessionStatus.Ok;
2828
public environment?: string;
2929
public ipAddress?: string;
3030
public init: boolean = true;
31+
public ignoreDuration: boolean = false;
3132

3233
public constructor(context?: Omit<SessionContext, 'started' | 'status'>) {
34+
// Both timestamp and started are in seconds since the UNIX epoch.
35+
const startingTime = timestampInSeconds();
36+
this.timestamp = startingTime;
37+
this.started = startingTime;
3338
if (context) {
3439
this.update(context);
3540
}
@@ -48,8 +53,10 @@ export class Session implements SessionInterface {
4853
}
4954
}
5055

51-
this.timestamp = context.timestamp || Date.now();
52-
56+
this.timestamp = context.timestamp || timestampInSeconds();
57+
if (context.ignoreDuration) {
58+
this.ignoreDuration = context.ignoreDuration;
59+
}
5360
if (context.sid) {
5461
// Good enough uuid validation. — Kamil
5562
this.sid = context.sid.length === 32 ? context.sid : uuid4();
@@ -63,10 +70,13 @@ export class Session implements SessionInterface {
6370
if (typeof context.started === 'number') {
6471
this.started = context.started;
6572
}
66-
if (typeof context.duration === 'number') {
73+
if (this.ignoreDuration) {
74+
this.duration = undefined;
75+
} else if (typeof context.duration === 'number') {
6776
this.duration = context.duration;
6877
} else {
69-
this.duration = this.timestamp - this.started;
78+
const duration = this.timestamp - this.started;
79+
this.duration = duration >= 0 ? duration : 0;
7080
}
7181
if (context.release) {
7282
this.release = context.release;
@@ -106,7 +116,7 @@ export class Session implements SessionInterface {
106116
did?: string;
107117
timestamp: string;
108118
started: string;
109-
duration: number;
119+
duration?: number;
110120
status: SessionStatus;
111121
errors: number;
112122
attrs?: {
@@ -119,8 +129,9 @@ export class Session implements SessionInterface {
119129
return dropUndefinedKeys({
120130
sid: `${this.sid}`,
121131
init: this.init,
122-
started: new Date(this.started).toISOString(),
123-
timestamp: new Date(this.timestamp).toISOString(),
132+
// Make sure that sec is converted to ms for date constructor
133+
started: new Date(this.started * 1000).toISOString(),
134+
timestamp: new Date(this.timestamp * 1000).toISOString(),
124135
status: this.status,
125136
errors: this.errors,
126137
did: typeof this.did === 'number' || typeof this.did === 'string' ? `${this.did}` : undefined,

packages/hub/test/session.test.ts

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
import { SessionContext, SessionStatus } from '@sentry/types';
2+
import { timestampInSeconds } from '@sentry/utils';
3+
4+
import { Session } from '../src/session';
5+
6+
describe('Session', () => {
7+
it('initializes with the proper defaults', () => {
8+
const session = new Session().toJSON();
9+
10+
// Grab current year to check if we are converting from sec -> ms correctly
11+
const currentYear = new Date(timestampInSeconds() * 1000).toISOString().slice(0, 4);
12+
expect(session).toEqual({
13+
attrs: {},
14+
duration: 0,
15+
errors: 0,
16+
init: true,
17+
sid: expect.any(String),
18+
started: expect.stringMatching(currentYear),
19+
status: SessionStatus.Ok,
20+
timestamp: expect.stringMatching(currentYear),
21+
});
22+
23+
expect(session.sid).toHaveLength(32);
24+
25+
// started and timestamp should be the same on creation
26+
expect(session.started).toEqual(session.timestamp);
27+
});
28+
29+
describe('update', () => {
30+
const time = timestampInSeconds();
31+
// [ name, in, out ]
32+
const table: Array<[string, SessionContext, Record<string, any>]> = [
33+
['sets an ip address', { user: { ip_address: '0.0.0.0' } }, { attrs: { ip_address: '0.0.0.0' } }],
34+
['sets a did', { user: { id: 'specialID123' } }, { did: 'specialID123' }],
35+
['sets a timestamp', { timestamp: time }, { timestamp: new Date(time * 1000).toISOString() }],
36+
['sets a sid', { sid: '99705f22a3f1468e95ba7386e84691aa' }, { sid: '99705f22a3f1468e95ba7386e84691aa' }],
37+
['requires custom sid to be of certain length', { sid: '123' }, { sid: expect.not.stringMatching('123') }],
38+
['requires custom sid to be of certain length', { sid: '123' }, { sid: expect.not.stringMatching('123') }],
39+
['sets an init', { init: false }, { init: false }],
40+
['sets an did', { did: 'specialID123' }, { did: 'specialID123' }],
41+
['overwrites user did with custom did', { did: 'custom-did', user: { id: 'user-id' } }, { did: 'custom-did' }],
42+
['sets a started time', { started: time }, { started: new Date(time * 1000).toISOString() }],
43+
['does not set a duration for browser env', { ignoreDuration: true }, { duration: undefined }],
44+
['sets a duration', { duration: 12000 }, { duration: 12000 }],
45+
[
46+
'does not use custom duration for browser env',
47+
{ duration: 12000, ignoreDuration: true },
48+
{ duration: undefined },
49+
],
50+
[
51+
'does not set a negative duration',
52+
{ timestamp: 10, started: 100 },
53+
{ duration: 0, timestamp: expect.any(String), started: expect.any(String) },
54+
],
55+
[
56+
'sets duration based on timestamp and started',
57+
{ timestamp: 100, started: 10 },
58+
{ duration: 90, timestamp: expect.any(String), started: expect.any(String) },
59+
],
60+
[
61+
'sets a release',
62+
{ release: 'f1557994979ecd969963f53c27ca946379d721f3' },
63+
{ attrs: { release: 'f1557994979ecd969963f53c27ca946379d721f3' } },
64+
],
65+
['sets an environment', { environment: 'staging' }, { attrs: { environment: 'staging' } }],
66+
['sets an ipAddress', { ipAddress: '0.0.0.0' }, { attrs: { ip_address: '0.0.0.0' } }],
67+
[
68+
'overwrites user ip_address did with custom ipAddress',
69+
{ ipAddress: '0.0.0.0', user: { ip_address: '1.1.1.1' } },
70+
{ attrs: { ip_address: '0.0.0.0' } },
71+
],
72+
['sets an userAgent', { userAgent: 'Mozilla/5.0' }, { attrs: { user_agent: 'Mozilla/5.0' } }],
73+
['sets errors', { errors: 3 }, { errors: 3 }],
74+
['sets status', { status: SessionStatus.Crashed }, { status: SessionStatus.Crashed }],
75+
];
76+
77+
test.each(table)('%s', (...test) => {
78+
// duration and timestamp can vary after session update, so let's expect anything unless
79+
// the out variable in a test explicitly refers to it.
80+
const DEFAULT_OUT = { duration: expect.any(Number), timestamp: expect.any(String) };
81+
82+
const session = new Session();
83+
const initSessionProps = session.toJSON();
84+
85+
session.update(test[1]);
86+
expect(session.toJSON()).toEqual({ ...initSessionProps, ...DEFAULT_OUT, ...test[2] });
87+
});
88+
});
89+
90+
describe('close', () => {
91+
it('exits a normal session', () => {
92+
const session = new Session();
93+
expect(session.status).toEqual(SessionStatus.Ok);
94+
session.close();
95+
expect(session.status).toEqual(SessionStatus.Exited);
96+
});
97+
98+
it('updates session status when give status', () => {
99+
const session = new Session();
100+
expect(session.status).toEqual(SessionStatus.Ok);
101+
102+
session.close(SessionStatus.Abnormal);
103+
expect(session.status).toEqual(SessionStatus.Abnormal);
104+
});
105+
106+
it('only changes status ok to exited', () => {
107+
const session = new Session();
108+
session.update({ status: SessionStatus.Crashed });
109+
expect(session.status).toEqual(SessionStatus.Crashed);
110+
111+
session.close();
112+
expect(session.status).toEqual(SessionStatus.Crashed);
113+
});
114+
});
115+
});

packages/types/src/session.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ export interface Session extends SessionContext {
1717
did?: string;
1818
timestamp: string;
1919
started: string;
20-
duration: number;
20+
duration?: number;
2121
status: SessionStatus;
2222
errors: number;
2323
attrs?: {
@@ -40,7 +40,9 @@ export interface SessionContext {
4040
sid?: string;
4141
did?: string;
4242
init?: boolean;
43+
// seconds since the UNIX epoch
4344
timestamp?: number;
45+
// seconds since the UNIX epoch
4446
started?: number;
4547
duration?: number;
4648
status?: SessionStatus;
@@ -50,6 +52,7 @@ export interface SessionContext {
5052
ipAddress?: string;
5153
errors?: number;
5254
user?: User | null;
55+
ignoreDuration?: boolean;
5356
}
5457

5558
/**

packages/utils/src/time.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ const timestampSource: TimestampSource =
103103
/**
104104
* Returns a timestamp in seconds since the UNIX epoch using the Date API.
105105
*/
106-
export const dateTimestampInSeconds = dateTimestampSource.nowSeconds.bind(dateTimestampSource);
106+
export const dateTimestampInSeconds: () => number = dateTimestampSource.nowSeconds.bind(dateTimestampSource);
107107

108108
/**
109109
* Returns a timestamp in seconds since the UNIX epoch using either the Performance or Date APIs, depending on the
@@ -116,7 +116,7 @@ export const dateTimestampInSeconds = dateTimestampSource.nowSeconds.bind(dateTi
116116
* skew can grow to arbitrary amounts like days, weeks or months.
117117
* See https://github.com/getsentry/sentry-javascript/issues/2590.
118118
*/
119-
export const timestampInSeconds = timestampSource.nowSeconds.bind(timestampSource);
119+
export const timestampInSeconds: () => number = timestampSource.nowSeconds.bind(timestampSource);
120120

121121
// Re-exported with an old name for backwards-compatibility.
122122
export const timestampWithMs = timestampInSeconds;

0 commit comments

Comments
 (0)