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

[Lens] Bind all time fields to the time picker #63874

Merged
merged 34 commits into from
Apr 30, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
840bbb7
Bind non primary time fields to timepicker
Apr 17, 2020
f04a572
Fix typescript argument types
Apr 17, 2020
c54fdeb
Merge branch 'master' into non-primary-time-fields
Apr 20, 2020
f4073fc
Allow auto interval on all fields
Apr 20, 2020
c78f3f1
Remove lens_auto_date function
Apr 20, 2020
fbde5fb
Fix existing jest tests and add test todos
Apr 20, 2020
c8baa9a
Remove lens_auto_date from esarchives
Apr 21, 2020
76aaff7
Add TimeBuckets jest tests
Apr 21, 2020
2dcbb76
Fix typo in esarchiver
Apr 21, 2020
290611d
Address review feedback
Apr 21, 2020
5b2f684
Make code a bit better readable
Apr 21, 2020
e69e101
Fix default time field retrieval
Apr 22, 2020
872d76e
Fix TS errors
Apr 22, 2020
47234a3
Add esaggs interpreter tests
Apr 22, 2020
1da467a
Change public API doc of data plugin
Apr 22, 2020
3158097
Add toExpression tests for index pattern datasource
Apr 22, 2020
cb777ed
Merge branch 'master' into non-primary-time-fields
Apr 23, 2020
b6a9fc2
Add migration stub
Apr 24, 2020
f4da7aa
Merge remote-tracking branch 'origin/master' into HEAD
wylieconlon Apr 27, 2020
d745736
Add full migration
wylieconlon Apr 28, 2020
20e69ad
Fix naming inconsistency in esaggs
wylieconlon Apr 28, 2020
e8dbdb1
Merge branch 'master' into non-primary-time-fields
elasticmachine Apr 28, 2020
b2407f9
Merge remote-tracking branch 'origin/master' into time-non-primary-ti…
wylieconlon Apr 28, 2020
c312ab0
Fix naming issue
wylieconlon Apr 28, 2020
6d6befd
Merge remote-tracking branch 'origin/master' into time-non-primary-ti…
wylieconlon Apr 28, 2020
8016d5a
Revert archives to un-migrated version
wylieconlon Apr 28, 2020
cbf7ecc
Merge remote-tracking branch 'origin/master' into time-non-primary-ti…
wylieconlon Apr 29, 2020
93e6956
Ignore expressions that are already migrated
wylieconlon Apr 29, 2020
caad0b7
test: remove extra spaces and timeField=\\"products.created_on\\"} t…
mbondyra Apr 29, 2020
5517aef
Rename all timeField -> timeFields
wylieconlon Apr 29, 2020
5f7d3e5
Combine duplicate functions
wylieconlon Apr 29, 2020
7a3adef
Merge remote-tracking branch 'origin/master' into time-non-primary-ti…
wylieconlon Apr 29, 2020
4b08d1e
Fix boolean error and add test for it
wylieconlon Apr 29, 2020
8562812
Commit API changes
wylieconlon Apr 29, 2020
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
52 changes: 39 additions & 13 deletions src/plugins/data/public/query/timefilter/get_time.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,44 @@ export function calculateBounds(timeRange: TimeRange, options: CalculateBoundsOp
};
}

function createTimeRangeFilter(
indexPattern: IIndexPattern,
timeRange: TimeRange,
field: IFieldType,
forceNow?: Date
) {
const bounds = calculateBounds(timeRange, { forceNow });
if (!bounds) {
return;
}
return buildRangeFilter(
field,
{
...(bounds.min && { gte: bounds.min.toISOString() }),
...(bounds.max && { lte: bounds.max.toISOString() }),
format: 'strict_date_optional_time',
},
indexPattern
);
}

export function getTimeForField(
indexPattern: IIndexPattern | undefined,
timeRange: TimeRange,
fieldName: string
) {
if (!indexPattern) {
return;
}

const field = indexPattern.fields.find(f => f.name === fieldName);
if (!field) {
return;
}

return createTimeRangeFilter(indexPattern, timeRange, field);
}

export function getTime(
Copy link
Contributor

Choose a reason for hiding this comment

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

getTimeFilter \ getTimeFilterDefault :)

Copy link
Member

Choose a reason for hiding this comment

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

++ The naming getTime and getTimeFilter is a bit unclear.

another nit - It would also be nice to maybe add an explanatory comment to each of the functions here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just took a harder look at this, thanks for the prodding to do this. The naming confusion was because both functions were basically 90% similar code with one extra input, so I've condensed these into the same function with an options argument instead of positional arguments.

