Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover/Reporting] Fix export that does not contain relative time filter #110459

Merged
merged 20 commits into from
Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
249eb5c
version 1 of fix: we set the time range on the search source at CSV g…
jloleysens Aug 30, 2021
16229e7
updated jest tests and updated API for getSharingData
jloleysens Aug 30, 2021
801f5ef
make time range optional for getSharingData
jloleysens Aug 30, 2021
dd948df
Merge branch 'master' into data/allow-relative-timefilters
kibanamachine Aug 31, 2021
8849a83
Merge branch 'master' into data/allow-relative-timefilters
kibanamachine Aug 31, 2021
484309c
pivot to updating "getTime" functionality by introducing a new flag
jloleysens Aug 31, 2021
c9051e7
update jest snapshots
jloleysens Aug 31, 2021
1eafd0b
update comment
jloleysens Aug 31, 2021
1b54f4f
Merge branch 'master' into data/allow-relative-timefilters
kibanamachine Sep 1, 2021
4fbe5b3
refactored coerceToAbsoluteTime -> coerceRelativeTimeToAbsoluteTime a…
jloleysens Sep 1, 2021
57c60b3
fix jest test
jloleysens Sep 1, 2021
445bd2f
Merge branch 'master' into data/allow-relative-timefilters
kibanamachine Sep 2, 2021
1bb3f79
Merge branch 'master' into data/allow-relative-timefilters
kibanamachine Sep 3, 2021
8d41c1e
Merge branch 'master' into data/allow-relative-timefilters
kibanamachine Sep 6, 2021
3ed6354
Merge branch 'master' into data/allow-relative-timefilters
kibanamachine Sep 7, 2021
e3d8a02
Merge branch 'master' into data/allow-relative-timefilters
kibanamachine Sep 13, 2021
324c844
do not change createFilter API, rather create new createRelativeFilte…
jloleysens Sep 13, 2021
b386dbd
update jest tests
jloleysens Sep 13, 2021
bc914d9
update mock
jloleysens Sep 13, 2021
01c68df
update jest test mock
jloleysens Sep 13, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 83 additions & 5 deletions src/plugins/data/common/query/timefilter/get_time.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,18 @@
*/

import { RangeFilter } from '@kbn/es-query';
import type { IIndexPattern } from '../..';
import moment from 'moment';
import sinon from 'sinon';
import { getTime, getAbsoluteTimeRange } from './get_time';
import { getTime, getRelativeTime, getAbsoluteTimeRange } from './get_time';

