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

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",

desktop/app/src/devices/AndroidDevice.tsx

@@ -86,7 +86,6 @@ export default class AndroidDevice extends BaseDevice {
   }
 
   clearLogs(): Promise<void> {
-    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,
     });
   }

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<DeviceLogEntry>;
     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<DeviceLogEntry>;
   archivedScreenshotHandle: string | null;
   isArchived = true;
 
@@ -40,15 +37,6 @@ export default class ArchivedDevice extends BaseDevice {
 
   supportRequestDetails?: SupportFormRequestDetailsState;
 
-  getLogs() {
-    return this.logs;
-  }
-
-  clearLogs(): Promise<void> {
-    this.logs = [];
-    return Promise.resolve();
-  }
-
   spawnShell(): DeviceShell | undefined | null {
     return null;
   }

desktop/app/src/devices/BaseDevice.tsx

@@ -33,7 +33,6 @@ export type DeviceExport = {
   title: string;
   deviceType: DeviceType;
   serial: string;
-  logs: Array<DeviceLogEntry>;
   pluginStates: Record<string, any>;
 };
 
@@ -71,7 +70,6 @@ export default class BaseDevice {
   icon: string | null | undefined;
 
   logListeners: Map<Symbol, DeviceLogListener> = new Map();
-  logEntries: Array<DeviceLogEntry> = [];
   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<void> {
-    // Only for device types that allow clearing.
-    this.logEntries = [];
-    return Promise.resolve();
-  }
-
   removeLogListener(id: Symbol) {
     this.logListeners.delete(id);
   }

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,
     });
   }

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 = {

desktop/app/src/utils/exportData.tsx

@@ -326,7 +326,6 @@ async function addSaltToDeviceSerial({
   pluginStates,
   pluginNotification,
   statusUpdate,
-  selectedPlugins,
   pluginStates2,
   devicePluginStates,
 }: AddSaltToDeviceSerialOptions): Promise<ExportType> {
@@ -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,

desktop/app/src/utils/reduxDevToolsConfig.tsx

@@ -19,16 +19,10 @@ export const stateSanitizer = (state: State) => {
       connections: {
         ...state.connections,
         devices: devices.map<DeviceExport>((device) => {
-          return {
-            ...device.toJSON(),
-            logs: [],
-          } as any;
+          return device.toJSON() as any;
         }),
         selectedDevice: selectedDevice
-          ? ({
-              ...selectedDevice.toJSON(),
-              logs: [],
-            } as any)
+          ? (selectedDevice.toJSON() as any)
           : null,
       },
     };

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<T = any> = (
   idler: Idler,
   onStatusMessage: (msg: string) => void,
-) => Promise<Record<string, any>>;
-type StateImportHandler = (data: Record<string, any>) => void;
+) => Promise<T>;
+type StateImportHandler<T = any> = (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<T = any>(exporter: StateExportHandler<T>): 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<T = any>(handler: StateImportHandler<T>): 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);

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);
+  }
+});

desktop/plugins/logs/index.tsx

@@ -320,6 +320,10 @@ export function supportsDevice(device: Device) {
   );
 }
 
+type ExportedState = {
+  logs: (Omit<DeviceLogEntry, 'date'> & {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<ManagedTableClass | null> = 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<ReadonlyArray<TableBodyRow>>(initialState.rows);
+  const rows = createState<ReadonlyArray<TableBodyRow>>([]);
   const entries = createState<Entries>([]);
   const highlightedRows = createState<ReadonlySet<string>>(new Set());
   const counters = createState<ReadonlyArray<Counter>>(restoreSavedCounters());
   const timeDirection = createState<'up' | 'down'>('up');
   const isDeeplinked = createState(false);
 
+  client.onExport<ExportedState>(async () => {
+    return {
+      logs: entries
+        .get()
+        .slice(-10000)
+        .map((e) => ({
+          ...e.entry,
+          date: e.entry.date.getTime(),
+        })),
+    };
+  });
+
+  client.onImport<ExportedState>((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());