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

timroes · 2020-04-17T16:13:20Z

This changes esaggs and Lens to support auto date histograms on any time field the user wants:

Fixes #63868

TODO:

Lens tests
Migration of Lens saved object
- Remove lens_auto_date from expression
- Specify all used date_fields in date_histograms as timeField parameters
- Provide tests for Migrations
Solve problem with single bars hiding

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

ppisljar · 2020-04-21T11:49:04Z

src/plugins/data/public/search/expressions/esaggs.ts

  partialRows: boolean;
  includeFormatHints: boolean;
  aggConfigs: string;
+  timeField?: string[];


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.

so we have timeField and timeFields ?

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).

^ 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 🙂

ppisljar · 2020-04-21T11:49:33Z

src/plugins/data/public/search/tabify/buckets.test.ts

 import { AggGroupNames } from '../aggs';
+import moment from 'moment';
+
+interface Bucket {


defining an interface in tests ?

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.

ppisljar

looked at it quickly and seems good

kibanamachine · 2020-04-21T11:52:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2dcbb76

History

💔 Build #42096 failed c8baa9a
💔 Build #41904 failed fbde5fb
💔 Build #41872 failed c78f3f1
💔 Build #41614 failed f04a572

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

lizozom

Added a few comment

lizozom · 2020-04-21T12:31:51Z

src/plugins/data/public/search/expressions/esaggs.ts

 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';


//nit

Should be exported from ../../query

lizozom · 2020-04-21T12:42:43Z

src/plugins/data/public/search/expressions/esaggs.ts

+    timeFields && timeFields.length > 0
+      ? timeFields
+      : [
+          indexPattern?.fields.find(field => field.name === indexPattern.timeFieldName)?.name,


indexPattern.getTimeField

lizozom · 2020-04-21T12:49:59Z

src/plugins/data/public/search/expressions/esaggs.ts

-      return getTime(searchSource.getField('index'), timeRange);
+      return allTimeFields
+        .map(fieldName => getTimeForField(indexPattern, timeRange, fieldName))
+        .filter((rangeFilter): rangeFilter is RangeFilter => Boolean(rangeFilter));


//nit
Maybe simplify with isRangeFilter?

lizozom · 2020-04-21T12:59:38Z

src/plugins/data/public/query/timefilter/get_time.ts

+  return createTimeRangeFilter(indexPattern, timeRange, field);
+}
+
 export function getTime(


getTimeFilter \ getTimeFilterDefault :)

++ 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.

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.

lizozom · 2020-04-21T13:07:22Z

src/plugins/data/public/search/tabify/tabify.ts

    doc_count: esResponse.hits.total,
  };

-  let timeRange: TabbedRangeFilterParams | undefined;


timroes · 2020-04-22T07:13:32Z

@elasticmachine merge upstream

ppisljar

code LGTM

mbondyra · 2020-04-28T14:59:14Z

Tested in Lens in commit "Add full migration"

migrations (from master -> this PR) date histogram with multiple layers both for primary and non-primary timefield, with auto interval and custom interval.
the behaviour for for primary and non-primary timefield with verifying correct datatable returned
Just in case, vertical bar date histogram visualization (not Lens)

…me-fields

wylieconlon · 2020-04-28T20:53:10Z

Fixed the bug in both the Lens migration and the unmigrated Lens expression. It turns out that timeField was the expected argument to esaggs, and I had migrated it to use timeFields causing the errors.

…me-fields

… timeField=\"products.created_on\"}

mbondyra

tested locally, LGTM 🆗

ppisljar

code LGTM

lukeelmers · 2020-04-29T15:22:09Z

src/plugins/data/public/query/timefilter/get_time.ts

