From 048194597ff1cd0f498e36e8085185568f784e85 Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Tue, 4 Jun 2024 15:04:36 +0200 Subject: [PATCH] Explore: Align time filters properly to day boundaries in query history (#88498) --- .../core/history/RichHistoryLocalStorage.ts | 5 +++- .../history/RichHistoryRemoteStorage.test.ts | 2 +- .../core/history/RichHistoryRemoteStorage.ts | 7 ++--- public/app/core/history/RichHistoryStorage.ts | 4 +-- .../history/richHistoryLocalStorageUtils.ts | 23 ++++++++++------ public/app/core/utils/richHistory.ts | 26 +++++++++++++++++-- public/app/core/utils/richHistoryTypes.ts | 10 +++++++ 7 files changed, 58 insertions(+), 19 deletions(-) diff --git a/public/app/core/history/RichHistoryLocalStorage.ts b/public/app/core/history/RichHistoryLocalStorage.ts index eee2d841584..4396b73a1fa 100644 --- a/public/app/core/history/RichHistoryLocalStorage.ts +++ b/public/app/core/history/RichHistoryLocalStorage.ts @@ -173,7 +173,10 @@ function updateRichHistory( */ function cleanUp(richHistory: RichHistoryLocalStorageDTO[]): RichHistoryLocalStorageDTO[] { const retentionPeriod: number = store.getObject(RICH_HISTORY_SETTING_KEYS.retentionPeriod, 7); - const retentionPeriodLastTs = createRetentionPeriodBoundary(retentionPeriod, false); + // We don't care about timezones that much here when creating the time stamp for deletion. First, not sure if we + // should be deleting entries based on timezone change that may serve only for querying and also the timezone + // difference would not change that much what is or isn't deleted compared to the default 2 weeks retention. + const retentionPeriodLastTs = createRetentionPeriodBoundary(retentionPeriod, { isLastTs: false }); /* Keep only queries, that are within the selected retention period or that are starred. * If no queries, initialize with empty array diff --git a/public/app/core/history/RichHistoryRemoteStorage.test.ts b/public/app/core/history/RichHistoryRemoteStorage.test.ts index c0cc2b260ca..31d5c726834 100644 --- a/public/app/core/history/RichHistoryRemoteStorage.test.ts +++ b/public/app/core/history/RichHistoryRemoteStorage.test.ts @@ -131,7 +131,7 @@ describe('RichHistoryRemoteStorage', () => { expect(fetchMock).toHaveBeenCalledWith({ method: 'GET', - url: `/api/query-history?datasourceUid=ds1&datasourceUid=ds2&searchString=${search}&sort=time-desc&to=now-${from}d&from=now-${to}d&limit=${expectedLimit}&page=${expectedPage}`, + url: `/api/query-history?datasourceUid=ds1&datasourceUid=ds2&searchString=${search}&sort=time-desc&to=${to}&from=${from}&limit=${expectedLimit}&page=${expectedPage}`, requestId: 'query-history-get-all', }); expect(richHistory).toMatchObject([richHistoryQuery]); diff --git a/public/app/core/history/RichHistoryRemoteStorage.ts b/public/app/core/history/RichHistoryRemoteStorage.ts index 2a7d4883916..8409db489a7 100644 --- a/public/app/core/history/RichHistoryRemoteStorage.ts +++ b/public/app/core/history/RichHistoryRemoteStorage.ts @@ -132,11 +132,8 @@ function buildQueryParams(filters: RichHistorySearchFilters): string { params = params + `&sort=${filters.sortOrder === SortOrder.Ascending ? 'time-asc' : 'time-desc'}`; } if (!filters.starred) { - const relativeFrom = filters.from === 0 ? 'now' : `now-${filters.from}d`; - const relativeTo = filters.to === 0 ? 'now' : `now-${filters.to}d`; - // TODO: Unify: remote storage from/to params are swapped comparing to frontend and local storage filters - params = params + `&to=${relativeFrom}`; - params = params + `&from=${relativeTo}`; + params = params + `&to=${filters.to}`; + params = params + `&from=${filters.from}`; } params = params + `&limit=100`; params = params + `&page=${filters.page || 1}`; diff --git a/public/app/core/history/RichHistoryStorage.ts b/public/app/core/history/RichHistoryStorage.ts index 6003fddd492..13d89a05172 100644 --- a/public/app/core/history/RichHistoryStorage.ts +++ b/public/app/core/history/RichHistoryStorage.ts @@ -1,4 +1,4 @@ -import { RichHistorySearchFilters, RichHistorySettings } from 'app/core/utils/richHistory'; +import { RichHistorySearchBackendFilters, RichHistorySettings } from 'app/core/utils/richHistoryTypes'; import { RichHistoryQuery } from '../../types'; @@ -35,7 +35,7 @@ export type RichHistoryResults = { richHistory: RichHistoryQuery[]; total?: numb * @alpha */ export default interface RichHistoryStorage { - getRichHistory(filters: RichHistorySearchFilters): Promise; + getRichHistory(filters: RichHistorySearchBackendFilters): Promise; /** * Creates new RichHistoryQuery, returns object with unique id and created date diff --git a/public/app/core/history/richHistoryLocalStorageUtils.ts b/public/app/core/history/richHistoryLocalStorageUtils.ts index 0cd8da310cb..d22b4816781 100644 --- a/public/app/core/history/richHistoryLocalStorageUtils.ts +++ b/public/app/core/history/richHistoryLocalStorageUtils.ts @@ -1,5 +1,7 @@ import { omit } from 'lodash'; +import { DateTime, dateTime, dateTimeForTimeZone } from '@grafana/data'; + import { RichHistoryQuery } from '../../types'; import { SortOrder } from '../utils/richHistoryTypes'; @@ -25,22 +27,27 @@ export function filterAndSortQueries( return sortQueries(filteredQueriesToBeSorted, sortOrder); } -export const createRetentionPeriodBoundary = (days: number, isLastTs: boolean) => { - const today = new Date(); - const date = new Date(today.setDate(today.getDate() - days)); +export const createRetentionPeriodBoundary = ( + days: number, + options: { isLastTs: boolean; tz?: string; now?: DateTime } +): number => { + let now = options.now; + if (!now) { + now = options.tz ? dateTimeForTimeZone(options.tz) : dateTime(); + } + now.add(-days, 'd'); + /* * As a retention period boundaries, we consider: * - The last timestamp equals to the 24:00 of the last day of retention * - The first timestamp that equals to the 00:00 of the first day of retention */ - const boundary = isLastTs ? date.setHours(24, 0, 0, 0) : date.setHours(0, 0, 0, 0); - return boundary; + const boundary = options.isLastTs ? now.endOf('d') : now.startOf('d'); + return boundary.valueOf(); }; function filterQueriesByTime(queries: RichHistoryQuery[], timeFilter: [number, number]) { - const filter1 = createRetentionPeriodBoundary(timeFilter[0], true); // probably the vars should have a different name - const filter2 = createRetentionPeriodBoundary(timeFilter[1], false); - return queries.filter((q) => q.createdAt < filter1 && q.createdAt > filter2); + return queries.filter((q) => q.createdAt > timeFilter[0] && q.createdAt < timeFilter[1]); } function filterQueriesByDataSource(queries: RichHistoryQuery[], listOfDatasourceFilters: string[]) { diff --git a/public/app/core/utils/richHistory.ts b/public/app/core/utils/richHistory.ts index ad95f625696..d02b886d19a 100644 --- a/public/app/core/utils/richHistory.ts +++ b/public/app/core/utils/richHistory.ts @@ -15,9 +15,16 @@ import { RichHistoryStorageWarning, RichHistoryStorageWarningDetails, } from '../history/RichHistoryStorage'; +import { createRetentionPeriodBoundary } from '../history/richHistoryLocalStorageUtils'; import { getLocalRichHistoryStorage, getRichHistoryStorage } from '../history/richHistoryStorageProvider'; +import { contextSrv } from '../services/context_srv'; -import { RichHistorySearchFilters, RichHistorySettings, SortOrder } from './richHistoryTypes'; +import { + RichHistorySearchBackendFilters, + RichHistorySearchFilters, + RichHistorySettings, + SortOrder, +} from './richHistoryTypes'; export { RichHistorySearchFilters, RichHistorySettings, SortOrder }; @@ -100,7 +107,22 @@ export async function addToRichHistory( } export async function getRichHistory(filters: RichHistorySearchFilters): Promise { - return await getRichHistoryStorage().getRichHistory(filters); + // Transforming from frontend filters where from and to are days from now to absolute timestamps. + const filtersCopy: RichHistorySearchBackendFilters = { + ...filters, + from: + filters.to === undefined + ? filters.to + : createRetentionPeriodBoundary(filters.to, { + isLastTs: true, + tz: contextSrv.user?.timezone, + }), + to: + filters.from === undefined + ? filters.from + : createRetentionPeriodBoundary(filters.from, { isLastTs: true, tz: contextSrv.user?.timezone }), + }; + return await getRichHistoryStorage().getRichHistory(filtersCopy); } export async function updateRichHistorySettings(settings: RichHistorySettings): Promise { diff --git a/public/app/core/utils/richHistoryTypes.ts b/public/app/core/utils/richHistoryTypes.ts index 4a02afbe594..0a944eef422 100644 --- a/public/app/core/utils/richHistoryTypes.ts +++ b/public/app/core/utils/richHistoryTypes.ts @@ -12,6 +12,7 @@ export enum SortOrder { } export interface RichHistorySettings { + // Number of days retentionPeriod: number; starredTabAsFirstTab: boolean; activeDatasourcesOnly: boolean; @@ -23,8 +24,17 @@ export type RichHistorySearchFilters = { sortOrder: SortOrder; /** Names of data sources (not uids) - used by local and remote storage **/ datasourceFilters: string[]; + // from and to represent number of days from now to filter by as the front end filtering is designed that way. + // so the resulting timerange from this will be [now - from, now - to]. from?: number; to?: number; starred: boolean; page?: number; }; + +export type RichHistorySearchBackendFilters = Omit & { + // This seems pointless but it serves as a documentation because we convert the filters from meaning days from now to + // mean absolute timestamps for the history backends. + from?: number; + to?: number; +};