From 61a102274d22e0f69442498c69eb5357d1eb0824 Mon Sep 17 00:00:00 2001 From: Darren Janeczek <38694490+darrenjaneczek@users.noreply.github.com> Date: Thu, 20 Jun 2024 22:13:51 -0400 Subject: [PATCH] datatrails: only store bookmark state instead of the entire trail step history (#86929) only store bookmark state instead of the entire step history --- public/app/features/trails/DataTrailCard.tsx | 53 ++-- public/app/features/trails/DataTrailsHome.tsx | 28 +- .../trails/TrailStore/TrailStore.test.ts | 244 +++++++++++++----- .../features/trails/TrailStore/TrailStore.ts | 105 ++++++-- public/app/features/trails/shared.ts | 3 +- 5 files changed, 321 insertions(+), 112 deletions(-) diff --git a/public/app/features/trails/DataTrailCard.tsx b/public/app/features/trails/DataTrailCard.tsx index a9ed32ee9b6..e441c5d3dc0 100644 --- a/public/app/features/trails/DataTrailCard.tsx +++ b/public/app/features/trails/DataTrailCard.tsx @@ -1,36 +1,57 @@ import { css } from '@emotion/css'; -import React from 'react'; +import React, { useMemo } from 'react'; import { dateTimeFormat, GrafanaTheme2 } from '@grafana/data'; import { AdHocFiltersVariable, sceneGraph } from '@grafana/scenes'; import { Card, IconButton, Stack, Tag, useStyles2 } from '@grafana/ui'; import { DataTrail } from './DataTrail'; +import { getTrailStore, DataTrailBookmark } from './TrailStore/TrailStore'; import { VAR_FILTERS } from './shared'; import { getDataSource, getDataSourceName, getMetricName } from './utils'; -export interface Props { - trail: DataTrail; - onSelect: (trail: DataTrail) => void; +export type Props = { + trail?: DataTrail; + bookmark?: DataTrailBookmark; + onSelect: () => void; onDelete?: () => void; -} +}; -export function DataTrailCard({ trail, onSelect, onDelete }: Props) { +export function DataTrailCard(props: Props) { + const { onSelect, onDelete, bookmark } = props; const styles = useStyles2(getStyles); - const filtersVariable = sceneGraph.lookupVariable(VAR_FILTERS, trail)!; - if (!(filtersVariable instanceof AdHocFiltersVariable)) { + const values = useMemo(() => { + let trail = props.trail || (bookmark && getTrailStore().getTrailForBookmark(bookmark)); + + if (!trail) { + return null; + } + + const filtersVariable = sceneGraph.lookupVariable(VAR_FILTERS, trail)!; + if (!(filtersVariable instanceof AdHocFiltersVariable)) { + return null; + } + + const createdAt = bookmark?.createdAt || trail.state.createdAt; + + return { + dsValue: getDataSource(trail), + filters: filtersVariable.state.filters, + metric: trail.state.metric, + createdAt, + }; + }, [props.trail, bookmark]); + + if (!values) { return null; } - const filters = filtersVariable.state.filters; - const dsValue = getDataSource(trail); - - const onClick = () => onSelect(trail); + const { dsValue, filters, metric, createdAt } = values; return ( - - {getMetricName(trail.state.metric)} + + {getMetricName(metric)}
{filters.map((f) => ( @@ -43,9 +64,9 @@ export function DataTrailCard({ trail, onSelect, onDelete }: Props) {
Datasource: {getDataSourceName(dsValue)}
- {trail.state.createdAt && ( + {createdAt && ( - Created: {dateTimeFormat(trail.state.createdAt, { format: 'LL' })} + Created: {dateTimeFormat(createdAt, { format: 'LL' })} )} diff --git a/public/app/features/trails/DataTrailsHome.tsx b/public/app/features/trails/DataTrailsHome.tsx index e65a65ff035..4e7a81144e0 100644 --- a/public/app/features/trails/DataTrailsHome.tsx +++ b/public/app/features/trails/DataTrailsHome.tsx @@ -10,7 +10,7 @@ import { Text } from '@grafana/ui/src/components/Text/Text'; import { DataTrail } from './DataTrail'; import { DataTrailCard } from './DataTrailCard'; import { DataTrailsApp } from './DataTrailsApp'; -import { getTrailStore } from './TrailStore/TrailStore'; +import { getBookmarkKey, getTrailStore } from './TrailStore/TrailStore'; import { reportExploreMetrics } from './interactions'; import { getDatasourceForNewTrail, getUrlForTrail, newMetricsTrail } from './utils'; @@ -29,9 +29,17 @@ export class DataTrailsHome extends SceneObjectBase { app.goToUrlForTrail(trail); }; - public onSelectTrail = (trail: DataTrail, isBookmark: boolean) => { + public onSelectRecentTrail = (trail: DataTrail) => { const app = getAppFor(this); - reportExploreMetrics('exploration_started', { cause: isBookmark ? 'bookmark_clicked' : 'recent_clicked' }); + reportExploreMetrics('exploration_started', { cause: 'recent_clicked' }); + getTrailStore().setRecentTrail(trail); + app.goToUrlForTrail(trail); + }; + + public onSelectBookmark = (bookmarkIndex: number) => { + const app = getAppFor(this); + reportExploreMetrics('exploration_started', { cause: 'bookmark_clicked' }); + const trail = getTrailStore().getTrailForBookmarkIndex(bookmarkIndex); getTrailStore().setRecentTrail(trail); app.goToUrlForTrail(trail); }; @@ -52,9 +60,6 @@ export class DataTrailsHome extends SceneObjectBase { return ; } - const onSelectRecent = (trail: DataTrail) => model.onSelectTrail(trail, false); - const onSelectBookmark = (trail: DataTrail) => model.onSelectTrail(trail, true); - return (
@@ -73,7 +78,7 @@ export class DataTrailsHome extends SceneObjectBase { model.onSelectRecentTrail(resolvedTrail)} /> ); })} @@ -83,13 +88,12 @@ export class DataTrailsHome extends SceneObjectBase {
Bookmarks
- {getTrailStore().bookmarks.map((trail, index) => { - const resolvedTrail = trail.resolve(); + {getTrailStore().bookmarks.map((bookmark, index) => { return ( model.onSelectBookmark(index)} onDelete={() => onDelete(index)} /> ); diff --git a/public/app/features/trails/TrailStore/TrailStore.test.ts b/public/app/features/trails/TrailStore/TrailStore.test.ts index 871dd2fc55c..d742f81cf9c 100644 --- a/public/app/features/trails/TrailStore/TrailStore.test.ts +++ b/public/app/features/trails/TrailStore/TrailStore.test.ts @@ -4,7 +4,7 @@ import { DataSourceType } from 'app/features/alerting/unified/utils/datasource'; import { MockDataSourceSrv, mockDataSource } from '../../alerting/unified/mocks'; import { DataTrail } from '../DataTrail'; -import { BOOKMARKED_TRAILS_KEY, RECENT_TRAILS_KEY, VAR_FILTERS } from '../shared'; +import { TRAIL_BOOKMARKS_KEY, RECENT_TRAILS_KEY, VAR_FILTERS } from '../shared'; import { SerializedTrail, getTrailStore } from './TrailStore'; @@ -482,11 +482,95 @@ describe('TrailStore', () => { }); }); - describe('Initialize store with one bookmark trail', () => { + describe('Initialize store with one bookmark trail but no recent trails', () => { beforeEach(() => { localStorage.clear(); localStorage.setItem( - BOOKMARKED_TRAILS_KEY, + TRAIL_BOOKMARKS_KEY, + JSON.stringify([ + { + urlValues: { + metric: 'bookmarked_metric', + from: 'now-1h', + to: 'now', + 'var-ds': 'prom-mock', + 'var-filters': [], + refresh: '', + }, + type: 'time', + }, + ]) + ); + getTrailStore().load(); + }); + + const store = getTrailStore(); + + it('should have no recent trails', () => { + expect(store.recent.length).toBe(0); + }); + + it('should accurately load bookmarked trails xx', () => { + expect(store.bookmarks.length).toBe(1); + const trail = store.getTrailForBookmarkIndex(0); + expect(trail.state.metric).toBe('bookmarked_metric'); + }); + + it('should save a new recent trail based on the bookmark', () => { + expect(store.recent.length).toBe(0); + const trail = store.getTrailForBookmarkIndex(0); + // Trail and history must be activated first + trail.activate(); + trail.state.history.activate(); + store.setRecentTrail(trail); + expect(store.recent.length).toBe(1); + }); + + it('should be able to obtain index of bookmark', () => { + const trail = store.getTrailForBookmarkIndex(0); + const index = store.getBookmarkIndex(trail); + expect(index).toBe(0); + }); + + it('index should be undefined for removed bookmarks', () => { + const trail = store.getTrailForBookmarkIndex(0); + store.removeBookmark(0); + const index = store.getBookmarkIndex(trail); + expect(index).toBe(undefined); + }); + + it('index should be undefined for a trail that has changed since it was bookmarked', () => { + const trail = store.getTrailForBookmarkIndex(0); + trail.setState({ metric: 'something_completely_different' }); + const index = store.getBookmarkIndex(trail); + expect(index).toBe(undefined); + }); + + it('should be able to obtain index of a bookmark for a trail that changed back to bookmarked state', () => { + const trail = store.getTrailForBookmarkIndex(0); + const bookmarkedMetric = trail.state.metric; + trail.setState({ metric: 'something_completely_different' }); + trail.setState({ metric: bookmarkedMetric }); + const index = store.getBookmarkIndex(trail); + expect(index).toBe(0); + }); + + it('should remove a bookmark', () => { + expect(store.bookmarks.length).toBe(1); + store.removeBookmark(0); + expect(store.bookmarks.length).toBe(0); + + jest.advanceTimersByTime(2000); + + expect(localStorage.getItem(TRAIL_BOOKMARKS_KEY)).toBe('[]'); + }); + }); + + describe('Initialize store with one legacy bookmark trail', () => { + beforeEach(() => { + localStorage.clear(); + localStorage.setItem( + TRAIL_BOOKMARKS_KEY, JSON.stringify([ { history: [ @@ -526,66 +610,18 @@ describe('TrailStore', () => { expect(store.recent.length).toBe(0); }); - it('should accurately load bookmarked trails', () => { + it('should accurately load legacy bookmark', () => { expect(store.bookmarks.length).toBe(1); - const trail = store.bookmarks[0].resolve(); - expect(trail.state.history.state.steps.length).toBe(2); - expect(trail.state.history.state.steps[0].type).toBe('start'); - expect(trail.state.history.state.steps[1].type).toBe('time'); - }); - - it('should save a new recent trail based on the bookmark', () => { - expect(store.recent.length).toBe(0); - const trail = store.bookmarks[0].resolve(); - store.setRecentTrail(trail); - expect(store.recent.length).toBe(1); - }); - - it('should be able to obtain index of bookmark', () => { - const trail = store.bookmarks[0].resolve(); - const index = store.getBookmarkIndex(trail); - expect(index).toBe(0); - }); - - it('index should be undefined for removed bookmarks', () => { - const trail = store.bookmarks[0].resolve(); - store.removeBookmark(0); - const index = store.getBookmarkIndex(trail); - expect(index).toBe(undefined); - }); - - it('index should be undefined for a trail that has changed since it was bookmarked', () => { - const trail = store.bookmarks[0].resolve(); - trail.setState({ metric: 'something_completely_different' }); - const index = store.getBookmarkIndex(trail); - expect(index).toBe(undefined); - }); - - it('should be able to obtain index of a bookmark for a trail that changed back to bookmarked state', () => { - const trail = store.bookmarks[0].resolve(); - const bookmarkedMetric = trail.state.metric; - trail.setState({ metric: 'something_completely_different' }); - trail.setState({ metric: bookmarkedMetric }); - const index = store.getBookmarkIndex(trail); - expect(index).toBe(0); - }); - - it('should remove a bookmark', () => { - expect(store.bookmarks.length).toBe(1); - store.removeBookmark(0); - expect(store.bookmarks.length).toBe(0); - - jest.advanceTimersByTime(2000); - - expect(localStorage.getItem(BOOKMARKED_TRAILS_KEY)).toBe('[]'); + const trail = store.getTrailForBookmarkIndex(0); + expect(trail.state.metric).toBe('access_permissions_duration_count'); }); }); - describe('Initialize store with one bookmark trail not on final step', () => { + describe('Initialize store with one legacy bookmark trail not bookmarked on final step', () => { beforeEach(() => { localStorage.clear(); localStorage.setItem( - BOOKMARKED_TRAILS_KEY, + TRAIL_BOOKMARKS_KEY, JSON.stringify([ { history: [ @@ -635,9 +671,87 @@ describe('TrailStore', () => { expect(store.recent.length).toBe(0); }); - it('should accurately load bookmarked trails', () => { + it('should accurately load legacy bookmark', () => { expect(store.bookmarks.length).toBe(1); - const trail = store.bookmarks[0].resolve(); + const trail = store.getTrailForBookmarkIndex(0); + expect(trail.state.metric).toBe('bookmarked_metric'); + }); + }); + + describe('Initialize store with one bookmark matching recent trail not on final step', () => { + beforeEach(() => { + localStorage.clear(); + localStorage.setItem( + RECENT_TRAILS_KEY, + JSON.stringify([ + { + history: [ + { + urlValues: { + from: 'now-1h', + to: 'now', + 'var-ds': 'prom-mock', + 'var-filters': [], + refresh: '', + }, + type: 'start', + }, + { + urlValues: { + metric: 'bookmarked_metric', + from: 'now-1h', + to: 'now', + 'var-ds': 'prom-mock', + 'var-filters': [], + refresh: '', + }, + type: 'time', + }, + { + urlValues: { + metric: 'some_other_metric', + from: 'now-1h', + to: 'now', + 'var-ds': 'prom-mock', + 'var-filters': [], + refresh: '', + }, + type: 'metric', + }, + ], + currentStep: 1, + }, + ]) + ); + localStorage.setItem( + TRAIL_BOOKMARKS_KEY, + JSON.stringify([ + { + urlValues: { + metric: 'bookmarked_metric', + from: 'now-1h', + to: 'now', + 'var-ds': 'prom-mock', + 'var-filters': [], + refresh: '', + }, + type: 'time', + }, + ]) + ); + getTrailStore().load(); + }); + + const store = getTrailStore(); + + it('should have 1 recent trail', () => { + expect(store.recent.length).toBe(1); + }); + + it('should accurately load bookmarked trail from matching recent', () => { + expect(store.bookmarks.length).toBe(1); + expect(store.recent.length).toBe(1); + const trail = store.getTrailForBookmarkIndex(0); expect(trail.state.history.state.steps.length).toBe(3); expect(trail.state.history.state.steps[0].type).toBe('start'); expect(trail.state.history.state.steps[1].type).toBe('time'); @@ -645,34 +759,34 @@ describe('TrailStore', () => { }); it('should save a new recent trail based on the bookmark', () => { - expect(store.recent.length).toBe(0); - const trail = store.bookmarks[0].resolve(); + expect(store.recent.length).toBe(1); + const trail = store.getTrailForBookmarkIndex(0); store.setRecentTrail(trail); expect(store.recent.length).toBe(1); }); it('should be able to obtain index of bookmark', () => { - const trail = store.bookmarks[0].resolve(); + const trail = store.getTrailForBookmarkIndex(0); const index = store.getBookmarkIndex(trail); expect(index).toBe(0); }); it('index should be undefined for removed bookmarks', () => { - const trail = store.bookmarks[0].resolve(); + const trail = store.getTrailForBookmarkIndex(0); store.removeBookmark(0); const index = store.getBookmarkIndex(trail); expect(index).toBe(undefined); }); it('index should be undefined for a trail that has changed since it was bookmarked', () => { - const trail = store.bookmarks[0].resolve(); + const trail = store.getTrailForBookmarkIndex(0); trail.setState({ metric: 'something_completely_different' }); const index = store.getBookmarkIndex(trail); expect(index).toBe(undefined); }); it('should be able to obtain index of a bookmark for a trail that changed back to bookmarked state', () => { - const trail = store.bookmarks[0].resolve(); + const trail = store.getTrailForBookmarkIndex(0); trail.setState({ metric: 'something_completely_different' }); expect(store.getBookmarkIndex(trail)).toBe(undefined); trail.setState({ metric: 'bookmarked_metric' }); @@ -684,7 +798,7 @@ describe('TrailStore', () => { store.removeBookmark(0); expect(store.bookmarks.length).toBe(0); jest.advanceTimersByTime(2000); - expect(localStorage.getItem(BOOKMARKED_TRAILS_KEY)).toBe('[]'); + expect(localStorage.getItem(TRAIL_BOOKMARKS_KEY)).toBe('[]'); }); }); }); diff --git a/public/app/features/trails/TrailStore/TrailStore.ts b/public/app/features/trails/TrailStore/TrailStore.ts index 69e2ff119b6..e11c9c8abdb 100644 --- a/public/app/features/trails/TrailStore/TrailStore.ts +++ b/public/app/features/trails/TrailStore/TrailStore.ts @@ -7,7 +7,8 @@ import { dispatch } from 'app/store/store'; import { notifyApp } from '../../../core/reducers/appNotification'; import { DataTrail } from '../DataTrail'; import { TrailStepType } from '../DataTrailsHistory'; -import { BOOKMARKED_TRAILS_KEY, RECENT_TRAILS_KEY } from '../shared'; +import { TRAIL_BOOKMARKS_KEY, RECENT_TRAILS_KEY } from '../shared'; +import { newMetricsTrail } from '../utils'; import { createBookmarkSavedNotification } from './utils'; @@ -20,13 +21,18 @@ export interface SerializedTrail { description: string; parentIndex: number; }>; - currentStep: number; + currentStep?: number; // Assume last step in history if not specified createdAt?: number; } +export interface DataTrailBookmark { + urlValues: SceneObjectUrlValues; + createdAt: number; +} + export class TrailStore { private _recent: Array> = []; - private _bookmarks: Array> = []; + private _bookmarks: DataTrailBookmark[] = []; private _save: () => void; constructor() { @@ -38,8 +44,7 @@ export class TrailStore { .map((trail) => this._serializeTrail(trail.resolve())); localStorage.setItem(RECENT_TRAILS_KEY, JSON.stringify(serializedRecent)); - const serializedBookmarks = this._bookmarks.map((trail) => this._serializeTrail(trail.resolve())); - localStorage.setItem(BOOKMARKED_TRAILS_KEY, JSON.stringify(serializedBookmarks)); + localStorage.setItem(TRAIL_BOOKMARKS_KEY, JSON.stringify(this._bookmarks)); }; this._save = debounce(doSave, 1000); @@ -51,9 +56,9 @@ export class TrailStore { }); } - private _loadFromStorage(key: string) { + private _loadRecentTrailsFromStorage() { const list: Array> = []; - const storageItem = localStorage.getItem(key); + const storageItem = localStorage.getItem(RECENT_TRAILS_KEY); if (storageItem) { const serializedTrails: SerializedTrail[] = JSON.parse(storageItem); @@ -65,6 +70,25 @@ export class TrailStore { return list; } + private _loadBookmarksFromStorage() { + const storageItem = localStorage.getItem(TRAIL_BOOKMARKS_KEY); + + const list: Array = storageItem ? JSON.parse(storageItem) : []; + + return list.map((item) => { + if (isSerializedTrail(item)) { + // Take the legacy SerializedTrail implementation of bookmark storage, and extract a DataTrailBookmark + const step = item.currentStep != null ? item.currentStep : item.history.length - 1; + const bookmark: DataTrailBookmark = { + urlValues: item.history[step].urlValues, + createdAt: item.createdAt || Date.now(), + }; + return bookmark; + } + return item; + }); + } + private _deserializeTrail(t: SerializedTrail): DataTrail { // reconstruct the trail based on the serialized history const trail = new DataTrail({ createdAt: t.createdAt }); @@ -107,6 +131,31 @@ export class TrailStore { }; } + public getTrailForBookmarkIndex(index: number) { + const bookmark = this._bookmarks[index]; + if (!bookmark) { + // Create a blank trail + return newMetricsTrail(); + } + return this.getTrailForBookmark(bookmark); + } + + public getTrailForBookmark(bookmark: DataTrailBookmark) { + const key = getBookmarkKey(bookmark); + // Match for recent trails that have the exact same state as the current step + for (const recent of this._recent) { + const trail = recent.resolve(); + if (getBookmarkKey(trail) === key) { + return trail; + } + } + // Just create a new trail with that state + + const trail = new DataTrail({}); + this._loadFromUrl(trail, bookmark.urlValues); + return trail; + } + private _loadFromUrl(node: SceneObject, urlValues: SceneObjectUrlValues) { const urlState = urlUtil.renderUrl('', urlValues); sceneUtils.syncStateFromSearchParams(node, new URLSearchParams(urlState)); @@ -118,8 +167,8 @@ export class TrailStore { } load() { - this._recent = this._loadFromStorage(RECENT_TRAILS_KEY); - this._bookmarks = this._loadFromStorage(BOOKMARKED_TRAILS_KEY); + this._recent = this._loadRecentTrailsFromStorage(); + this._bookmarks = this._loadBookmarksFromStorage(); this._refreshBookmarkIndexMap(); } @@ -152,8 +201,14 @@ export class TrailStore { } addBookmark(trail: DataTrail) { - const bookmark = new DataTrail(sceneUtils.cloneSceneObjectState(trail.state)); - this._bookmarks.unshift(bookmark.getRef()); + const urlState = getUrlSyncManager().getUrlState(trail); + + const bookmarkState: DataTrailBookmark = { + urlValues: urlState, + createdAt: Date.now(), + }; + + this._bookmarks.unshift(bookmarkState); this._refreshBookmarkIndexMap(); this._save(); dispatch(notifyApp(createBookmarkSavedNotification())); @@ -178,9 +233,7 @@ export class TrailStore { private _refreshBookmarkIndexMap() { this._bookmarkIndexMap.clear(); this._bookmarks.forEach((bookmarked, index) => { - const trail = bookmarked.resolve(); - - const key = getBookmarkKey(trail); + const key = getBookmarkKey(bookmarked); // If there are duplicate bookmarks, the latest index will be kept this._bookmarkIndexMap.set(key, index); }); @@ -190,24 +243,36 @@ export class TrailStore { function getUrlStateForComparison(trail: DataTrail) { const urlState = getUrlSyncManager().getUrlState(trail); // Make a few corrections + correctUrlStateForComparison(urlState); + return urlState; +} + +function correctUrlStateForComparison(urlState: SceneObjectUrlValues) { // Omit some URL parameters that are not useful for state comparison, // as they can change in the URL without creating new steps delete urlState.actionView; delete urlState.layout; delete urlState.metricSearch; + delete urlState.refresh; // Populate defaults - if (urlState['var-groupby'] === '') { + if (urlState['var-groupby'] === '' || urlState['var-groupby'] === undefined) { urlState['var-groupby'] = '$__all'; } + if (typeof urlState['var-filters'] !== 'string') { + urlState['var-filters'] = urlState['var-filters']?.filter((filter) => filter !== ''); + } + return urlState; } -function getBookmarkKey(trail: DataTrail) { - const key = JSON.stringify(getUrlStateForComparison(trail)); - return key; +export function getBookmarkKey(trail: DataTrail | DataTrailBookmark) { + if (trail instanceof DataTrail) { + return JSON.stringify(getUrlStateForComparison(trail)); + } + return JSON.stringify(correctUrlStateForComparison({ ...trail.urlValues })); } let store: TrailStore | undefined; @@ -218,3 +283,7 @@ export function getTrailStore(): TrailStore { return store; } + +function isSerializedTrail(serialized: unknown): serialized is SerializedTrail { + return serialized != null && typeof serialized === 'object' && 'history' in serialized; +} diff --git a/public/app/features/trails/shared.ts b/public/app/features/trails/shared.ts index 7c8ed5697b3..4b12725cb9a 100644 --- a/public/app/features/trails/shared.ts +++ b/public/app/features/trails/shared.ts @@ -31,7 +31,8 @@ export const trailDS = { uid: VAR_DATASOURCE_EXPR }; // Local storage keys export const RECENT_TRAILS_KEY = 'grafana.trails.recent'; -export const BOOKMARKED_TRAILS_KEY = 'grafana.trails.bookmarks'; + +export const TRAIL_BOOKMARKS_KEY = 'grafana.trails.bookmarks'; export type MakeOptional = Pick, K> & Omit;