From 4e3c06cd54703b804d65132a75a250aae2532bb5 Mon Sep 17 00:00:00 2001 From: Pritesh Nandgaonkar Date: Tue, 31 Mar 2020 10:59:38 -0700 Subject: [PATCH] Update the text to be clear that the export was successfull Summary: This diff adds the capability to show the plugin names for which flipper failed to fetch metadata. These changes are done for both export as URL and export as File. This diff also fixes the logging for the export as a file and logs the result object in scuba. Reviewed By: jknoxville Differential Revision: D20724860 fbshipit-source-id: 4c9591267ca05045e0ed084804d96851c9d7636d --- .../app/src/chrome/ShareSheetErrorList.tsx | 8 +- .../app/src/chrome/ShareSheetExportFile.tsx | 34 ++++- .../app/src/chrome/ShareSheetExportUrl.tsx | 28 +++- desktop/app/src/utils/exportData.tsx | 129 +++++++++++------- desktop/app/src/utils/exportMetrics.tsx | 6 +- desktop/headless/index.tsx | 6 +- 6 files changed, 135 insertions(+), 76 deletions(-) diff --git a/desktop/app/src/chrome/ShareSheetErrorList.tsx b/desktop/app/src/chrome/ShareSheetErrorList.tsx index fe47efcc6..067499847 100644 --- a/desktop/app/src/chrome/ShareSheetErrorList.tsx +++ b/desktop/app/src/chrome/ShareSheetErrorList.tsx @@ -12,6 +12,8 @@ import {Text, styled, Info, VBox} from '../ui'; type Props = { errors: Array; + title: string; + type: 'info' | 'spinning' | 'warning' | 'error'; }; const ErrorMessage = styled(Text)({ @@ -47,10 +49,8 @@ export default class Popover extends PureComponent { } return ( - - - The following errors occurred while exporting your data - + + {this.props.title} {this.props.errors.map((e: Error) => ( {formatError(e)} ))} diff --git a/desktop/app/src/chrome/ShareSheetExportFile.tsx b/desktop/app/src/chrome/ShareSheetExportFile.tsx index 9ae847965..15e362cbe 100644 --- a/desktop/app/src/chrome/ShareSheetExportFile.tsx +++ b/desktop/app/src/chrome/ShareSheetExportFile.tsx @@ -18,6 +18,7 @@ import {Idler} from '../utils/Idler'; import { exportStoreToFile, EXPORT_FLIPPER_TRACE_EVENT, + displayFetchMetadataErrors, } from '../utils/exportData'; import ShareSheetErrorList from './ShareSheetErrorList'; import ShareSheetPendingDialog from './ShareSheetPendingDialog'; @@ -53,7 +54,9 @@ type Props = { }; type State = { - errorArray: Array; + fetchMetaDataErrors: { + [plugin: string]: Error; + } | null; result: | { kind: 'success'; @@ -77,7 +80,7 @@ export default class ShareSheetExportFile extends Component { } state: State = { - errorArray: [], + fetchMetaDataErrors: null, result: {kind: 'pending'}, statusUpdate: null, runInBackground: false, @@ -106,7 +109,7 @@ export default class ShareSheetExportFile extends Component { if (!this.props.file) { return; } - const {errorArray} = await reportPlatformFailures( + const {fetchMetaDataErrors} = await reportPlatformFailures( exportStoreToFile( this.props.file, this.store, @@ -130,18 +133,31 @@ export default class ShareSheetExportFile extends Component { }); return; } - this.setState({errorArray, result: {kind: 'success'}}); + this.setState({fetchMetaDataErrors, result: {kind: 'success'}}); this.props.logger.trackTimeSince(mark, 'export:file-success'); } catch (err) { + const result: { + kind: 'error'; + error: Error; + } = { + kind: 'error', + error: err, + }; if (!this.state.runInBackground) { - this.setState({errorArray: [], result: {kind: 'error', error: err}}); + // Show the error in UI. + this.setState({result}); } - this.props.logger.trackTimeSince(mark, 'export:file-error'); + this.store.dispatch(unsetShare()); + this.props.logger.trackTimeSince(mark, 'export:file-error', result); throw err; } } renderSuccess() { + const {title, errorArray} = displayFetchMetadataErrors( + this.state.fetchMetaDataErrors, + ); + return ( {({store}) => ( @@ -153,7 +169,11 @@ export default class ShareSheetExportFile extends Component { might contain sensitive information like access tokens used in network requests. - + diff --git a/desktop/app/src/chrome/ShareSheetExportUrl.tsx b/desktop/app/src/chrome/ShareSheetExportUrl.tsx index ebf1b8c44..1c25e6807 100644 --- a/desktop/app/src/chrome/ShareSheetExportUrl.tsx +++ b/desktop/app/src/chrome/ShareSheetExportUrl.tsx @@ -30,7 +30,11 @@ import { DataExportResult, DataExportError, } from '../fb-stubs/user'; -import {exportStore, EXPORT_FLIPPER_TRACE_EVENT} from '../utils/exportData'; +import { + exportStore, + EXPORT_FLIPPER_TRACE_EVENT, + displayFetchMetadataErrors, +} from '../utils/exportData'; import {clipboard} from 'electron'; import ShareSheetErrorList from './ShareSheetErrorList'; import {reportPlatformFailures} from '../utils/metrics'; @@ -78,7 +82,9 @@ type Props = { type State = { runInBackground: boolean; - errorArray: Array; + fetchMetaDataErrors: { + [plugin: string]: Error; + } | null; result: DataExportError | DataExportResult | null; statusUpdate: string | null; }; @@ -87,7 +93,7 @@ export default class ShareSheetExportUrl extends Component { static contextType = ReactReduxContext; state: State = { - errorArray: [], + fetchMetaDataErrors: null, result: null, statusUpdate: null, runInBackground: false, @@ -124,7 +130,10 @@ export default class ShareSheetExportUrl extends Component { this.setState({statusUpdate: msg}); } }; - const {serializedString, errorArray} = await reportPlatformFailures( + const { + serializedString, + fetchMetaDataErrors, + } = await reportPlatformFailures( exportStore(this.store, false, this.idler, statusUpdate), `${EXPORT_FLIPPER_TRACE_EVENT}:UI_LINK`, ); @@ -145,7 +154,7 @@ export default class ShareSheetExportUrl extends Component { getLogger().trackTimeSince(uploadMarker, uploadMarker, { plugins: this.store.getState().plugins.selectedPlugins, }); - this.setState({errorArray, result}); + this.setState({fetchMetaDataErrors, result}); const flipperUrl = (result as DataExportResult).flipperUrl; if (flipperUrl) { clipboard.writeText(String(flipperUrl)); @@ -223,11 +232,12 @@ export default class ShareSheetExportUrl extends Component { } render() { - const {result, statusUpdate, errorArray} = this.state; + const {result, statusUpdate, fetchMetaDataErrors} = this.state; if (!result) { return this.renderPending(statusUpdate); } + const {title, errorArray} = displayFetchMetadataErrors(fetchMetaDataErrors); return ( {({store}) => ( @@ -251,7 +261,11 @@ export default class ShareSheetExportUrl extends Component { data might contain sensitve information like access tokens used in network requests. - + ) : ( <> diff --git a/desktop/app/src/utils/exportData.tsx b/desktop/app/src/utils/exportData.tsx index fe9fcd1f0..7c42371c7 100644 --- a/desktop/app/src/utils/exportData.tsx +++ b/desktop/app/src/utils/exportData.tsx @@ -108,6 +108,26 @@ type AddSaltToDeviceSerialOptions = { statusUpdate?: (msg: string) => void; }; +export function displayFetchMetadataErrors( + fetchMetaDataErrors: { + [plugin: string]: Error; + } | null, +): {title: string; errorArray: Array} { + const errors = fetchMetaDataErrors ? Object.values(fetchMetaDataErrors) : []; + const pluginsWithFetchMetadataErrors = fetchMetaDataErrors + ? Object.keys(fetchMetaDataErrors) + : []; + const title = + fetchMetaDataErrors && pluginsWithFetchMetadataErrors.length > 0 + ? `Export was successfull, but plugin${ + pluginsWithFetchMetadataErrors.length > 1 ? 's' : '' + } ${pluginsWithFetchMetadataErrors.join( + ', ', + )} might be ignored because of the following errors.` + : ''; + return {title, errorArray: errors}; +} + export function processClients( clients: Array, serial: string, @@ -344,7 +364,7 @@ type ProcessStoreOptions = { export const processStore = async ( options: ProcessStoreOptions, idler?: Idler, -): Promise => { +): Promise => { const { activeNotifications, device, @@ -410,7 +430,7 @@ export const processStore = async ( return exportFlipperData; } - return null; + throw new Error('Selected device is null, please select a device'); }; export async function fetchMetadata( @@ -419,9 +439,12 @@ export async function fetchMetadata( state: ReduxState, statusUpdate?: (msg: string) => void, idler?: Idler, -): Promise<{pluginStates: PluginStatesState; errorArray: Array}> { +): Promise<{ + pluginStates: PluginStatesState; + errors: {[plugin: string]: Error} | null; +}> { const newPluginState = {...pluginStates}; - const errorArray: Array = []; + let errorObject: {[plugin: string]: Error} | null = null; for (const { pluginName, @@ -448,12 +471,20 @@ export async function fetchMetadata( ), `Timed out while collecting data for ${pluginName}`, ); + if (!data) { + throw new Error( + `Metadata returned by the ${pluginName} is undefined`, + ); + } getLogger().trackTimeSince(fetchMetaDataMarker, fetchMetaDataMarker, { pluginId, }); newPluginState[pluginKey] = data; } catch (e) { - errorArray.push(e); + if (!errorObject) { + errorObject = {}; + } + errorObject[pluginName] = e; getLogger().trackTimeSince(fetchMetaDataMarker, fetchMetaDataMarker, { pluginId, error: e, @@ -463,7 +494,7 @@ export async function fetchMetadata( } } - return {pluginStates: newPluginState, errorArray}; + return {pluginStates: newPluginState, errors: errorObject}; } async function processQueues( @@ -550,7 +581,10 @@ export async function getStoreExport( store: MiddlewareAPI, statusUpdate?: (msg: string) => void, idler?: Idler, -): Promise<{exportData: ExportType | null; errorArray: Array}> { +): Promise<{ + exportData: ExportType; + fetchMetaDataErrors: {[plugin: string]: Error} | null; +}> { const state = store.getState(); const {clients, selectedApp, selectedDevice} = state.connections; const pluginsToProcess = determinePluginsToProcess( @@ -578,7 +612,7 @@ export async function getStoreExport( getLogger().trackTimeSince(fetchMetaDataMarker, fetchMetaDataMarker, { plugins: state.plugins.selectedPlugins, }); - const {errorArray} = metadata; + const {errors} = metadata; const newPluginState = metadata.pluginStates; const {activeNotifications} = state.notifications; @@ -597,7 +631,7 @@ export async function getStoreExport( }, idler, ); - return {exportData, errorArray}; + return {exportData, fetchMetaDataErrors: errors}; } export async function exportStore( @@ -607,58 +641,43 @@ export async function exportStore( statusUpdate?: (msg: string) => void, ): Promise<{ serializedString: string; - errorArray: Array; - exportStoreData: ExportType | null; + fetchMetaDataErrors: { + [plugin: string]: Error; + } | null; + exportStoreData: ExportType; }> { getLogger().track('usage', EXPORT_FLIPPER_TRACE_EVENT); performance.mark(EXPORT_FLIPPER_TRACE_TIME_SERIALIZATION_EVENT); statusUpdate && statusUpdate('Preparing to export Flipper data...'); const state = store.getState(); - const {exportData, errorArray} = await getStoreExport( + const {exportData, fetchMetaDataErrors} = await getStoreExport( store, statusUpdate, idler, ); - if (exportData != null) { - if (includeSupportDetails) { - exportData.supportRequestDetails = { - ...state.supportForm?.supportFormV2, - appName: - state.connections.selectedApp == null - ? '' - : deconstructClientId(state.connections.selectedApp).app, - }; - } - - statusUpdate && statusUpdate('Serializing Flipper data...'); - const serializedString = JSON.stringify(exportData); - if (serializedString.length <= 0) { - throw new Error('Serialize function returned empty string'); - } - getLogger().trackTimeSince( - EXPORT_FLIPPER_TRACE_TIME_SERIALIZATION_EVENT, - EXPORT_FLIPPER_TRACE_TIME_SERIALIZATION_EVENT, - { - plugins: state.plugins.selectedPlugins, - }, - ); - if (errorArray.length > 0) { - const errorStr = errorArray.join(', '); - logPlatformSuccessRate('export-store-task', { - kind: 'failure', - supportedOperation: true, - error: errorStr, - }); - console.error('Export Store Task Failures: ', errorStr); - } - return {serializedString, errorArray, exportStoreData: exportData}; - } else { - return { - serializedString: '{}', - errorArray: [], - exportStoreData: exportData, + if (includeSupportDetails) { + exportData.supportRequestDetails = { + ...state.supportForm?.supportFormV2, + appName: + state.connections.selectedApp == null + ? '' + : deconstructClientId(state.connections.selectedApp).app, }; } + + statusUpdate && statusUpdate('Serializing Flipper data...'); + const serializedString = JSON.stringify(exportData); + if (serializedString.length <= 0) { + throw new Error('Serialize function returned empty string'); + } + getLogger().trackTimeSince( + EXPORT_FLIPPER_TRACE_TIME_SERIALIZATION_EVENT, + EXPORT_FLIPPER_TRACE_TIME_SERIALIZATION_EVENT, + { + plugins: state.plugins.selectedPlugins, + }, + ); + return {serializedString, fetchMetaDataErrors, exportStoreData: exportData}; } export const exportStoreToFile = ( @@ -667,13 +686,17 @@ export const exportStoreToFile = ( includeSupportDetails: boolean, idler?: Idler, statusUpdate?: (msg: string) => void, -): Promise<{errorArray: Array}> => { +): Promise<{ + fetchMetaDataErrors: { + [plugin: string]: Error; + } | null; +}> => { return exportStore(store, includeSupportDetails, idler, statusUpdate).then( - ({serializedString, errorArray}) => { + ({serializedString, fetchMetaDataErrors}) => { return promisify(fs.writeFile)(exportFilePath, serializedString).then( () => { store.dispatch(resetSupportFormV2State()); - return {errorArray}; + return {fetchMetaDataErrors}; }, ); }, diff --git a/desktop/app/src/utils/exportMetrics.tsx b/desktop/app/src/utils/exportMetrics.tsx index e8b0ba2c7..8a5a30ad8 100644 --- a/desktop/app/src/utils/exportMetrics.tsx +++ b/desktop/app/src/utils/exportMetrics.tsx @@ -83,9 +83,9 @@ export async function exportMetricsWithoutTrace( store.getState(), ); const newPluginStates = metadata.pluginStates; - const {errorArray} = metadata; - if (errorArray.length > 0) { - console.error(errorArray); + const {errors} = metadata; + if (errors) { + console.error(errors); } const metrics = await exportMetrics( diff --git a/desktop/headless/index.tsx b/desktop/headless/index.tsx index b6dc307e2..1424b2357 100644 --- a/desktop/headless/index.tsx +++ b/desktop/headless/index.tsx @@ -182,8 +182,10 @@ async function exitActions( ); outputAndExit(payload); } else { - const {serializedString, errorArray} = await exportStore(store); - errorArray.forEach(console.error); + const {serializedString, fetchMetaDataErrors} = await exportStore( + store, + ); + console.error('Error while fetching metadata', fetchMetaDataErrors); outputAndExit(serializedString); } } catch (e) {