-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(replay): Log warning if trying to flush initial segment without checkout #8748
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,22 +11,42 @@ export function logInfo(message: string, shouldAddBreadcrumb?: boolean): void { | |
|
||
logger.info(message); | ||
|
||
if (shouldAddBreadcrumb) { | ||
addBreadcrumb(message); | ||
} | ||
} | ||
|
||
/** | ||
* Log a message, and add a breadcrumb in the next tick. | ||
* This is necessary when the breadcrumb may be added before the replay is initialized. | ||
*/ | ||
export function logInfoNextTick(message: string, shouldAddBreadcrumb?: boolean): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ended up splitting this into two methods |
||
if (!__DEBUG_BUILD__) { | ||
return; | ||
} | ||
|
||
logger.info(message); | ||
|
||
if (shouldAddBreadcrumb) { | ||
// Wait a tick here to avoid race conditions for some initial logs | ||
// which may be added before replay is initialized | ||
setTimeout(() => { | ||
const hub = getCurrentHub(); | ||
hub.addBreadcrumb( | ||
{ | ||
category: 'console', | ||
data: { | ||
logger: 'replay', | ||
}, | ||
level: 'info', | ||
message, | ||
}, | ||
{ level: 'info' }, | ||
); | ||
addBreadcrumb(message); | ||
}, 0); | ||
} | ||
} | ||
|
||
function addBreadcrumb(message: string): void { | ||
const hub = getCurrentHub(); | ||
hub.addBreadcrumb( | ||
{ | ||
category: 'console', | ||
data: { | ||
logger: 'replay', | ||
}, | ||
level: 'info', | ||
message, | ||
}, | ||
{ level: 'info' }, | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import * as SentryUtils from '@sentry/utils'; | ||
|
||
import { DEFAULT_FLUSH_MIN_DELAY, WINDOW } from '../../src/constants'; | ||
import { DEFAULT_FLUSH_MIN_DELAY, MAX_SESSION_LIFE, WINDOW } from '../../src/constants'; | ||
import type { ReplayContainer } from '../../src/replay'; | ||
import { clearSession } from '../../src/session/clearSession'; | ||
import type { EventBuffer } from '../../src/types'; | ||
|
@@ -286,15 +286,22 @@ describe('Integration | flush', () => { | |
|
||
expect(mockFlush).toHaveBeenCalledTimes(20); | ||
expect(mockSendReplay).toHaveBeenCalledTimes(1); | ||
|
||
replay.getOptions().minReplayDuration = 0; | ||
}); | ||
|
||
it('does not flush if session is too long', async () => { | ||
replay.timeouts.maxSessionLife = 100_000; | ||
jest.setSystemTime(new Date(BASE_TIMESTAMP)); | ||
jest.setSystemTime(BASE_TIMESTAMP); | ||
|
||
sessionStorage.clear(); | ||
clearSession(replay); | ||
replay['_loadAndCheckSession'](); | ||
// No-op _loadAndCheckSession to avoid us resetting the session for this test | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually noticed this test was incorrect before, it just worked because we never reset |
||
const _tmp = replay['_loadAndCheckSession']; | ||
replay['_loadAndCheckSession'] = () => { | ||
return true; | ||
}; | ||
|
||
await advanceTimers(120_000); | ||
|
||
|
@@ -308,7 +315,71 @@ describe('Integration | flush', () => { | |
mockRecord._emitter(TEST_EVENT); | ||
|
||
await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); | ||
|
||
expect(mockFlush).toHaveBeenCalledTimes(1); | ||
expect(mockSendReplay).toHaveBeenCalledTimes(0); | ||
|
||
replay.timeouts.maxSessionLife = MAX_SESSION_LIFE; | ||
replay['_loadAndCheckSession'] = _tmp; | ||
}); | ||
|
||
it('logs warning if flushing initial segment without checkout', async () => { | ||
replay.getOptions()._experiments.traceInternals = true; | ||
|
||
sessionStorage.clear(); | ||
clearSession(replay); | ||
replay['_loadAndCheckSession'](); | ||
await new Promise(process.nextTick); | ||
jest.setSystemTime(BASE_TIMESTAMP); | ||
|
||
// Clear the event buffer to simulate no checkout happened | ||
replay.eventBuffer!.clear(); | ||
|
||
// click happens first | ||
domHandler({ | ||
name: 'click', | ||
}); | ||
|
||
// no checkout! | ||
await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); | ||
|
||
expect(mockFlush).toHaveBeenCalledTimes(1); | ||
expect(mockSendReplay).toHaveBeenCalledTimes(1); | ||
|
||
const replayData = mockSendReplay.mock.calls[0][0]; | ||
|
||
expect(JSON.parse(replayData.recordingData)).toEqual([ | ||
{ | ||
type: 5, | ||
timestamp: BASE_TIMESTAMP, | ||
data: { | ||
tag: 'breadcrumb', | ||
payload: { | ||
timestamp: BASE_TIMESTAMP / 1000, | ||
type: 'default', | ||
category: 'ui.click', | ||
message: '<unknown>', | ||
data: {}, | ||
}, | ||
}, | ||
}, | ||
{ | ||
type: 5, | ||
timestamp: BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY, | ||
data: { | ||
tag: 'breadcrumb', | ||
payload: { | ||
timestamp: (BASE_TIMESTAMP + DEFAULT_FLUSH_MIN_DELAY) / 1000, | ||
type: 'default', | ||
category: 'console', | ||
data: { logger: 'replay' }, | ||
level: 'info', | ||
message: '[Replay] Flushing initial segment without checkout.', | ||
}, | ||
}, | ||
}, | ||
]); | ||
|
||
replay.getOptions()._experiments.traceInternals = false; | ||
}); | ||
}); |
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.
Hmm won't this log for session based replays?
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.
there should still always be a checkout in the initial segment, right?