From ad757b48e7d5bb59d7a5c9ed2f9d85c2ccdf5a7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Jamr=C3=B3z?= Date: Mon, 11 Oct 2021 09:48:25 +0200 Subject: [PATCH] Explore: Improve local storage error handling when rich history is added (#39943) * Rich History: improve local storage error handling * Reduce number of max items and update docs * Rotate not-starred items and add tests * Add missing property to initial state * Unify date in richHistory tests * Show a warning message that rich history limit has been reached * Add missing param --- public/app/core/store.ts | 4 +- public/app/core/utils/richHistory.test.ts | 384 ++++++++++-------- public/app/core/utils/richHistory.ts | 53 ++- .../app/features/explore/QueryRows.test.tsx | 2 + .../RichHistory/RichHistorySettings.tsx | 3 +- public/app/features/explore/state/main.ts | 18 + public/app/features/explore/state/query.ts | 20 +- public/app/types/explore.ts | 11 + 8 files changed, 318 insertions(+), 177 deletions(-) diff --git a/public/app/core/store.ts b/public/app/core/store.ts index c2e5af1a25f..d66444a0d6b 100644 --- a/public/app/core/store.ts +++ b/public/app/core/store.ts @@ -41,7 +41,9 @@ export class Store { this.set(key, json); } catch (error) { // Likely hitting storage quota - throw new Error(`Could not save item in localStorage: ${key}. [${error}]`); + const errorToThrow = new Error(`Could not save item in localStorage: ${key}. [${error}]`); + errorToThrow.name = error.name; + throw errorToThrow; } return true; } diff --git a/public/app/core/utils/richHistory.test.ts b/public/app/core/utils/richHistory.test.ts index 390b109e6af..80e95a3b663 100644 --- a/public/app/core/utils/richHistory.test.ts +++ b/public/app/core/utils/richHistory.test.ts @@ -9,9 +9,11 @@ import { deleteQueryInRichHistory, filterAndSortQueries, SortOrder, + MAX_HISTORY_ITEMS, } from './richHistory'; import store from 'app/core/store'; import { dateTime, DataQuery } from '@grafana/data'; +import { RichHistoryQuery } from '../../types'; const mock: any = { storedHistory: [ @@ -41,177 +43,235 @@ const mock: any = { const key = 'grafana.explore.richHistory'; -describe('addToRichHistory', () => { +describe('richHistory', () => { beforeEach(() => { - deleteAllFromRichHistory(); - expect(store.exists(key)).toBeFalsy(); + jest.useFakeTimers('modern'); + jest.setSystemTime(new Date(1970, 0, 1)); }); - const expectedResult = [ - { - comment: mock.testComment, - datasourceId: mock.testDatasourceId, - datasourceName: mock.testDatasourceName, - queries: mock.testQueries, - sessionName: mock.testSessionName, - starred: mock.testStarred, - ts: 2, - }, - mock.storedHistory[0], - ]; - - it('should append query to query history', () => { - Date.now = jest.fn(() => 2); - const newHistory = addToRichHistory( - mock.storedHistory, - mock.testDatasourceId, - mock.testDatasourceName, - mock.testQueries, - mock.testStarred, - mock.testComment, - mock.testSessionName - ); - expect(newHistory).toEqual(expectedResult); + afterEach(() => { + jest.useRealTimers(); }); - it('should save query history to localStorage', () => { - Date.now = jest.fn(() => 2); + describe('addToRichHistory', () => { + beforeEach(() => { + deleteAllFromRichHistory(); + expect(store.exists(key)).toBeFalsy(); + }); + const expectedResult = [ + { + comment: mock.testComment, + datasourceId: mock.testDatasourceId, + datasourceName: mock.testDatasourceName, + queries: mock.testQueries, + sessionName: mock.testSessionName, + starred: mock.testStarred, + ts: 2, + }, + mock.storedHistory[0], + ]; - addToRichHistory( - mock.storedHistory, - mock.testDatasourceId, - mock.testDatasourceName, - mock.testQueries, - mock.testStarred, - mock.testComment, - mock.testSessionName - ); - expect(store.exists(key)).toBeTruthy(); - expect(store.getObject(key)).toMatchObject(expectedResult); + it('should append query to query history', () => { + Date.now = jest.fn(() => 2); + const { richHistory: newHistory } = addToRichHistory( + mock.storedHistory, + mock.testDatasourceId, + mock.testDatasourceName, + mock.testQueries, + mock.testStarred, + mock.testComment, + mock.testSessionName, + true, + true + ); + expect(newHistory).toEqual(expectedResult); + }); + + it('should save query history to localStorage', () => { + Date.now = jest.fn(() => 2); + + addToRichHistory( + mock.storedHistory, + mock.testDatasourceId, + mock.testDatasourceName, + mock.testQueries, + mock.testStarred, + mock.testComment, + mock.testSessionName, + true, + true + ); + expect(store.exists(key)).toBeTruthy(); + expect(store.getObject(key)).toMatchObject(expectedResult); + }); + + it('should not append duplicated query to query history', () => { + Date.now = jest.fn(() => 2); + const { richHistory: newHistory } = addToRichHistory( + mock.storedHistory, + mock.storedHistory[0].datasourceId, + mock.storedHistory[0].datasourceName, + [{ expr: 'query1', maxLines: null, refId: 'A' } as DataQuery, { expr: 'query2', refId: 'B' } as DataQuery], + mock.testStarred, + mock.testComment, + mock.testSessionName, + true, + true + ); + expect(newHistory).toEqual([mock.storedHistory[0]]); + }); + + it('should not save duplicated query to localStorage', () => { + Date.now = jest.fn(() => 2); + addToRichHistory( + mock.storedHistory, + mock.storedHistory[0].datasourceId, + mock.storedHistory[0].datasourceName, + [{ expr: 'query1', maxLines: null, refId: 'A' } as DataQuery, { expr: 'query2', refId: 'B' } as DataQuery], + mock.testStarred, + mock.testComment, + mock.testSessionName, + true, + true + ); + expect(store.exists(key)).toBeFalsy(); + }); + + it('should not save more than MAX_HISTORY_ITEMS', () => { + Date.now = jest.fn(() => 2); + const extraItems = 100; + + // the history has more than MAX + let history = []; + // history = [ { starred: true, comment: "0" }, { starred: false, comment: "1" }, ... ] + for (let i = 0; i < MAX_HISTORY_ITEMS + extraItems; i++) { + history.push({ + starred: i % 2 === 0, + comment: i.toString(), + queries: [], + ts: new Date(2019, 11, 31).getTime(), + }); + } + + const starredItemsInHistory = (MAX_HISTORY_ITEMS + extraItems) / 2; + const notStarredItemsInHistory = (MAX_HISTORY_ITEMS + extraItems) / 2; + + expect(history.filter((h) => h.starred)).toHaveLength(starredItemsInHistory); + expect(history.filter((h) => !h.starred)).toHaveLength(notStarredItemsInHistory); + + const { richHistory: newHistory } = addToRichHistory( + (history as any) as RichHistoryQuery[], + mock.storedHistory[0].datasourceId, + mock.storedHistory[0].datasourceName, + [{ expr: 'query1', maxLines: null, refId: 'A' } as DataQuery, { expr: 'query2', refId: 'B' } as DataQuery], + true, + mock.testComment, + mock.testSessionName, + true, + true + ); + + // one not starred replaced with a newly added starred item + const removedNotStarredItems = extraItems + 1; // + 1 to make space for the new item + expect(newHistory.filter((h) => h.starred)).toHaveLength(starredItemsInHistory + 1); // starred item added + expect(newHistory.filter((h) => !h.starred)).toHaveLength(starredItemsInHistory - removedNotStarredItems); + }); }); - it('should not append duplicated query to query history', () => { - Date.now = jest.fn(() => 2); - const newHistory = addToRichHistory( - mock.storedHistory, - mock.storedHistory[0].datasourceId, - mock.storedHistory[0].datasourceName, - [{ expr: 'query1', maxLines: null, refId: 'A' } as DataQuery, { expr: 'query2', refId: 'B' } as DataQuery], - mock.testStarred, - mock.testComment, - mock.testSessionName - ); - expect(newHistory).toEqual([mock.storedHistory[0]]); + describe('updateStarredInRichHistory', () => { + it('should update starred in query in history', () => { + const updatedStarred = updateStarredInRichHistory(mock.storedHistory, 1); + expect(updatedStarred[0].starred).toEqual(false); + }); + it('should update starred in localStorage', () => { + updateStarredInRichHistory(mock.storedHistory, 1); + expect(store.exists(key)).toBeTruthy(); + expect(store.getObject(key)[0].starred).toEqual(false); + }); }); - it('should not save duplicated query to localStorage', () => { - Date.now = jest.fn(() => 2); - addToRichHistory( - mock.storedHistory, - mock.storedHistory[0].datasourceId, - mock.storedHistory[0].datasourceName, - [{ expr: 'query1', maxLines: null, refId: 'A' } as DataQuery, { expr: 'query2', refId: 'B' } as DataQuery], - mock.testStarred, - mock.testComment, - mock.testSessionName - ); - expect(store.exists(key)).toBeFalsy(); - }); -}); - -describe('updateStarredInRichHistory', () => { - it('should update starred in query in history', () => { - const updatedStarred = updateStarredInRichHistory(mock.storedHistory, 1); - expect(updatedStarred[0].starred).toEqual(false); - }); - it('should update starred in localStorage', () => { - updateStarredInRichHistory(mock.storedHistory, 1); - expect(store.exists(key)).toBeTruthy(); - expect(store.getObject(key)[0].starred).toEqual(false); - }); -}); - -describe('updateCommentInRichHistory', () => { - it('should update comment in query in history', () => { - const updatedComment = updateCommentInRichHistory(mock.storedHistory, 1, 'new comment'); - expect(updatedComment[0].comment).toEqual('new comment'); - }); - it('should update comment in localStorage', () => { - updateCommentInRichHistory(mock.storedHistory, 1, 'new comment'); - expect(store.exists(key)).toBeTruthy(); - expect(store.getObject(key)[0].comment).toEqual('new comment'); - }); -}); - -describe('deleteQueryInRichHistory', () => { - it('should delete query in query in history', () => { - const deletedHistory = deleteQueryInRichHistory(mock.storedHistory, 1); - expect(deletedHistory).toEqual([]); - }); - it('should delete query in localStorage', () => { - deleteQueryInRichHistory(mock.storedHistory, 1); - expect(store.exists(key)).toBeTruthy(); - expect(store.getObject(key)).toEqual([]); - }); -}); - -describe('mapNumbertoTimeInSlider', () => { - it('should correctly map number to value', () => { - const value = mapNumbertoTimeInSlider(25); - expect(value).toEqual('25 days ago'); - }); -}); - -describe('createDateStringFromTs', () => { - it('should correctly create string value from timestamp', () => { - const value = createDateStringFromTs(1583932327000); - expect(value).toEqual('March 11'); - }); -}); - -describe('filterQueries', () => { - it('should filter out queries based on data source filter', () => { - const filteredQueries = filterAndSortQueries( - mock.storedHistory, - SortOrder.Ascending, - ['not provided data source'], - '' - ); - expect(filteredQueries).toHaveLength(0); - }); - it('should keep queries based on data source filter', () => { - const filteredQueries = filterAndSortQueries( - mock.storedHistory, - SortOrder.Ascending, - ['datasource history name'], - '' - ); - expect(filteredQueries).toHaveLength(1); - }); - it('should filter out all queries based on search filter', () => { - const filteredQueries = filterAndSortQueries( - mock.storedHistory, - SortOrder.Ascending, - [], - 'i do not exist in query' - ); - expect(filteredQueries).toHaveLength(0); - }); - it('should include queries based on search filter', () => { - const filteredQueries = filterAndSortQueries(mock.storedHistory, SortOrder.Ascending, [], 'query1'); - expect(filteredQueries).toHaveLength(1); - }); -}); - -describe('createQueryHeading', () => { - it('should correctly create heading for queries when sort order is ascending ', () => { - // Have to offset the timezone of a 1 microsecond epoch, and then reverse the changes - mock.storedHistory[0].ts = 1 + -1 * dateTime().utcOffset() * 60 * 1000; - const heading = createQueryHeading(mock.storedHistory[0], SortOrder.Ascending); - expect(heading).toEqual('January 1'); - }); - it('should correctly create heading for queries when sort order is datasourceAZ ', () => { - const heading = createQueryHeading(mock.storedHistory[0], SortOrder.DatasourceAZ); - expect(heading).toEqual(mock.storedHistory[0].datasourceName); + describe('updateCommentInRichHistory', () => { + it('should update comment in query in history', () => { + const updatedComment = updateCommentInRichHistory(mock.storedHistory, 1, 'new comment'); + expect(updatedComment[0].comment).toEqual('new comment'); + }); + it('should update comment in localStorage', () => { + updateCommentInRichHistory(mock.storedHistory, 1, 'new comment'); + expect(store.exists(key)).toBeTruthy(); + expect(store.getObject(key)[0].comment).toEqual('new comment'); + }); + }); + + describe('deleteQueryInRichHistory', () => { + it('should delete query in query in history', () => { + const deletedHistory = deleteQueryInRichHistory(mock.storedHistory, 1); + expect(deletedHistory).toEqual([]); + }); + it('should delete query in localStorage', () => { + deleteQueryInRichHistory(mock.storedHistory, 1); + expect(store.exists(key)).toBeTruthy(); + expect(store.getObject(key)).toEqual([]); + }); + }); + + describe('mapNumbertoTimeInSlider', () => { + it('should correctly map number to value', () => { + const value = mapNumbertoTimeInSlider(25); + expect(value).toEqual('25 days ago'); + }); + }); + + describe('createDateStringFromTs', () => { + it('should correctly create string value from timestamp', () => { + const value = createDateStringFromTs(1583932327000); + expect(value).toEqual('March 11'); + }); + }); + + describe('filterQueries', () => { + it('should filter out queries based on data source filter', () => { + const filteredQueries = filterAndSortQueries( + mock.storedHistory, + SortOrder.Ascending, + ['not provided data source'], + '' + ); + expect(filteredQueries).toHaveLength(0); + }); + it('should keep queries based on data source filter', () => { + const filteredQueries = filterAndSortQueries( + mock.storedHistory, + SortOrder.Ascending, + ['datasource history name'], + '' + ); + expect(filteredQueries).toHaveLength(1); + }); + it('should filter out all queries based on search filter', () => { + const filteredQueries = filterAndSortQueries( + mock.storedHistory, + SortOrder.Ascending, + [], + 'i do not exist in query' + ); + expect(filteredQueries).toHaveLength(0); + }); + it('should include queries based on search filter', () => { + const filteredQueries = filterAndSortQueries(mock.storedHistory, SortOrder.Ascending, [], 'query1'); + expect(filteredQueries).toHaveLength(1); + }); + }); + + describe('createQueryHeading', () => { + it('should correctly create heading for queries when sort order is ascending ', () => { + // Have to offset the timezone of a 1 microsecond epoch, and then reverse the changes + mock.storedHistory[0].ts = 1 + -1 * dateTime().utcOffset() * 60 * 1000; + const heading = createQueryHeading(mock.storedHistory[0], SortOrder.Ascending); + expect(heading).toEqual('January 1'); + }); + it('should correctly create heading for queries when sort order is datasourceAZ ', () => { + const heading = createQueryHeading(mock.storedHistory[0], SortOrder.DatasourceAZ); + expect(heading).toEqual(mock.storedHistory[0].datasourceName); + }); }); }); diff --git a/public/app/core/utils/richHistory.ts b/public/app/core/utils/richHistory.ts index fd3a60a33b3..1ab0dd0604c 100644 --- a/public/app/core/utils/richHistory.ts +++ b/public/app/core/utils/richHistory.ts @@ -6,7 +6,7 @@ import { DataQuery, DataSourceApi, dateTimeFormat, urlUtil, ExploreUrlState } fr import store from 'app/core/store'; import { dispatch } from 'app/store/store'; import { notifyApp } from 'app/core/actions'; -import { createErrorNotification } from 'app/core/copy/appNotification'; +import { createErrorNotification, createWarningNotification } from 'app/core/copy/appNotification'; // Types import { RichHistoryQuery } from 'app/types/explore'; @@ -34,6 +34,8 @@ export enum SortOrder { * Side-effect: store history in local storage */ +export const MAX_HISTORY_ITEMS = 10000; + export function addToRichHistory( richHistory: RichHistoryQuery[], datasourceId: string, @@ -41,8 +43,10 @@ export function addToRichHistory( queries: DataQuery[], starred: boolean, comment: string | null, - sessionName: string -): any { + sessionName: string, + showQuotaExceededError: boolean, + showLimitExceededWarning: boolean +): { richHistory: RichHistoryQuery[]; localStorageFull?: boolean; limitExceeded?: boolean } { const ts = Date.now(); /* Save only queries, that are not falsy (e.g. empty object, null, ...) */ const newQueriesToSave: DataQuery[] = queries && queries.filter((query) => notEmptyQuery(query)); @@ -66,24 +70,53 @@ export function addToRichHistory( }); if (isEqual(newQueriesToCompare, lastQueriesToCompare)) { - return richHistory; + return { richHistory }; } - let updatedHistory = [ - { queries: newQueriesToSave, ts, datasourceId, datasourceName, starred, comment, sessionName }, + // remove oldest non-starred items to give space for the recent query + let limitExceeded = false; + let current = queriesToKeep.length - 1; + while (current >= 0 && queriesToKeep.length >= MAX_HISTORY_ITEMS) { + if (!queriesToKeep[current].starred) { + queriesToKeep.splice(current, 1); + limitExceeded = true; + } + current--; + } + + let updatedHistory: RichHistoryQuery[] = [ + { + queries: newQueriesToSave, + ts, + datasourceId, + datasourceName: datasourceName ?? '', + starred, + comment: comment ?? '', + sessionName, + }, ...queriesToKeep, ]; try { + showLimitExceededWarning && + limitExceeded && + dispatch( + notifyApp( + createWarningNotification( + `Query history reached the limit of ${MAX_HISTORY_ITEMS}. Old, not-starred items will be removed.` + ) + ) + ); store.setObject(RICH_HISTORY_KEY, updatedHistory); - return updatedHistory; + return { richHistory: updatedHistory, limitExceeded, localStorageFull: false }; } catch (error) { - dispatch(notifyApp(createErrorNotification('Saving rich history failed', error.message))); - return richHistory; + showQuotaExceededError && + dispatch(notifyApp(createErrorNotification('Saving rich history failed', error.message))); + return { richHistory: updatedHistory, limitExceeded, localStorageFull: error.name === 'QuotaExceededError' }; } } - return richHistory; + return { richHistory }; } export function getRichHistory(): RichHistoryQuery[] { diff --git a/public/app/features/explore/QueryRows.test.tsx b/public/app/features/explore/QueryRows.test.tsx index 0b4c5d896df..a447ff1215f 100644 --- a/public/app/features/explore/QueryRows.test.tsx +++ b/public/app/features/explore/QueryRows.test.tsx @@ -49,6 +49,8 @@ function setup(queries: DataQuery[]) { right: undefined, richHistory: [], autoLoadLogsVolume: false, + localStorageFull: false, + richHistoryLimitExceededWarningShown: false, }; const store = configureStore({ explore: initialState, user: { orgId: 1 } as UserState }); diff --git a/public/app/features/explore/RichHistory/RichHistorySettings.tsx b/public/app/features/explore/RichHistory/RichHistorySettings.tsx index be82348ade9..27cdd934a8f 100644 --- a/public/app/features/explore/RichHistory/RichHistorySettings.tsx +++ b/public/app/features/explore/RichHistory/RichHistorySettings.tsx @@ -7,6 +7,7 @@ import { ShowConfirmModalEvent } from '../../../types/events'; import { dispatch } from 'app/store/store'; import { notifyApp } from 'app/core/actions'; import { createSuccessNotification } from 'app/core/copy/appNotification'; +import { MAX_HISTORY_ITEMS } from '../../../core/utils/richHistory'; export interface RichHistorySettingsProps { retentionPeriod: number; @@ -80,7 +81,7 @@ export function RichHistorySettings(props: RichHistorySettingsProps) {
diff --git a/public/app/features/explore/state/main.ts b/public/app/features/explore/state/main.ts index 2de1e049379..3481737ce80 100644 --- a/public/app/features/explore/state/main.ts +++ b/public/app/features/explore/state/main.ts @@ -21,6 +21,8 @@ export interface SyncTimesPayload { export const syncTimesAction = createAction('explore/syncTimes'); export const richHistoryUpdatedAction = createAction('explore/richHistoryUpdated'); +export const localStorageFullAction = createAction('explore/localStorageFullAction'); +export const richHistoryLimitExceededAction = createAction('explore/richHistoryLimitExceededAction'); /** * Stores new value of auto-load logs volume switch. Used only internally. changeAutoLogsVolume() is used to @@ -172,6 +174,8 @@ export const initialExploreState: ExploreState = { left: initialExploreItemState, right: undefined, richHistory: [], + localStorageFull: false, + richHistoryLimitExceededWarningShown: false, autoLoadLogsVolume: store.getBool(AUTO_LOAD_LOGS_VOLUME_SETTING_KEY, false), }; @@ -227,6 +231,20 @@ export const exploreReducer = (state = initialExploreState, action: AnyAction): }; } + if (localStorageFullAction.match(action)) { + return { + ...state, + localStorageFull: true, + }; + } + + if (richHistoryLimitExceededAction.match(action)) { + return { + ...state, + richHistoryLimitExceededWarningShown: true, + }; + } + if (storeAutoLoadLogsVolumeAction.match(action)) { const autoLoadLogsVolume = action.payload; return { diff --git a/public/app/features/explore/state/query.ts b/public/app/features/explore/state/query.ts index 3dd4fca9e4d..18c8d035b40 100644 --- a/public/app/features/explore/state/query.ts +++ b/public/app/features/explore/state/query.ts @@ -32,7 +32,13 @@ import { notifyApp } from '../../../core/actions'; import { runRequest } from '../../query/state/runRequest'; import { decorateData } from '../utils/decorators'; import { createErrorNotification } from '../../../core/copy/appNotification'; -import { richHistoryUpdatedAction, stateSave, storeAutoLoadLogsVolumeAction } from './main'; +import { + localStorageFullAction, + richHistoryLimitExceededAction, + richHistoryUpdatedAction, + stateSave, + storeAutoLoadLogsVolumeAction, +} from './main'; import { AnyAction, createAction, PayloadAction } from '@reduxjs/toolkit'; import { updateTime } from './time'; import { historyUpdatedAction } from './history'; @@ -415,17 +421,25 @@ export const runQueries = ( if (!data.error && firstResponse) { // Side-effect: Saving history in localstorage const nextHistory = updateHistory(history, datasourceId, queries); - const nextRichHistory = addToRichHistory( + const { richHistory: nextRichHistory, localStorageFull, limitExceeded } = addToRichHistory( richHistory || [], datasourceId, datasourceName, queries, false, '', - '' + '', + !getState().explore.localStorageFull, + !getState().explore.richHistoryLimitExceededWarningShown ); dispatch(historyUpdatedAction({ exploreId, history: nextHistory })); dispatch(richHistoryUpdatedAction({ richHistory: nextRichHistory })); + if (localStorageFull) { + dispatch(localStorageFullAction()); + } + if (limitExceeded) { + dispatch(richHistoryLimitExceededAction()); + } // We save queries to the URL here so that only successfully run queries change the URL. dispatch(stateSave({ replace: options?.replaceUrl })); diff --git a/public/app/types/explore.ts b/public/app/types/explore.ts index 14e462f648b..cb54e1f4cc9 100644 --- a/public/app/types/explore.ts +++ b/public/app/types/explore.ts @@ -46,6 +46,17 @@ export interface ExploreState { */ richHistory: RichHistoryQuery[]; + /** + * True if local storage quota was exceeded when a new item was added. This is to prevent showing + * multiple errors when local storage is full. + */ + localStorageFull: boolean; + + /** + * True if a warning message of hitting the exceeded number of items has been shown already. + */ + richHistoryLimitExceededWarningShown: boolean; + /** * Auto-loading logs volume after running the query */