indexPattern: IIndexPattern | undefined,
timeRange: TimeRange,
Expand All @@ -50,17 +88,5 @@ export function getTime(
return;
}

const bounds = calculateBounds(timeRange, { forceNow });
if (!bounds) {
return;
}
return buildRangeFilter(
timefield,
{
...(bounds.min && { gte: bounds.min.toISOString() }),
...(bounds.max && { lte: bounds.max.toISOString() }),
format: 'strict_date_optional_time',
},
indexPattern
);
return createTimeRangeFilter(indexPattern, timeRange, timefield, forceNow);
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const updateTimeBuckets = (
customBuckets?: IBucketDateHistogramAggConfig['buckets']
) => {
const bounds =
agg.params.timeRange && agg.fieldIsTimeField()
agg.params.timeRange && (agg.fieldIsTimeField() || agg.params.interval === 'auto')
? timefilter.calculateBounds(agg.params.timeRange)
: undefined;
const buckets = customBuckets || agg.buckets;
Expand Down
47 changes: 41 additions & 6 deletions src/plugins/data/public/search/expressions/esaggs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,26 @@ import { Adapters } from '../../../../../plugins/inspector/public';
import { IAggConfigs } from '../aggs';
import { ISearchSource, SearchSource } from '../search_source';
import { tabifyAggResponse } from '../tabify';
import { Filter, Query, serializeFieldFormat, TimeRange } from '../../../common';
import { FilterManager, getTime } from '../../query';
import {
Filter,
Query,
serializeFieldFormat,
TimeRange,
IIndexPattern,
RangeFilter,
} from '../../../common';
import { FilterManager } from '../../query';
import { getSearchService, getQueryService, getIndexPatterns } from '../../services';
import { buildTabularInspectorData } from './build_tabular_inspector_data';
import { getRequestInspectorStats, getResponseInspectorStats, serializeAggConfig } from './utils';
import { calculateBounds, getTimeForField } from '../../query/timefilter/get_time';
Copy link
Contributor

Choose a reason for hiding this comment

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

//nit

Should be exported from ../../query


export interface RequestHandlerParams {
searchSource: ISearchSource;
aggs: IAggConfigs;
timeRange?: TimeRange;
timeFields?: string[];
indexPattern?: IIndexPattern;
query?: Query;
filters?: Filter[];
forceFetch: boolean;
Expand All @@ -65,12 +75,15 @@ interface Arguments {
partialRows: boolean;
includeFormatHints: boolean;
aggConfigs: string;
timeField?: string[];
Copy link
Member

Choose a reason for hiding this comment

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

remove ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parameter is not defined is required in the definition below, so it can be skipped and thus can be undefined. Actually the typing of the other parameters here is wrong, because they should also all have ? or being marked as required: true in their definition, but I didn't want to fix this in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

so we have timeField and timeFields ?

Copy link
Member

Choose a reason for hiding this comment

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

Actually the typing of the other parameters here is wrong, because they should also all have ? or being marked as required: true in their definition, but I didn't want to fix this in this PR.

@timroes This actually highlights weird thing about the Expression function typings which I don't think we have been doing consistently:

The types are what get enforced inside the function body. So while any arg with default: 'whatever' set is technically optional for users of the expression function, once you are inside the expression function itself, if it is not marked with ? in the arguments interface, TS will warn that the arg can be possibly undefined (since TS doesn't know that the defaults get injected at runtime).

So which is right? Do we write the typings to be accurate for the users of the functions (which means lots of ?), at the expense of function authors needing a lot of unnecessary if (args.whatever) checks, or args.whatever! ... Or do we write the typings to be accurate for the function authors, at the expense of users of the functions not having accurate typings?

IMO your proposed approach is probably better (include ? if there's a default or no required: true). It just means needing to update many of our function definitions (including Canvas functions which seem to have this discrepancy as well).

Copy link
Member

Choose a reason for hiding this comment

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

^ Thinking about this more, in ExpressionFunctionDefinition we might be able to enforce the presence of required: true for non-optional args.

At any rate; that's a topic for a different thread 🙂

}

const handleCourierRequest = async ({
searchSource,
aggs,
timeRange,
timeFields,
indexPattern,
query,
filters,
forceFetch,
Expand Down Expand Up @@ -111,9 +124,22 @@ const handleCourierRequest = async ({
return aggs.onSearchRequestStart(paramSearchSource, options);
});

if (timeRange) {
// If timeFields have been specified, use the specified ones, otherwise use primary time field of index
// pattern if it's available.
const allTimeFields =
timeFields && timeFields.length > 0
? timeFields
: [
indexPattern?.fields.find(field => field.name === indexPattern.timeFieldName)?.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

indexPattern.getTimeField

].filter((fieldName): fieldName is string => Boolean(fieldName));

// If a timeRange has been specified and we had at least one timeField available, create range
// filters for that those time fields
if (timeRange && allTimeFields.length > 0) {
timeFilterSearchSource.setField('filter', () => {
return getTime(searchSource.getField('index'), timeRange);
return allTimeFields
.map(fieldName => getTimeForField(indexPattern, timeRange, fieldName))
.filter((rangeFilter): rangeFilter is RangeFilter => Boolean(rangeFilter));
Copy link
Contributor

Choose a reason for hiding this comment

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

//nit
Maybe simplify with isRangeFilter?

});
}

Expand Down Expand Up @@ -181,11 +207,13 @@ const handleCourierRequest = async ({

(searchSource as any).finalResponse = resp;

const parsedTimeRange = timeRange ? getTime(aggs.indexPattern, timeRange) : null;
const parsedTimeRange = timeRange ? calculateBounds(timeRange) : null;
const tabifyParams = {
metricsAtAllLevels,
partialRows,
timeRange: parsedTimeRange ? parsedTimeRange.range : undefined,
timeRange: parsedTimeRange
? { from: parsedTimeRange.min, to: parsedTimeRange.max, timeFields: allTimeFields }
: undefined,
};

const tabifyCacheHash = calculateObjectHash({ tabifyAggs: aggs, ...tabifyParams });
Expand Down Expand Up @@ -242,6 +270,11 @@ export const esaggs = (): ExpressionFunctionDefinition<typeof name, Input, Argum
default: '""',
help: '',
},
timeField: {
types: ['string'],
help: '',
multi: true,
},
},
async fn(input, args, { inspectorAdapters, abortSignal }) {
const indexPatterns = getIndexPatterns();
Expand All @@ -260,9 +293,11 @@ export const esaggs = (): ExpressionFunctionDefinition<typeof name, Input, Argum
const response = await handleCourierRequest({
searchSource,
aggs,
indexPattern,
timeRange: get(input, 'timeRange', undefined),
query: get(input, 'query', undefined),
filters: get(input, 'filters', undefined),
timeFields: args.timeField,
forceFetch: true,
metricsAtAllLevels: args.metricsAtAllLevels,
partialRows: args.partialRows,
Expand Down
63 changes: 51 additions & 12 deletions src/plugins/data/public/search/tabify/buckets.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@

import { TabifyBuckets } from './buckets';
import { AggGroupNames } from '../aggs';
import moment from 'moment';

interface Bucket {
Copy link
Member

Choose a reason for hiding this comment

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

defining an interface in tests ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous definition is any and I just didn't want to continue using any below. I could have also inlined this type, but since I used it in multiple places just defined it as an interface on top.

key: number | string;
}

describe('Buckets wrapper', () => {
const check = (aggResp: any, count: number, keys: string[]) => {
Expand Down Expand Up @@ -187,9 +192,9 @@ describe('Buckets wrapper', () => {
},
};
const timeRange = {
gte: 150,
lte: 350,
name: 'date',
from: moment(150),
to: moment(350),
timeFields: ['date'],
};
const buckets = new TabifyBuckets(aggResp, aggParams, timeRange);

Expand All @@ -204,9 +209,9 @@ describe('Buckets wrapper', () => {
},
};
const timeRange = {
gte: 150,
lte: 350,
name: 'date',
from: moment(150),
to: moment(350),
timeFields: ['date'],
};
const buckets = new TabifyBuckets(aggResp, aggParams, timeRange);

Expand All @@ -221,9 +226,9 @@ describe('Buckets wrapper', () => {
},
};
const timeRange = {
gte: 100,
lte: 400,
name: 'date',
from: moment(100),
to: moment(400),
timeFields: ['date'],
};
const buckets = new TabifyBuckets(aggResp, aggParams, timeRange);

Expand All @@ -238,13 +243,47 @@ describe('Buckets wrapper', () => {
},
};
const timeRange = {
gte: 150,
lte: 350,
name: 'date',
from: moment(150),
to: moment(350),
timeFields: ['date'],
};
const buckets = new TabifyBuckets(aggResp, aggParams, timeRange);

expect(buckets).toHaveLength(4);
});

test('does drop bucket when multiple time fields specified', () => {
const aggParams = {
drop_partials: true,
field: {
name: 'date',
},
};
const timeRange = {
from: moment(100),
to: moment(350),
timeFields: ['date', 'other_datefield'],
};
const buckets = new TabifyBuckets(aggResp, aggParams, timeRange);

expect(buckets.buckets.map((b: Bucket) => b.key)).toEqual([100, 200]);
});

test('does not drop bucket when no timeFields have been specified', () => {
const aggParams = {
drop_partials: true,
field: {
name: 'date',
},
};
const timeRange = {
from: moment(100),
to: moment(350),
timeFields: [],
};
const buckets = new TabifyBuckets(aggResp, aggParams, timeRange);

expect(buckets.buckets.map((b: Bucket) => b.key)).toEqual([0, 100, 200, 300]);
});
});
});
12 changes: 6 additions & 6 deletions src/plugins/data/public/search/tabify/buckets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { get, isPlainObject, keys, findKey } from 'lodash';
import moment from 'moment';
import { IAggConfig } from '../aggs';
import { AggResponseBucket, TabbedRangeFilterParams } from './types';
import { AggResponseBucket, TabbedRangeFilterParams, TimeRangeInformation } from './types';

type AggParams = IAggConfig['params'] & {
drop_partials: boolean;
Expand All @@ -36,7 +36,7 @@ export class TabifyBuckets {
buckets: any;
_keys: any[] = [];

constructor(aggResp: any, aggParams?: AggParams, timeRange?: TabbedRangeFilterParams) {
constructor(aggResp: any, aggParams?: AggParams, timeRange?: TimeRangeInformation) {
if (aggResp && aggResp.buckets) {
this.buckets = aggResp.buckets;
} else if (aggResp) {
Expand Down Expand Up @@ -107,23 +107,23 @@ export class TabifyBuckets {

// dropPartials should only be called if the aggParam setting is enabled,
// and the agg field is the same as the Time Range.
private dropPartials(params: AggParams, timeRange?: TabbedRangeFilterParams) {
private dropPartials(params: AggParams, timeRange?: TimeRangeInformation) {
if (
!timeRange ||
this.buckets.length <= 1 ||
this.objectMode ||
params.field.name !== timeRange.name
!timeRange.timeFields.includes(params.field.name)
) {
return;
}

const interval = this.buckets[1].key - this.buckets[0].key;

this.buckets = this.buckets.filter((bucket: AggResponseBucket) => {
if (moment(bucket.key).isBefore(timeRange.gte)) {
if (moment(bucket.key).isBefore(timeRange.from)) {
return false;
}
if (moment(bucket.key + interval).isAfter(timeRange.lte)) {
if (moment(bucket.key + interval).isAfter(timeRange.to)) {
return false;
}
return true;
Expand Down
18 changes: 2 additions & 16 deletions src/plugins/data/public/search/tabify/tabify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { get } from 'lodash';
import { TabbedAggResponseWriter } from './response_writer';
import { TabifyBuckets } from './buckets';
import { TabbedResponseWriterOptions, TabbedRangeFilterParams } from './types';
import { TabbedResponseWriterOptions } from './types';
import { AggResponseBucket } from './types';
import { AggGroupNames, IAggConfigs } from '../aggs';

Expand Down Expand Up @@ -54,7 +54,7 @@ export function tabifyAggResponse(
switch (agg.type.type) {
case AggGroupNames.Buckets:
const aggBucket = get(bucket, agg.id);
const tabifyBuckets = new TabifyBuckets(aggBucket, agg.params, timeRange);
const tabifyBuckets = new TabifyBuckets(aggBucket, agg.params, respOpts?.timeRange);

if (tabifyBuckets.length) {
tabifyBuckets.forEach((subBucket, tabifyBucketKey) => {
Expand Down Expand Up @@ -153,20 +153,6 @@ export function tabifyAggResponse(
doc_count: esResponse.hits.total,
};

let timeRange: TabbedRangeFilterParams | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

yay


// Extract the time range object if provided
if (respOpts && respOpts.timeRange) {
const [timeRangeKey] = Object.keys(respOpts.timeRange);

if (timeRangeKey) {
timeRange = {
name: timeRangeKey,
...respOpts.timeRange[timeRangeKey],
};
}
}

collectBucket(aggConfigs, write, topLevelBucket, '', 1);

return write.response();
Expand Down
10 changes: 9 additions & 1 deletion src/plugins/data/public/search/tabify/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/

import { Moment } from 'moment';
import { RangeFilterParams } from '../../../common';
import { IAggConfig } from '../aggs';

Expand All @@ -25,11 +26,18 @@ export interface TabbedRangeFilterParams extends RangeFilterParams {
name: string;
}

/** @internal */
export interface TimeRangeInformation {
from?: Moment;
to?: Moment;
timeFields: string[];
}

/** @internal **/
export interface TabbedResponseWriterOptions {
metricsAtAllLevels: boolean;
partialRows: boolean;
timeRange?: { [key: string]: RangeFilterParams };
timeRange?: TimeRangeInformation;
}

/** @internal */
Expand Down
Loading