From f317316fd4f3fa5bb6e1310eb547c86fa4e094b5 Mon Sep 17 00:00:00 2001 From: Nathan L Smith Date: Tue, 2 Feb 2021 10:17:48 -0600 Subject: [PATCH] Round start and end values (#89030) When getting the start and end times, use the d3 time scale `ticks` function to round the start and end times. Example from a query: Before: ```json { "range": { "@timestamp": { "gte": 1611262874814, "lte": 1611263774814, "format": "epoch_millis" } } }, ``` After: ```json { "range": { "@timestamp": { "gte": 1611263040000, "lte": 1611263880000, "format": "epoch_millis" } } }, ``` The `ticks` function makes it so the amount of rounding is proportional to the size of the time range, so shorter time ranges will be rounded less. Also fix a bug where invalid ranges in the query string were not handled correctly. Fixes #84530. --- .../app/TraceLink/trace_link.test.tsx | 10 +- .../shared/DatePicker/date_picker.test.tsx | 3 +- .../url_params_context/helpers.test.ts | 97 +++++++++++++++---- .../context/url_params_context/helpers.ts | 29 ++++-- .../mock_url_params_context_provider.tsx | 3 +- .../url_params_context.test.tsx | 54 +++++++---- .../url_params_context/url_params_context.tsx | 54 +++++------ .../plugins/apm/public/hooks/use_fetcher.tsx | 3 + 8 files changed, 176 insertions(+), 77 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/TraceLink/trace_link.test.tsx b/x-pack/plugins/apm/public/components/app/TraceLink/trace_link.test.tsx index c07e00ef387c9c..a33be140d9a361 100644 --- a/x-pack/plugins/apm/public/components/app/TraceLink/trace_link.test.tsx +++ b/x-pack/plugins/apm/public/components/app/TraceLink/trace_link.test.tsx @@ -60,12 +60,13 @@ describe('TraceLink', () => { describe('when no transaction is found', () => { it('renders a trace page', () => { jest.spyOn(urlParamsHooks, 'useUrlParams').mockReturnValue({ + rangeId: 0, + refreshTimeRange: jest.fn(), + uiFilters: {}, urlParams: { rangeFrom: 'now-24h', rangeTo: 'now', }, - refreshTimeRange: jest.fn(), - uiFilters: {}, }); jest.spyOn(hooks, 'useFetcher').mockReturnValue({ data: { transaction: undefined }, @@ -87,12 +88,13 @@ describe('TraceLink', () => { describe('transaction page', () => { beforeAll(() => { jest.spyOn(urlParamsHooks, 'useUrlParams').mockReturnValue({ + rangeId: 0, + refreshTimeRange: jest.fn(), + uiFilters: {}, urlParams: { rangeFrom: 'now-24h', rangeTo: 'now', }, - refreshTimeRange: jest.fn(), - uiFilters: {}, }); }); diff --git a/x-pack/plugins/apm/public/components/shared/DatePicker/date_picker.test.tsx b/x-pack/plugins/apm/public/components/shared/DatePicker/date_picker.test.tsx index 222c27cc7ed6d8..1bf3cee32f286e 100644 --- a/x-pack/plugins/apm/public/components/shared/DatePicker/date_picker.test.tsx +++ b/x-pack/plugins/apm/public/components/shared/DatePicker/date_picker.test.tsx @@ -31,8 +31,9 @@ function MockUrlParamsProvider({ return ( { - describe('getParsedDate', () => { - describe('given undefined', () => { - it('returns undefined', () => { - expect(helpers.getParsedDate(undefined)).toBeUndefined(); + describe('getDateRange', () => { + describe('with non-rounded dates', () => { + describe('one minute', () => { + it('rounds the values', () => { + expect( + helpers.getDateRange({ + state: {}, + rangeFrom: '2021-01-28T05:47:52.134Z', + rangeTo: '2021-01-28T05:48:55.304Z', + }) + ).toEqual({ + start: '2021-01-28T05:47:50.000Z', + end: '2021-01-28T05:49:00.000Z', + }); + }); }); - }); - - describe('given a parsable date', () => { - it('returns the parsed date', () => { - expect(helpers.getParsedDate('1970-01-01T00:00:00.000Z')).toEqual( - '1970-01-01T00:00:00.000Z' - ); + describe('one day', () => { + it('rounds the values', () => { + expect( + helpers.getDateRange({ + state: {}, + rangeFrom: '2021-01-27T05:46:07.377Z', + rangeTo: '2021-01-28T05:46:13.367Z', + }) + ).toEqual({ + start: '2021-01-27T03:00:00.000Z', + end: '2021-01-28T06:00:00.000Z', + }); + }); }); - }); - describe('given a non-parsable date', () => { - it('returns null', () => { - expect(helpers.getParsedDate('nope')).toEqual(null); + describe('one year', () => { + it('rounds the values', () => { + expect( + helpers.getDateRange({ + state: {}, + rangeFrom: '2020-01-28T05:52:36.290Z', + rangeTo: '2021-01-28T05:52:39.741Z', + }) + ).toEqual({ + start: '2020-01-01T00:00:00.000Z', + end: '2021-02-01T00:00:00.000Z', + }); + }); }); }); - }); - describe('getDateRange', () => { describe('when rangeFrom and rangeTo are not changed', () => { it('returns the previous state', () => { expect( @@ -52,6 +76,45 @@ describe('url_params_context helpers', () => { }); }); + describe('when rangeFrom or rangeTo are falsy', () => { + it('returns the previous state', () => { + // Disable console warning about not receiving a valid date for rangeFrom + jest.spyOn(console, 'warn').mockImplementationOnce(() => {}); + + expect( + helpers.getDateRange({ + state: { + start: '1972-01-01T00:00:00.000Z', + end: '1973-01-01T00:00:00.000Z', + }, + rangeFrom: '', + rangeTo: 'now', + }) + ).toEqual({ + start: '1972-01-01T00:00:00.000Z', + end: '1973-01-01T00:00:00.000Z', + }); + }); + }); + + describe('when the start or end are invalid', () => { + it('returns the previous state', () => { + expect( + helpers.getDateRange({ + state: { + start: '1972-01-01T00:00:00.000Z', + end: '1973-01-01T00:00:00.000Z', + }, + rangeFrom: 'nope', + rangeTo: 'now', + }) + ).toEqual({ + start: '1972-01-01T00:00:00.000Z', + end: '1973-01-01T00:00:00.000Z', + }); + }); + }); + describe('when rangeFrom or rangeTo have changed', () => { it('returns new state', () => { jest.spyOn(datemath, 'parse').mockReturnValue(moment(0).utc()); diff --git a/x-pack/plugins/apm/public/context/url_params_context/helpers.ts b/x-pack/plugins/apm/public/context/url_params_context/helpers.ts index bff2ef5deb86cd..0be11d440aecb2 100644 --- a/x-pack/plugins/apm/public/context/url_params_context/helpers.ts +++ b/x-pack/plugins/apm/public/context/url_params_context/helpers.ts @@ -4,15 +4,16 @@ * you may not use this file except in compliance with the Elastic License. */ -import { compact, pickBy } from 'lodash'; import datemath from '@elastic/datemath'; +import { scaleUtc } from 'd3-scale'; +import { compact, pickBy } from 'lodash'; import { IUrlParams } from './types'; -export function getParsedDate(rawDate?: string, opts = {}) { +function getParsedDate(rawDate?: string, options = {}) { if (rawDate) { - const parsed = datemath.parse(rawDate, opts); - if (parsed) { - return parsed.toISOString(); + const parsed = datemath.parse(rawDate, options); + if (parsed && parsed.isValid()) { + return parsed.toDate(); } } } @@ -26,13 +27,27 @@ export function getDateRange({ rangeFrom?: string; rangeTo?: string; }) { + // If the previous state had the same range, just return that instead of calculating a new range. if (state.rangeFrom === rangeFrom && state.rangeTo === rangeTo) { return { start: state.start, end: state.end }; } + const start = getParsedDate(rangeFrom); + const end = getParsedDate(rangeTo, { roundUp: true }); + + // `getParsedDate` will return undefined for invalid or empty dates. We return + // the previous state if either date is undefined. + if (!start || !end) { + return { start: state.start, end: state.end }; + } + + // Calculate ticks for the time ranges to produce nicely rounded values. + const ticks = scaleUtc().domain([start, end]).nice().ticks(); + + // Return the first and last tick values. return { - start: getParsedDate(rangeFrom), - end: getParsedDate(rangeTo, { roundUp: true }), + start: ticks[0].toISOString(), + end: ticks[ticks.length - 1].toISOString(), }; } diff --git a/x-pack/plugins/apm/public/context/url_params_context/mock_url_params_context_provider.tsx b/x-pack/plugins/apm/public/context/url_params_context/mock_url_params_context_provider.tsx index b593cbd57a9a9a..1e546599ee8a30 100644 --- a/x-pack/plugins/apm/public/context/url_params_context/mock_url_params_context_provider.tsx +++ b/x-pack/plugins/apm/public/context/url_params_context/mock_url_params_context_provider.tsx @@ -30,8 +30,9 @@ export function MockUrlParamsContextProvider({ return ( ) { } describe('UrlParamsContext', () => { - beforeEach(() => { + beforeAll(() => { moment.tz.setDefault('Etc/GMT'); }); - afterEach(() => { + afterAll(() => { moment.tz.setDefault(''); }); @@ -50,8 +49,11 @@ describe('UrlParamsContext', () => { const wrapper = mountParams(location); const params = getDataFromOutput(wrapper); - expect(params.start).toEqual('2010-03-15T12:00:00.000Z'); - expect(params.end).toEqual('2010-04-10T12:00:00.000Z'); + + expect([params.start, params.end]).toEqual([ + '2010-03-15T00:00:00.000Z', + '2010-04-11T00:00:00.000Z', + ]); }); it('should update param values if location has changed', () => { @@ -66,8 +68,11 @@ describe('UrlParamsContext', () => { // force an update wrapper.setProps({ abc: 123 }); const params = getDataFromOutput(wrapper); - expect(params.start).toEqual('2009-03-15T12:00:00.000Z'); - expect(params.end).toEqual('2009-04-10T12:00:00.000Z'); + + expect([params.start, params.end]).toEqual([ + '2009-03-15T00:00:00.000Z', + '2009-04-11T00:00:00.000Z', + ]); }); it('should parse relative time ranges on mount', () => { @@ -76,13 +81,20 @@ describe('UrlParamsContext', () => { search: '?rangeFrom=now-1d%2Fd&rangeTo=now-1d%2Fd&transactionId=UPDATED', } as Location; + const nowSpy = jest.spyOn(Date, 'now').mockReturnValue(0); + const wrapper = mountParams(location); // force an update wrapper.setProps({ abc: 123 }); const params = getDataFromOutput(wrapper); - expect(params.start).toEqual(getParsedDate('now-1d/d')); - expect(params.end).toEqual(getParsedDate('now-1d/d', { roundUp: true })); + + expect([params.start, params.end]).toEqual([ + '1969-12-31T00:00:00.000Z', + '1970-01-01T00:00:00.000Z', + ]); + + nowSpy.mockRestore(); }); it('should refresh the time range with new values', async () => { @@ -130,8 +142,11 @@ describe('UrlParamsContext', () => { expect(calls.length).toBe(2); const params = getDataFromOutput(wrapper); - expect(params.start).toEqual('2005-09-20T12:00:00.000Z'); - expect(params.end).toEqual('2005-10-21T12:00:00.000Z'); + + expect([params.start, params.end]).toEqual([ + '2005-09-19T00:00:00.000Z', + '2005-10-23T00:00:00.000Z', + ]); }); it('should refresh the time range with new values if time range is relative', async () => { @@ -177,7 +192,10 @@ describe('UrlParamsContext', () => { await waitFor(() => {}); const params = getDataFromOutput(wrapper); - expect(params.start).toEqual('2000-06-14T00:00:00.000Z'); - expect(params.end).toEqual('2000-06-14T23:59:59.999Z'); + + expect([params.start, params.end]).toEqual([ + '2000-06-14T00:00:00.000Z', + '2000-06-15T00:00:00.000Z', + ]); }); }); diff --git a/x-pack/plugins/apm/public/context/url_params_context/url_params_context.tsx b/x-pack/plugins/apm/public/context/url_params_context/url_params_context.tsx index 0a3f8459ff0023..f66a45db261a70 100644 --- a/x-pack/plugins/apm/public/context/url_params_context/url_params_context.tsx +++ b/x-pack/plugins/apm/public/context/url_params_context/url_params_context.tsx @@ -4,28 +4,25 @@ * you may not use this file except in compliance with the Elastic License. */ +import { mapValues } from 'lodash'; import React, { createContext, - useMemo, useCallback, + useMemo, useRef, useState, } from 'react'; import { withRouter } from 'react-router-dom'; -import { uniqueId, mapValues } from 'lodash'; -import { IUrlParams } from './types'; -import { getParsedDate } from './helpers'; -import { resolveUrlParams } from './resolve_url_params'; -import { UIFilters } from '../../../typings/ui_filters'; -import { - localUIFilterNames, - - // eslint-disable-next-line @kbn/eslint/no-restricted-paths -} from '../../../server/lib/ui_filters/local_ui_filters/config'; +import { ENVIRONMENT_ALL } from '../../../common/environment_filter_values'; +import { LocalUIFilterName } from '../../../common/ui_filter'; import { pickKeys } from '../../../common/utils/pick_keys'; +// eslint-disable-next-line @kbn/eslint/no-restricted-paths +import { localUIFilterNames } from '../../../server/lib/ui_filters/local_ui_filters/config'; +import { UIFilters } from '../../../typings/ui_filters'; import { useDeepObjectIdentity } from '../../hooks/useDeepObjectIdentity'; -import { LocalUIFilterName } from '../../../common/ui_filter'; -import { ENVIRONMENT_ALL } from '../../../common/environment_filter_values'; +import { getDateRange } from './helpers'; +import { resolveUrlParams } from './resolve_url_params'; +import { IUrlParams } from './types'; interface TimeRange { rangeFrom: string; @@ -49,9 +46,10 @@ function useUiFilters(params: IUrlParams): UIFilters { const defaultRefresh = (_time: TimeRange) => {}; const UrlParamsContext = createContext({ - urlParams: {} as IUrlParams, + rangeId: 0, refreshTimeRange: defaultRefresh, uiFilters: {} as UIFilters, + urlParams: {} as IUrlParams, }); const UrlParamsProvider: React.ComponentClass<{}> = withRouter( @@ -60,7 +58,8 @@ const UrlParamsProvider: React.ComponentClass<{}> = withRouter( const { start, end, rangeFrom, rangeTo } = refUrlParams.current; - const [, forceUpdate] = useState(''); + // Counter to force an update in useFetcher when the refresh button is clicked. + const [rangeId, setRangeId] = useState(0); const urlParams = useMemo( () => @@ -75,28 +74,25 @@ const UrlParamsProvider: React.ComponentClass<{}> = withRouter( refUrlParams.current = urlParams; - const refreshTimeRange = useCallback( - (timeRange: TimeRange) => { - refUrlParams.current = { - ...refUrlParams.current, - start: getParsedDate(timeRange.rangeFrom), - end: getParsedDate(timeRange.rangeTo, { roundUp: true }), - }; - - forceUpdate(uniqueId()); - }, - [forceUpdate] - ); + const refreshTimeRange = useCallback((timeRange: TimeRange) => { + refUrlParams.current = { + ...refUrlParams.current, + ...getDateRange({ state: {}, ...timeRange }), + }; + + setRangeId((prevRangeId) => prevRangeId + 1); + }, []); const uiFilters = useUiFilters(urlParams); const contextValue = useMemo(() => { return { - urlParams, + rangeId, refreshTimeRange, + urlParams, uiFilters, }; - }, [urlParams, refreshTimeRange, uiFilters]); + }, [rangeId, refreshTimeRange, uiFilters, urlParams]); return ( diff --git a/x-pack/plugins/apm/public/hooks/use_fetcher.tsx b/x-pack/plugins/apm/public/hooks/use_fetcher.tsx index 27427d80adc962..0da96691be9571 100644 --- a/x-pack/plugins/apm/public/hooks/use_fetcher.tsx +++ b/x-pack/plugins/apm/public/hooks/use_fetcher.tsx @@ -13,6 +13,7 @@ import { AutoAbortedAPMClient, } from '../services/rest/createCallApmApi'; import { useApmPluginContext } from '../context/apm_plugin/use_apm_plugin_context'; +import { useUrlParams } from '../context/url_params_context/use_url_params'; export enum FETCH_STATUS { LOADING = 'loading', @@ -75,6 +76,7 @@ export function useFetcher( status: FETCH_STATUS.NOT_INITIATED, }); const [counter, setCounter] = useState(0); + const { rangeId } = useUrlParams(); useEffect(() => { let controller: AbortController = new AbortController(); @@ -157,6 +159,7 @@ export function useFetcher( }, [ counter, preservePreviousData, + rangeId, showToastOnError, ...fnDeps, /* eslint-enable react-hooks/exhaustive-deps */