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

@@ -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;
   }

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);
-
-    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);
-  });
+  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],
+    ];
 
-  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]]);
-  });
+    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 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();
-  });
-});
+    it('should save query history to localStorage', () => {
+      Date.now = jest.fn(() => 2);
 
-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);
-  });
-});
+      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);
+    });
 
-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');
-  });
-});
+    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]]);
+    });
 
-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([]);
-  });
-});
+    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;
 
-describe('mapNumbertoTimeInSlider', () => {
-  it('should correctly map number to value', () => {
-    const value = mapNumbertoTimeInSlider(25);
-    expect(value).toEqual('25 days ago');
+      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);
+    });
   });
-});
 
-describe('createDateStringFromTs', () => {
-  it('should correctly create string value from timestamp', () => {
-    const value = createDateStringFromTs(1583932327000);
-    expect(value).toEqual('March 11');
+  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('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);
+  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');
+    });
   });
-  it('should keep queries based on data source filter', () => {
-    const filteredQueries = filterAndSortQueries(
-      mock.storedHistory,
-      SortOrder.Ascending,
-      ['datasource history name'],
-      ''
-    );
-    expect(filteredQueries).toHaveLength(1);
+
+  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([]);
+    });
   });
-  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);
+
+  describe('mapNumbertoTimeInSlider', () => {
+    it('should correctly map number to value', () => {
+      const value = mapNumbertoTimeInSlider(25);
+      expect(value).toEqual('25 days ago');
+    });
   });
-  it('should include queries based on search filter', () => {
-    const filteredQueries = filterAndSortQueries(mock.storedHistory, SortOrder.Ascending, [], 'query1');
-    expect(filteredQueries).toHaveLength(1);
+
+  describe('createDateStringFromTs', () => {
+    it('should correctly create string value from timestamp', () => {
+      const value = createDateStringFromTs(1583932327000);
+      expect(value).toEqual('March 11');
+    });
   });
-});
 
-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');
+  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);
+    });
   });
-  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('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);
+    });
   });
 });

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 };
+    }
+
+    // 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 = [
-      { queries: newQueriesToSave, ts, datasourceId, datasourceName, starred, comment, sessionName },
+    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[] {

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

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) {
     <div className={styles.container}>
       <Field
         label="History time span"
-        description="Select the period of time for which Grafana will save your query history"
+        description={`Select the period of time for which Grafana will save your query history. Up to ${MAX_HISTORY_ITEMS} entries will be stored.`}
         className="space-between"
       >
         <div className={styles.input}>

public/app/features/explore/state/main.ts

@@ -21,6 +21,8 @@ export interface SyncTimesPayload {
 export const syncTimesAction = createAction<SyncTimesPayload>('explore/syncTimes');
 
 export const richHistoryUpdatedAction = createAction<any>('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 {

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

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
    */