describe('get_time', () => {
describe('getTime', () => {
test('build range filter in iso format', () => {
const clock = sinon.useFakeTimers(moment.utc([2000, 1, 1, 0, 0, 0, 0]).valueOf());

const filter = getTime(
{
({
id: 'test',
title: 'test',
timeFieldName: 'date',
Expand All @@ -31,7 +32,7 @@ describe('get_time', () => {
filterable: true,
},
],
} as any,
} as unknown) as IIndexPattern,
{ from: 'now-60y', to: 'now' }
) as RangeFilter;
expect(filter.range.date).toEqual({
Expand All @@ -46,7 +47,7 @@ describe('get_time', () => {
const clock = sinon.useFakeTimers(moment.utc([2000, 1, 1, 0, 0, 0, 0]).valueOf());

const filter = getTime(
{
({
id: 'test',
title: 'test',
timeFieldName: 'date',
Expand All @@ -68,7 +69,7 @@ describe('get_time', () => {
filterable: true,
},
],
} as any,
} as unknown) as IIndexPattern,
{ from: 'now-60y', to: 'now' },
{ fieldName: 'myCustomDate' }
) as RangeFilter;
Expand All @@ -80,6 +81,83 @@ describe('get_time', () => {
clock.restore();
});
});
describe('getRelativeTime', () => {
test('do not coerce relative time to absolute time when given flag', () => {
const filter = getRelativeTime(
({
id: 'test',
title: 'test',
timeFieldName: 'date',
fields: [
{
name: 'date',
type: 'date',
esTypes: ['date'],
aggregatable: true,
searchable: true,
filterable: true,
},
{
name: 'myCustomDate',
type: 'date',
esTypes: ['date'],
aggregatable: true,
searchable: true,
filterable: true,
},
],
} as unknown) as IIndexPattern,
{ from: 'now-60y', to: 'now' },
{ fieldName: 'myCustomDate' }
) as RangeFilter;

expect(filter.range.myCustomDate).toEqual({
gte: 'now-60y',
lte: 'now',
format: 'strict_date_optional_time',
});
});
test('do not coerce relative time to absolute time when given flag - with mixed from and to times', () => {
const clock = sinon.useFakeTimers(moment.utc().valueOf());
const filter = getRelativeTime(
({
id: 'test',
title: 'test',
timeFieldName: 'date',
fields: [
{
name: 'date',
type: 'date',
esTypes: ['date'],
aggregatable: true,
searchable: true,
filterable: true,
},
{
name: 'myCustomDate',
type: 'date',
esTypes: ['date'],
aggregatable: true,
searchable: true,
filterable: true,
},
],
} as unknown) as IIndexPattern,
{
from: '2020-09-01T08:30:00.000Z',
to: 'now',
},
{ fieldName: 'myCustomDate' }
) as RangeFilter;

expect(filter.range.myCustomDate).toEqual({
gte: '2020-09-01T08:30:00.000Z',
lte: 'now',
format: 'strict_date_optional_time',
});
clock.restore();
});
});
describe('getAbsoluteTimeRange', () => {
test('should forward absolute timerange as is', () => {
const from = '2000-01-01T00:00:00.000Z';
Expand Down
71 changes: 54 additions & 17 deletions src/plugins/data/common/query/timefilter/get_time.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,30 @@
*/

import dateMath from '@elastic/datemath';
import { omitBy } from 'lodash';
import { buildRangeFilter } from '@kbn/es-query';
import type { IIndexPattern, TimeRange, TimeRangeBounds } from '../..';
import type { Moment } from 'moment';
import type { IIndexPattern, TimeRange, TimeRangeBounds, RangeFilterParams } from '../..';

interface CalculateBoundsOptions {
forceNow?: Date;
}

const calculateLowerBound = (from: string, forceNow?: Date): undefined | Moment =>
dateMath.parse(from, { forceNow });

const calculateUpperBound = (to: string, forceNow?: Date): undefined | Moment =>
dateMath.parse(to, { roundUp: true, forceNow });

const isRelativeTime = (value: string): boolean => value.includes('now');

export function calculateBounds(
timeRange: TimeRange,
options: CalculateBoundsOptions = {}
): TimeRangeBounds {
return {
min: dateMath.parse(timeRange.from, { forceNow: options.forceNow }),
max: dateMath.parse(timeRange.to, { roundUp: true, forceNow: options.forceNow }),
min: calculateLowerBound(timeRange.from, options.forceNow),
max: calculateUpperBound(timeRange.to, options.forceNow),
};
}

Expand All @@ -44,15 +54,31 @@ export function getTime(
indexPattern,
timeRange,
options?.fieldName || indexPattern?.timeFieldName,
options?.forceNow
options?.forceNow,
true
);
}

export function getRelativeTime(
indexPattern: IIndexPattern | undefined,
timeRange: TimeRange,
options?: { forceNow?: Date; fieldName?: string }
) {
return createTimeRangeFilter(
indexPattern,
timeRange,
options?.fieldName || indexPattern?.timeFieldName,
options?.forceNow,
false
);
}

function createTimeRangeFilter(
indexPattern: IIndexPattern | undefined,
timeRange: TimeRange,
fieldName?: string,
forceNow?: Date
forceNow?: Date,
coerceRelativeTimeToAbsoluteTime: boolean = true
) {
if (!indexPattern) {
return;
Expand All @@ -64,17 +90,28 @@ function createTimeRangeFilter(
return;
}

const bounds = calculateBounds(timeRange, { forceNow });
if (!bounds) {
return;
let rangeFilterParams: RangeFilterParams = {
format: 'strict_date_optional_time',
};

if (coerceRelativeTimeToAbsoluteTime) {
const bounds = calculateBounds(timeRange, { forceNow });
if (!bounds) {
return;
}
rangeFilterParams.gte = bounds.min?.toISOString();
rangeFilterParams.lte = bounds.max?.toISOString();
} else {
rangeFilterParams.gte = isRelativeTime(timeRange.from)
? timeRange.from
: calculateLowerBound(timeRange.from, forceNow)?.toISOString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed if we are working with absolute time ? or could we always just take timeRange.from ?

Copy link
Contributor Author

@jloleysens jloleysens Sep 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption is yes. Some tests give values like Jan 1, 2019 @ 00:00:00.000 for absolute time which ES does not understand and so moment was converting these to ISO strings.

[UPDATE]
My assumption is that if it is contained in tests then it could also occur "in the wild"


rangeFilterParams.lte = isRelativeTime(timeRange.to)
? timeRange.to
: calculateUpperBound(timeRange.to, forceNow)?.toISOString();
}
return buildRangeFilter(
field,
{
...(bounds.min && { gte: bounds.min.toISOString() }),
...(bounds.max && { lte: bounds.max.toISOString() }),
format: 'strict_date_optional_time',
},
indexPattern
);

rangeFilterParams = omitBy(rangeFilterParams, (v) => v == null);

return buildRangeFilter(field, rangeFilterParams, indexPattern);
}
24 changes: 23 additions & 1 deletion src/plugins/data/public/query/timefilter/timefilter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
calculateBounds,
getAbsoluteTimeRange,
getTime,
getRelativeTime,
IIndexPattern,
RefreshInterval,
TimeRange,
Expand All @@ -27,7 +28,6 @@ import { createAutoRefreshLoop, AutoRefreshDoneFn } from './lib/auto_refresh_loo
export { AutoRefreshDoneFn };

// TODO: remove!

export class Timefilter {
// Fired when isTimeRangeSelectorEnabled \ isAutoRefreshSelectorEnabled are toggled
private enabledUpdated$ = new BehaviorSubject(false);
Expand Down Expand Up @@ -178,12 +178,34 @@ export class Timefilter {
}
};

/**
* Create a time filter that coerces all time values to absolute time.
*
* This is useful for creating a filter that ensures all ES queries will fetch the exact same data
* and leverages ES query cache for performance improvement.
*
* One use case is keeping different elements embedded in the same UI in sync.
*/
public createFilter = (indexPattern: IIndexPattern, timeRange?: TimeRange) => {
return getTime(indexPattern, timeRange ? timeRange : this._time, {
forceNow: this.nowProvider.get(),
});
};

/**
* Create a time filter that converts only absolute time to ISO strings, it leaves relative time
* values unchanged (e.g. "now-1").
*
* This is useful for sending datemath values to ES endpoints to generate reports over time.
*
* @note Consumers of this function need to ensure that the ES endpoint supports datemath.
*/
public createRelativeFilter = (indexPattern: IIndexPattern, timeRange?: TimeRange) => {
return getRelativeTime(indexPattern, timeRange ? timeRange : this._time, {
forceNow: this.nowProvider.get(),
});
};

public getBounds(): TimeRangeBounds {
return this.calculateBounds(this._time);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const createSetupContractMock = () => {
getBounds: jest.fn(),
calculateBounds: jest.fn(),
createFilter: jest.fn(),
createRelativeFilter: jest.fn(),
getRefreshIntervalDefaults: jest.fn(),
getTimeDefaults: jest.fn(),
getAbsoluteTime: jest.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ export const getTopNavLinks = ({
state.appStateContainer.getState(),
services.uiSettings
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

services.share.toggleShareContextMenu({
anchorElement,
allowEmbed: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ export function updateSearchSource(

// this is not the default index pattern, it determines that it's not of type rollup
if (indexPatternsUtils.isDefault(indexPattern)) {
searchSource.setField('filter', data.query.timefilter.timefilter.createFilter(indexPattern));
searchSource.setField(
'filter',
data.query.timefilter.timefilter.createRelativeFilter(indexPattern)
);
}

if (useNewFieldsApi) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ export class CsvGenerator {
const index = searchSource.getField('index');

if (!index) {
throw new Error(`The search must have a revference to an index pattern!`);
throw new Error(`The search must have a reference to an index pattern!`);
}

const { maxSizeBytes, bom, escapeFormulaValues, scroll: scrollSettings } = settings;
Expand Down