From 7cc55daf34410ff05342c95117a04d4f6f2017bd Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Mon, 1 Feb 2021 11:40:20 -0800 Subject: [PATCH] Stop storing device logs on the device object and in the plugin Summary: Logs were stored hardcoded on the Device object first, this diff makes it normal plugin state. This makes sure that we can use the same abstractions as in all plugins that store large data sets, and that we can leverage the upcoming DataSource abstraction. Reviewed By: nikoant Differential Revision: D26127243 fbshipit-source-id: 7c386a615fa7989f35ba0df5b7c1d218d37b57a2 --- .../createMockFlipperWithPlugin.node.tsx.snap | 2 - desktop/app/src/devices/AndroidDevice.tsx | 2 - desktop/app/src/devices/ArchivedDevice.tsx | 14 +--- desktop/app/src/devices/BaseDevice.tsx | 19 ----- desktop/app/src/devices/MetroDevice.tsx | 1 - .../src/utils/__tests__/exportData.node.tsx | 17 ----- desktop/app/src/utils/exportData.tsx | 13 +--- desktop/app/src/utils/reduxDevToolsConfig.tsx | 10 +-- .../flipper-plugin/src/plugin/PluginBase.tsx | 12 ++-- desktop/plugins/logs/__tests__/index.node.ts | 69 +++++++++++++++++++ desktop/plugins/logs/index.tsx | 47 +++++++++---- 11 files changed, 112 insertions(+), 94 deletions(-) diff --git a/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap b/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap index 8c290e6d2..7c84e10a2 100644 --- a/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap +++ b/desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap @@ -19,7 +19,6 @@ Object { "devices": Array [ Object { "deviceType": "physical", - "logs": Array [], "os": "Android", "serial": "serial", "title": "MockAndroidDevice", @@ -28,7 +27,6 @@ Object { "selectedApp": "TestApp#Android#MockAndroidDevice#serial", "selectedDevice": Object { "deviceType": "physical", - "logs": Array [], "os": "Android", "serial": "serial", "title": "MockAndroidDevice", diff --git a/desktop/app/src/devices/AndroidDevice.tsx b/desktop/app/src/devices/AndroidDevice.tsx index 1666e5dc7..8be72ddf3 100644 --- a/desktop/app/src/devices/AndroidDevice.tsx +++ b/desktop/app/src/devices/AndroidDevice.tsx @@ -86,7 +86,6 @@ export default class AndroidDevice extends BaseDevice { } clearLogs(): Promise { - this.logEntries = []; return this.executeShell(['logcat', '-c']); } @@ -96,7 +95,6 @@ export default class AndroidDevice extends BaseDevice { deviceType: this.deviceType, title: this.title, os: this.os, - logEntries: [...this.logEntries], screenshotHandle: null, }); } diff --git a/desktop/app/src/devices/ArchivedDevice.tsx b/desktop/app/src/devices/ArchivedDevice.tsx index 32ffdbc20..82b8b5a06 100644 --- a/desktop/app/src/devices/ArchivedDevice.tsx +++ b/desktop/app/src/devices/ArchivedDevice.tsx @@ -8,7 +8,7 @@ */ import BaseDevice from './BaseDevice'; -import type {DeviceLogEntry, DeviceType} from 'flipper-plugin'; +import type {DeviceType} from 'flipper-plugin'; import {OS, DeviceShell} from './BaseDevice'; import {SupportFormRequestDetailsState} from '../reducers/supportForm'; @@ -18,19 +18,16 @@ export default class ArchivedDevice extends BaseDevice { deviceType: DeviceType; title: string; os: OS; - logEntries: Array; screenshotHandle: string | null; source?: string; supportRequestDetails?: SupportFormRequestDetailsState; }) { super(options.serial, options.deviceType, options.title, options.os); - this.logs = options.logEntries; this.source = options.source || ''; this.supportRequestDetails = options.supportRequestDetails; this.archivedScreenshotHandle = options.screenshotHandle; } - logs: Array; archivedScreenshotHandle: string | null; isArchived = true; @@ -40,15 +37,6 @@ export default class ArchivedDevice extends BaseDevice { supportRequestDetails?: SupportFormRequestDetailsState; - getLogs() { - return this.logs; - } - - clearLogs(): Promise { - this.logs = []; - return Promise.resolve(); - } - spawnShell(): DeviceShell | undefined | null { return null; } diff --git a/desktop/app/src/devices/BaseDevice.tsx b/desktop/app/src/devices/BaseDevice.tsx index 855de5810..0d14ddfda 100644 --- a/desktop/app/src/devices/BaseDevice.tsx +++ b/desktop/app/src/devices/BaseDevice.tsx @@ -33,7 +33,6 @@ export type DeviceExport = { title: string; deviceType: DeviceType; serial: string; - logs: Array; pluginStates: Record; }; @@ -71,7 +70,6 @@ export default class BaseDevice { icon: string | null | undefined; logListeners: Map = new Map(); - logEntries: Array = []; isArchived: boolean = false; // if imported, stores the original source location source = ''; @@ -116,7 +114,6 @@ export default class BaseDevice { title: this.title, deviceType: this.deviceType, serial: this.serial, - logs: this.getLogs(), }; } @@ -146,25 +143,9 @@ export default class BaseDevice { } addLogEntry(entry: DeviceLogEntry) { - this.logEntries.push(entry); this._notifyLogListeners(entry); } - // TODO: remove getLogs T70688226 - getLogs(startDate: Date | null = null) { - return startDate != null - ? this.logEntries.filter((log) => { - return log.date > startDate; - }) - : this.logEntries; - } - - clearLogs(): Promise { - // Only for device types that allow clearing. - this.logEntries = []; - return Promise.resolve(); - } - removeLogListener(id: Symbol) { this.logListeners.delete(id); } diff --git a/desktop/app/src/devices/MetroDevice.tsx b/desktop/app/src/devices/MetroDevice.tsx index 7d6c68f1e..8d0fc8cb2 100644 --- a/desktop/app/src/devices/MetroDevice.tsx +++ b/desktop/app/src/devices/MetroDevice.tsx @@ -205,7 +205,6 @@ export default class MetroDevice extends BaseDevice { deviceType: this.deviceType, title: this.title, os: this.os, - logEntries: [...this.logEntries], screenshotHandle: null, }); } diff --git a/desktop/app/src/utils/__tests__/exportData.node.tsx b/desktop/app/src/utils/__tests__/exportData.node.tsx index ada698aad..d7466b5a8 100644 --- a/desktop/app/src/utils/__tests__/exportData.node.tsx +++ b/desktop/app/src/utils/__tests__/exportData.node.tsx @@ -108,7 +108,6 @@ test('test generateClientIndentifierWithSalt helper function', () => { deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const identifier = generateClientIdentifier(device, 'app'); @@ -123,7 +122,6 @@ test('test generateClientFromClientWithSalt helper function', () => { deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const client = generateClientFromDevice(device, 'app'); @@ -154,7 +152,6 @@ test('test generateClientFromDevice helper function', () => { deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const client = generateClientFromDevice(device, 'app'); @@ -175,7 +172,6 @@ test('test generateClientIdentifier helper function', () => { deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const identifier = generateClientIdentifier(device, 'app'); @@ -218,7 +214,6 @@ test('test processStore function for an iOS device connected', async () => { deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }), pluginStates: {}, @@ -254,7 +249,6 @@ test('test processStore function for an iOS device connected with client plugin deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const client = generateClientFromDevice(device, 'testapp'); @@ -305,7 +299,6 @@ test('test processStore function to have only the client for the selected device deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const unselectedDevice = new ArchivedDevice({ @@ -313,7 +306,6 @@ test('test processStore function to have only the client for the selected device deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); @@ -374,7 +366,6 @@ test('test processStore function to have multiple clients for the selected devic deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); @@ -441,7 +432,6 @@ test('test processStore function for device plugin state and no clients', async deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const json = await processStore({ @@ -479,7 +469,6 @@ test('test processStore function for unselected device plugin state and no clien deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const json = await processStore({ @@ -513,7 +502,6 @@ test('test processStore function for notifications for selected device', async ( deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const client = generateClientFromDevice(selectedDevice, 'testapp1'); @@ -563,7 +551,6 @@ test('test processStore function for notifications for unselected device', async deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const unselectedDevice = new ArchivedDevice({ @@ -571,7 +558,6 @@ test('test processStore function for notifications for unselected device', async deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); @@ -619,7 +605,6 @@ test('test processStore function for selected plugins', async () => { deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); @@ -670,7 +655,6 @@ test('test processStore function for no selected plugins', async () => { deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const client = generateClientFromDevice(selectedDevice, 'app'); @@ -1021,7 +1005,6 @@ test('test determinePluginsToProcess to ignore archived clients', async () => { deviceType: 'emulator', title: 'TestiPhone', os: 'iOS', - logEntries: [], screenshotHandle: null, }); const logger = { diff --git a/desktop/app/src/utils/exportData.tsx b/desktop/app/src/utils/exportData.tsx index 91023f4bb..3f0f66fe0 100644 --- a/desktop/app/src/utils/exportData.tsx +++ b/desktop/app/src/utils/exportData.tsx @@ -326,7 +326,6 @@ async function addSaltToDeviceSerial({ pluginStates, pluginNotification, statusUpdate, - selectedPlugins, pluginStates2, devicePluginStates, }: AddSaltToDeviceSerialOptions): Promise { @@ -337,11 +336,6 @@ async function addSaltToDeviceSerial({ deviceType: device.deviceType, title: device.title, os: device.os, - logEntries: selectedPlugins.includes('DeviceLogs') - ? device.getLogs( - new Date(new Date().getTime() - 1000 * 60 * 10), // Last 10 mins of logs - ) - : [], screenshotHandle: deviceScreenshot, }); statusUpdate && @@ -776,18 +770,13 @@ export function importDataToStore(source: string, data: string, store: Store) { if (device == null) { return; } - const {serial, deviceType, title, os, logs} = device; + const {serial, deviceType, title, os} = device; const archivedDevice = new ArchivedDevice({ serial, deviceType, title, os, - logEntries: logs - ? logs.map((l) => { - return {...l, date: new Date(l.date)}; - }) - : [], screenshotHandle: deviceScreenshot, source, supportRequestDetails, diff --git a/desktop/app/src/utils/reduxDevToolsConfig.tsx b/desktop/app/src/utils/reduxDevToolsConfig.tsx index 29d921ab9..0d8e446c1 100644 --- a/desktop/app/src/utils/reduxDevToolsConfig.tsx +++ b/desktop/app/src/utils/reduxDevToolsConfig.tsx @@ -19,16 +19,10 @@ export const stateSanitizer = (state: State) => { connections: { ...state.connections, devices: devices.map((device) => { - return { - ...device.toJSON(), - logs: [], - } as any; + return device.toJSON() as any; }), selectedDevice: selectedDevice - ? ({ - ...selectedDevice.toJSON(), - logs: [], - } as any) + ? (selectedDevice.toJSON() as any) : null, }, }; diff --git a/desktop/flipper-plugin/src/plugin/PluginBase.tsx b/desktop/flipper-plugin/src/plugin/PluginBase.tsx index cb8c0707e..e18741ff6 100644 --- a/desktop/flipper-plugin/src/plugin/PluginBase.tsx +++ b/desktop/flipper-plugin/src/plugin/PluginBase.tsx @@ -17,11 +17,11 @@ import {batched} from '../state/batch'; import {Idler} from '../utils/Idler'; import {message} from 'antd'; -type StateExportHandler = ( +type StateExportHandler = ( idler: Idler, onStatusMessage: (msg: string) => void, -) => Promise>; -type StateImportHandler = (data: Record) => void; +) => Promise; +type StateImportHandler = (data: T) => void; export interface BasePluginClient { readonly device: Device; @@ -50,13 +50,13 @@ export interface BasePluginClient { * Triggered when the current plugin is being exported and should create a snapshot of the state exported. * Overrides the default export behavior and ignores any 'persist' flags of state. */ - onExport(exporter: StateExportHandler): void; + onExport(exporter: StateExportHandler): void; /** * Triggered directly after the plugin instance was created, if the plugin is being restored from a snapshot. * Should be the inverse of the onExport handler */ - onImport(handler: StateImportHandler): void; + onImport(handler: StateImportHandler): void; /** * Register menu entries in the Flipper toolbar @@ -163,7 +163,7 @@ export abstract class BasePluginInstance { if (this.initialStates) { if (this.importHandler) { try { - this.importHandler(this.initialStates); + batched(this.importHandler)(this.initialStates); } catch (e) { const msg = `Error occurred when importing date for plugin '${this.definition.id}': '${e}`; console.error(msg, e); diff --git a/desktop/plugins/logs/__tests__/index.node.ts b/desktop/plugins/logs/__tests__/index.node.ts index 2fc4e6613..d93d1be7e 100644 --- a/desktop/plugins/logs/__tests__/index.node.ts +++ b/desktop/plugins/logs/__tests__/index.node.ts @@ -7,8 +7,10 @@ * @format */ +import {sleep, TestUtils} from 'flipper-plugin'; import {addEntriesToState, processEntry} from '../index'; import {DeviceLogEntry} from 'flipper'; +import * as LogsPlugin from '../index'; const entry: DeviceLogEntry = { tag: 'OpenGLRenderer', @@ -83,3 +85,70 @@ test('addEntriesToState with reversed direction (add to front)', () => { expect(newState.entries.length).toBe(2); expect(newState.rows.length).toBe(2); }); + +test('export / import plugin does work', async () => { + const { + instance, + exportStateAsync, + sendLogEntry, + } = TestUtils.startDevicePlugin(LogsPlugin); + + sendLogEntry({ + date: new Date(1611854112859), + message: 'test1', + pid: 0, + tag: 'test', + tid: 1, + type: 'error', + app: 'X', + }); + sendLogEntry({ + date: new Date(1611854117859), + message: 'test2', + pid: 2, + tag: 'test', + tid: 3, + type: 'warn', + app: 'Y', + }); + + // batching and storage is async atm + await sleep(500); + + const data = await exportStateAsync(); + expect(data).toMatchInlineSnapshot(` + Object { + "logs": Array [ + Object { + "app": "X", + "date": 1611854112859, + "message": "test1", + "pid": 0, + "tag": "test", + "tid": 1, + "type": "error", + }, + Object { + "app": "Y", + "date": 1611854117859, + "message": "test2", + "pid": 2, + "tag": "test", + "tid": 3, + "type": "warn", + }, + ], + } + `); + + expect(instance.rows.get().length).toBe(2); + + // Run a second import + { + const {exportStateAsync} = TestUtils.startDevicePlugin(LogsPlugin, { + initialState: data, + }); + + expect(await exportStateAsync()).toEqual(data); + } +}); diff --git a/desktop/plugins/logs/index.tsx b/desktop/plugins/logs/index.tsx index 6b99ac81e..c220effa6 100644 --- a/desktop/plugins/logs/index.tsx +++ b/desktop/plugins/logs/index.tsx @@ -320,6 +320,10 @@ export function supportsDevice(device: Device) { ); } +type ExportedState = { + logs: (Omit & {date: number})[]; +}; + export function devicePlugin(client: DevicePluginClient) { let counter = 0; let batch: Array<{ @@ -330,22 +334,41 @@ export function devicePlugin(client: DevicePluginClient) { let batchTimer: NodeJS.Timeout | undefined; const tableRef: MutableRefObject = createRef(); - // TODO T70688226: this can be removed once plugin stores logs, - // rather than the device. - - const initialState = addEntriesToState( - client.device.realDevice - .getLogs() - .map((log: DeviceLogEntry) => processEntry(log, '' + counter++)), - ); - - const rows = createState>(initialState.rows); + const rows = createState>([]); const entries = createState([]); const highlightedRows = createState>(new Set()); const counters = createState>(restoreSavedCounters()); const timeDirection = createState<'up' | 'down'>('up'); const isDeeplinked = createState(false); + client.onExport(async () => { + return { + logs: entries + .get() + .slice(-10000) + .map((e) => ({ + ...e.entry, + date: e.entry.date.getTime(), + })), + }; + }); + + client.onImport((data) => { + const imported = addEntriesToState( + data.logs.map((log) => + processEntry( + { + ...log, + date: new Date(log.date), + }, + '' + counter++, + ), + ), + ); + rows.set(imported.rows); + entries.set(imported.entries); + }); + client.onDeepLink((payload: unknown) => { if (typeof payload === 'string') { highlightedRows.set(calculateHighlightedRows(payload, rows.get())); @@ -485,10 +508,6 @@ export function devicePlugin(client: DevicePluginClient) { } function clearLogs() { - // TODO T70688226: implement this when the store is local - client.device.realDevice.clearLogs().catch((e: any) => { - console.error('Failed to clear logs: ', e); - }); entries.set([]); rows.set([]); highlightedRows.set(new Set());