From 36521eb693cb3395a359a2084edce7173e71fa57 Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Oct 2023 17:03:17 +0200 Subject: [PATCH] fix: Performance issue with player logs (#17948) --- .../notebooks/Notebook/SlashCommands.tsx | 65 ++++++++++++++++- .../NotebookSelectButton.stories.tsx | 22 +++++- .../NotebookSelectButton.tsx | 20 ++++- .../sessionRecordingPlayerLogic.test.ts | 30 ++++++++ .../player/sessionRecordingPlayerLogic.ts | 73 ++++++++++++------- 5 files changed, 176 insertions(+), 34 deletions(-) diff --git a/frontend/src/scenes/notebooks/Notebook/SlashCommands.tsx b/frontend/src/scenes/notebooks/Notebook/SlashCommands.tsx index 87d5ee8c1e5c2..04a27423ee2b8 100644 --- a/frontend/src/scenes/notebooks/Notebook/SlashCommands.tsx +++ b/frontend/src/scenes/notebooks/Notebook/SlashCommands.tsx @@ -19,13 +19,14 @@ import { import { forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useState } from 'react' import { EditorCommands, EditorRange } from './utils' import { NotebookNodeType } from '~/types' -import { examples } from '~/queries/examples' import { Popover } from 'lib/lemon-ui/Popover' import { KeyboardShortcut } from '~/layout/navigation-3000/components/KeyboardShortcut' import Fuse from 'fuse.js' import { useValues } from 'kea' import { notebookLogic } from './notebookLogic' import { selectFile } from '../Nodes/utils' +import { NodeKind } from '~/queries/schema' +import { defaultDataTableColumns } from '~/queries/nodes/DataTable/utils' type SlashCommandsProps = { mode: 'slash' | 'add' @@ -243,21 +244,77 @@ const SLASH_COMMANDS: SlashCommandsItem[] = [ search: 'sql', icon: , command: (chain) => - chain.insertContent({ type: NotebookNodeType.Query, attrs: { query: examples['HogQLTable'] } }), + chain.insertContent({ + type: NotebookNodeType.Query, + attrs: { + query: { + kind: NodeKind.DataTableNode, + full: true, + source: { + kind: NodeKind.HogQLQuery, + query: `select event, + person.properties.email, + properties.$browser, + count() + from events + where {filters} -- replaced with global date and property filters + and person.properties.email is not null + group by event, + properties.$browser, + person.properties.email + order by count() desc + limit 100`, + filters: { + dateRange: { + date_from: '-24h', + }, + }, + }, + }, + }, + }), }, { title: 'Events', search: 'data explore', icon: , command: (chain) => - chain.insertContent({ type: NotebookNodeType.Query, attrs: { query: examples['EventsTableFull'] } }), + chain.insertContent({ + type: NotebookNodeType.Query, + attrs: { + query: { + kind: NodeKind.DataTableNode, + full: true, + source: { + kind: NodeKind.EventsQuery, + select: defaultDataTableColumns(NodeKind.EventsQuery), + properties: [], + after: '-24h', + limit: 100, + }, + }, + }, + }), }, { title: 'Persons', search: 'people users', icon: , command: (chain) => - chain.insertContent({ type: NotebookNodeType.Query, attrs: { query: examples['PersonsTableFull'] } }), + chain.insertContent({ + type: NotebookNodeType.Query, + attrs: { + query: { + kind: NodeKind.DataTableNode, + full: true, + columns: defaultDataTableColumns(NodeKind.PersonsNode), + source: { + kind: NodeKind.PersonsNode, + properties: [], + }, + }, + }, + }), }, { title: 'Session Replays', diff --git a/frontend/src/scenes/notebooks/NotebookSelectButton/NotebookSelectButton.stories.tsx b/frontend/src/scenes/notebooks/NotebookSelectButton/NotebookSelectButton.stories.tsx index 64e6fadadb513..1baff1b2871f6 100644 --- a/frontend/src/scenes/notebooks/NotebookSelectButton/NotebookSelectButton.stories.tsx +++ b/frontend/src/scenes/notebooks/NotebookSelectButton/NotebookSelectButton.stories.tsx @@ -13,9 +13,27 @@ const allNotebooks = [ { title: 'my amazing notebook', short_id: 'abc', + created_by: { + first_name: 'Ben', + email: 'ben@posthog.com', + }, + }, + { + title: 'and another amazing notebook', + short_id: 'def', + created_by: { + first_name: 'Paul', + email: 'paul@posthog.com', + }, + }, + { + title: 'an empty notebook', + short_id: 'ghi', + created_by: { + first_name: 'David', + email: 'david@posthog.com', + }, }, - { title: 'and another amazing notebook', short_id: 'def' }, - { title: 'an empty notebook', short_id: 'ghi' }, ] const Template: StoryFn = (props) => { diff --git a/frontend/src/scenes/notebooks/NotebookSelectButton/NotebookSelectButton.tsx b/frontend/src/scenes/notebooks/NotebookSelectButton/NotebookSelectButton.tsx index edc4a3a3173b7..8aec2c9ffc917 100644 --- a/frontend/src/scenes/notebooks/NotebookSelectButton/NotebookSelectButton.tsx +++ b/frontend/src/scenes/notebooks/NotebookSelectButton/NotebookSelectButton.tsx @@ -17,7 +17,7 @@ import { notebookNodeLogicType } from '../Nodes/notebookNodeLogicType' import { FlaggedFeature } from 'lib/components/FlaggedFeature' import { FEATURE_FLAGS } from 'lib/constants' import { ReactChild, useEffect } from 'react' -import { LemonDivider } from '@posthog/lemon-ui' +import { LemonDivider, ProfilePicture } from '@posthog/lemon-ui' export type NotebookSelectProps = NotebookSelectButtonLogicProps & { newNotebookTitle?: string @@ -50,8 +50,22 @@ function NotebooksChoiceList(props: { ) : ( props.notebooks.map((notebook, i) => { return ( - props.onClick(notebook.short_id)}> - {notebook.title || `Untitled (${notebook.short_id})`} + `} + /> + ) : null + } + fullWidth + onClick={() => props.onClick(notebook.short_id)} + > + {notebook.title || `Untitled (${notebook.short_id})`} ) }) diff --git a/frontend/src/scenes/session-recordings/player/sessionRecordingPlayerLogic.test.ts b/frontend/src/scenes/session-recordings/player/sessionRecordingPlayerLogic.test.ts index b93b36f6df6bb..8b4526d450338 100644 --- a/frontend/src/scenes/session-recordings/player/sessionRecordingPlayerLogic.test.ts +++ b/frontend/src/scenes/session-recordings/player/sessionRecordingPlayerLogic.test.ts @@ -18,8 +18,11 @@ import { urls } from 'scenes/urls' describe('sessionRecordingPlayerLogic', () => { let logic: ReturnType + const mockWarn = jest.fn() beforeEach(() => { + console.warn = mockWarn + mockWarn.mockClear() useMocks({ get: { '/api/projects/:team/session_recordings/:id/snapshots/': (req, res, ctx) => { @@ -324,5 +327,32 @@ describe('sessionRecordingPlayerLogic', () => { }), }) }) + + it('captures replayer warnings', async () => { + jest.useFakeTimers() + logic = sessionRecordingPlayerLogic({ + sessionRecordingId: '4', + playerKey: 'test', + matchingEventsMatchType: { + matchType: 'uuid', + eventUUIDs: listOfMatchingEvents.map((event) => event.uuid), + }, + }) + logic.mount() + + console.warn('[replayer]', 'test') + console.warn('[replayer]', 'test2') + + expect(mockWarn).not.toHaveBeenCalled() + + expect((window as any).__posthog_player_warnings).toEqual([ + ['[replayer]', 'test'], + ['[replayer]', 'test2'], + ]) + jest.runOnlyPendingTimers() + expect(mockWarn).toHaveBeenCalledWith( + '[PostHog Replayer] 2 warnings (window.__posthog_player_warnings to safely log them)' + ) + }) }) }) diff --git a/frontend/src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts b/frontend/src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts index f01b3f67ca870..d5df80dd1fda5 100644 --- a/frontend/src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts +++ b/frontend/src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts @@ -958,13 +958,13 @@ export const sessionRecordingPlayerLogic = kea( delete (window as any).__debug_player actions.stopAnimation() - cache.resetConsoleWarn?.() + cache.hasInitialized = false - clearTimeout(cache.consoleWarnDebounceTimer) document.removeEventListener('fullscreenchange', cache.fullScreenListener) cache.pausedMediaElements = [] values.player?.replayer?.pause() actions.setPlayer(null) + cache.unmountConsoleWarns?.() const playTimeMs = values.playingTimeTracking.watchTime || 0 const summaryAnalytics: RecordingViewedSummaryAnalytics = { @@ -1017,29 +1017,7 @@ export const sessionRecordingPlayerLogic = kea( cache.openTime = performance.now() - // NOTE: RRWeb can log _alot_ of warnings, so we debounce the count otherwise we just end up making the performance worse - let warningCount = 0 - cache.consoleWarnDebounceTimer = null - - const debouncedCounter = (): void => { - warningCount += 1 - - if (!cache.consoleWarnDebounceTimer) { - cache.consoleWarnDebounceTimer = setTimeout(() => { - cache.consoleWarnDebounceTimer = null - actions.incrementWarningCount(warningCount) - warningCount = 0 - }, 1000) - } - } - - cache.resetConsoleWarn = wrapConsole('warn', (args) => { - if (typeof args[0] === 'string' && args[0].includes('[replayer]')) { - debouncedCounter() - } - - return true - }) + cache.unmountConsoleWarns = manageConsoleWarns(cache, actions.incrementWarningCount) }), ]) @@ -1048,3 +1026,48 @@ export const getCurrentPlayerTime = (logicProps: SessionRecordingPlayerLogicProp const playerTime = sessionRecordingPlayerLogic.findMounted(logicProps)?.values.currentPlayerTime || 0 return Math.floor(playerTime / 1000) } + +export const manageConsoleWarns = (cache: any, onIncrement: (count: number) => void): (() => void) => { + // NOTE: RRWeb can log _alot_ of warnings, so we debounce the count otherwise we just end up making the performance worse + // We also don't log the warnings directly. Sometimes the sheer size of messages and warnings can cause the browser to crash deserializing it all + ;(window as any).__posthog_player_warnings = [] + const warnings: any[][] = (window as any).__posthog_player_warnings + + let counter = 0 + + let consoleWarnDebounceTimer: NodeJS.Timeout | null = null + + const actualConsoleWarn = console.warn + + const debouncedCounter = (args: any[]): void => { + warnings.push(args) + counter += 1 + + if (!consoleWarnDebounceTimer) { + consoleWarnDebounceTimer = setTimeout(() => { + consoleWarnDebounceTimer = null + onIncrement(warnings.length) + + actualConsoleWarn( + `[PostHog Replayer] ${counter} warnings (window.__posthog_player_warnings to safely log them)` + ) + counter = 0 + }, 1000) + } + } + + const resetConsoleWarn = wrapConsole('warn', (args) => { + if (typeof args[0] === 'string' && args[0].includes('[replayer]')) { + debouncedCounter(args) + // WARNING: Logging these out can cause the browser to completely crash, so we want to delay it and + return false + } + + return true + }) + + return () => { + resetConsoleWarn() + clearTimeout(cache.consoleWarnDebounceTimer) + } +}