+  return createTimeRangeFilter(indexPattern, timeRange, field);
+}
+
 export function getTime(


++ 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.

lukeelmers · 2020-04-29T15:23:25Z

src/plugins/data/public/search/expressions/esaggs.ts

 type Output = Promise<KibanaDatatable>;

-interface Arguments {
+export interface Arguments {


Why does this need to be exported now? (I might have missed it)

…me-fields

wylieconlon · 2020-04-29T18:47:54Z

I think I've resolved all of the naming comments that I could find. Thanks for bringing them up- @lukeelmers @lizozom if you want to review again, feel free.

lukeelmers

Thanks @wylieconlon, I think those changes make things much clearer!

Otherwise, the code here LGTM (I only reviewed updates to the data plugin).

kibanamachine · 2020-04-29T23:53:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8562812

History

💔 Build #44238 failed 4b08d1e
💔 Build #44197 failed 7a3adef
💚 Build #44054 succeeded caad0b7
💔 Build #43965 failed 93e6956
💔 Build #43888 failed c312ab0

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Bind non primary time fields to timepicker * Fix typescript argument types * Allow auto interval on all fields * Remove lens_auto_date function * Fix existing jest tests and add test todos * Remove lens_auto_date from esarchives * Add TimeBuckets jest tests * Fix typo in esarchiver * Address review feedback * Make code a bit better readable * Fix default time field retrieval * Fix TS errors * Add esaggs interpreter tests * Change public API doc of data plugin * Add toExpression tests for index pattern datasource * Add migration stub * Add full migration * Fix naming inconsistency in esaggs * Fix naming issue * Revert archives to un-migrated version * Ignore expressions that are already migrated * test: remove extra spaces and timeField=\\"products.created_on\\"} to timeField=\"products.created_on\"} * Rename all timeField -> timeFields * Combine duplicate functions * Fix boolean error and add test for it * Commit API changes Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]>

* master: (42 commits) [Ingest] Allow aggent to send metadata compliant with ECS (elastic#64452) [Endpoint] Remove todos, urls to issues (elastic#64833) [Uptime] Remove hard coded value for monitor states histograms (elastic#64396) Feature/send feedback link (elastic#64845) [ML] Moving get filters capability to admin (elastic#64879) Remove edit alert button from alerts list (elastic#64643) [EPM] Handle constant_keyword type in KB index patterns and ES index templates (elastic#64876) [ML] Disable data frame anaylics clone button based on permission (elastic#64830) Dashboard url generator to preserve saved filters from destination dashboard (elastic#64767) add generic typings for SavedObjectMigrationFn (elastic#63943) Allow to define and update a defaultPath for applications (elastic#64498) [Event Log] add rel=primary to saved objects for query targets (elastic#64615) [Lens] Use a size of 5 for first string field in visualization (elastic#64726) [SIEM][Lists] Removes plugin dependencies, adds more unit tests, fixes more TypeScript types [Ingest] Edit datasource UI (elastic#64727) [Lens] Bind all time fields to the time picker (elastic#63874) [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613) Improve alpha messaging (elastic#64692) [Ingest] Allow to enable monitoring of elastic agent (elastic#63598) [Metrics UI] Fix alerting when a filter query is present (elastic#64575) ...

* [Lens] Bind all time fields to the time picker (#63874) * Bind non primary time fields to timepicker * Fix typescript argument types * Allow auto interval on all fields * Remove lens_auto_date function * Fix existing jest tests and add test todos * Remove lens_auto_date from esarchives * Add TimeBuckets jest tests * Fix typo in esarchiver * Address review feedback * Make code a bit better readable * Fix default time field retrieval * Fix TS errors * Add esaggs interpreter tests * Change public API doc of data plugin * Add toExpression tests for index pattern datasource * Add migration stub * Add full migration * Fix naming inconsistency in esaggs * Fix naming issue * Revert archives to un-migrated version * Ignore expressions that are already migrated * test: remove extra spaces and timeField=\\"products.created_on\\"} to timeField=\"products.created_on\"} * Rename all timeField -> timeFields * Combine duplicate functions * Fix boolean error and add test for it * Commit API changes Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]> * fix: fix migration * types fix Co-authored-by: Tim Roes <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]>

…bana into pipeline-editor-part-mvp-2 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (90 commits) remove unused import address review feedback [Ingest pipelines] Cleanup (elastic#64794) [Ingest] Edit datasource UI (elastic#64727) [Lens] Bind all time fields to the time picker (elastic#63874) [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613) Improve alpha messaging (elastic#64692) [Ingest] Allow to enable monitoring of elastic agent (elastic#63598) [Metrics UI] Fix alerting when a filter query is present (elastic#64575) skip flaky suite (elastic#64812) (elastic#64723) [Maps] do not display EMS or kibana layer wizards when not configured (elastic#64554) [Reporting/Test] Convert functional test code to Typescript (elastic#64601) make inserting timestamp with navigate methods optional with default true (elastic#64655) [EPM] Update UI to handle package versions and updates (elastic#64689) Minimize dependencies required by our telemetry middleware (elastic#64665) [Telemetry] oss api tests (elastic#64602) [ML] Adding endpoint capability checks (elastic#64662) Update jest config for coverage (elastic#64648) [SIEM][NP] Fixes bug in ML signals promotion (elastic#64720) share single data plugin bundle (elastic#64549) ...

Bind non primary time fields to timepicker

840bbb7

timroes added the Feature:Lens label Apr 17, 2020

Tim Roes added 8 commits April 17, 2020 18:24

Fix typescript argument types

f04a572

Merge branch 'master' into non-primary-time-fields

c54fdeb

Allow auto interval on all fields

f4073fc

Remove lens_auto_date function

c78f3f1

Fix existing jest tests and add test todos

fbde5fb

Remove lens_auto_date from esarchives

c8baa9a

Add TimeBuckets jest tests

76aaff7

Fix typo in esarchiver

2dcbb76

ppisljar reviewed Apr 21, 2020

View reviewed changes

lizozom reviewed Apr 21, 2020

View reviewed changes

Tim Roes added 2 commits April 21, 2020 17:20

Address review feedback

290611d

Make code a bit better readable

5b2f684

Tim Roes added 5 commits April 22, 2020 09:20

Fix default time field retrieval

e69e101

Fix TS errors

872d76e

Add esaggs interpreter tests

47234a3

Change public API doc of data plugin

1da467a

Add toExpression tests for index pattern datasource

3158097

ppisljar approved these changes Apr 23, 2020

View reviewed changes

Tim Roes added 2 commits April 23, 2020 16:37

Merge branch 'master' into non-primary-time-fields

cb777ed

Add migration stub

b6a9fc2

This was referenced Apr 27, 2020

[Lens] Don't use auto interval to get interval for single bar #64502

Merged

[Lens] Get minInterval to pass to charts for single bar xyCharts for elastic-charts #64516

Closed

wylieconlon added 2 commits April 27, 2020 18:01

Merge remote-tracking branch 'origin/master' into HEAD

f4da7aa

Add full migration

d745736

wylieconlon added 2 commits April 28, 2020 16:09

Merge remote-tracking branch 'origin/master' into time-non-primary-ti…

b2407f9

…me-fields

Fix naming issue

c312ab0

wylieconlon and others added 5 commits April 28, 2020 18:57

Merge remote-tracking branch 'origin/master' into time-non-primary-ti…

6d6befd

…me-fields

Revert archives to un-migrated version

8016d5a

Merge remote-tracking branch 'origin/master' into time-non-primary-ti…

cbf7ecc

…me-fields

Ignore expressions that are already migrated

93e6956

test: remove extra spaces and timeField=\\"products.created_on\\"} to…

caad0b7

… timeField=\"products.created_on\"}

mbondyra approved these changes Apr 29, 2020

View reviewed changes

mbondyra requested review from lizozom and ppisljar April 29, 2020 14:31

ppisljar approved these changes Apr 29, 2020

View reviewed changes

lukeelmers reviewed Apr 29, 2020

View reviewed changes

wylieconlon added 3 commits April 29, 2020 14:26

Rename all timeField -> timeFields

5517aef

Combine duplicate functions

5f7d3e5

Merge remote-tracking branch 'origin/master' into time-non-primary-ti…

7a3adef

…me-fields

lukeelmers approved these changes Apr 29, 2020

View reviewed changes

wylieconlon added 2 commits April 29, 2020 16:37

Fix boolean error and add test for it

4b08d1e

Commit API changes

8562812

wylieconlon merged commit 9b65cbd into elastic:master Apr 30, 2020

wylieconlon mentioned this pull request Apr 30, 2020

[7.x] [Lens] Bind all time fields to the time picker (#63874) #64857

Merged

wylieconlon mentioned this pull request Apr 30, 2020

[Lens] date histogram sent with wrong interval on non-time index-patterns #55165

Closed

mbondyra mentioned this pull request May 5, 2020

[Lens] Data histogram on not primary timefield in lens fails to render correctly for XYChart #60733

Closed

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

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

Uh oh!

Conversation

timroes commented Apr 17, 2020 • edited by wylieconlon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 21, 2020

💚 Build Succeeded

History

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timroes commented Apr 22, 2020

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

mbondyra commented Apr 28, 2020

Uh oh!

wylieconlon commented Apr 28, 2020

Uh oh!

mbondyra left a comment

Choose a reason for hiding this comment

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wylieconlon commented Apr 29, 2020

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 29, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

timroes commented Apr 17, 2020 •

edited by wylieconlon

Loading