removing kibana_datatable `in favor of` datatable` #75184

ppisljar · 2020-08-17T16:39:16Z

Summary

Removes kibana_datatable and makes all expression functions/renderers use same format of datatable

followup:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-08-17T16:39:18Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

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

lukeelmers

Finished a full pass, overall LGTM other than the skipped tests, which I assume were just for debugging

src/plugins/data/public/actions/filters/create_filters_from_value_click.ts

src/plugins/expressions/common/expression_types/specs/datatable.ts

test/functional/apps/dashboard/index.js

test/functional/config.js

wylieconlon · 2020-10-07T18:26:53Z

Fixed the latest merge conflicts

flash1293

I mostly went through the Lens part of the code. All of the changes look good to me but I'm not 100% happy with the solution of putting the format hint into meta.params like that - there is no indication this object is allowed to be used like that on the datatable level, right? How is it meant to be used? I'm sure there is some issue describing it, but it would be really to helpful to have that as a comment on the data table types - this is an extremely central type for us and with meta and params and sourceParams and so on I don't think it's intuitive anymore what it's doing. We should document this as well as possible

I fixed a unit test here
7e827bd

And updated snapshots here
03ca92a

There was one failure in the functional tests, not sure whether it's real or not.

flash1293 · 2020-10-08T08:37:08Z

x-pack/plugins/lens/public/utils.ts

@@ -14,8 +14,7 @@ export const desanitizeFilterContext = (
  const emptyTextValue = i18n.translate('xpack.lens.indexpattern.emptyTextColumnValue', {
    defaultMessage: '(empty)',
  });
-  return {
-    ...context,


What's wrong with spreading the context in here?

flash1293 · 2020-10-08T14:43:05Z

fail: runPipeline metricVis pipeline expression tests correctly renders metric with invalid data
15:58:52 │ ScriptTimeoutError: script timeout

@ppisljar I got that test failure locally as well - something is not triggered right on invalid data rendering. Could you take a look at that one?

# Conflicts: # src/plugins/vis_type_table/public/table_vis_fn.ts

flash1293

Noticed a regression in Lens data table

flash1293 · 2020-10-12T15:20:51Z

x-pack/plugins/lens/public/datatable_visualization/expression.test.tsx

@@ -105,8 +140,7 @@ describe('datatable_expression', () => {
            value: 'shoes',
          },
        ],
-        negate: true,


This is indicating a real bug - the value should be filtered out but instead it's only filtering for this value.
To reproduce:

Go to Lens

Create data table

Filter out value (clicking on the "-" button appearing when hovering over a cell)

Filter gets created filtering only for that value

flash1293 · 2020-10-12T15:24:55Z

x-pack/plugins/lens/public/utils.ts

@@ -14,8 +14,7 @@ export const desanitizeFilterContext = (
  const emptyTextValue = i18n.translate('xpack.lens.indexpattern.emptyTextColumnValue', {
    defaultMessage: '(empty)',
  });


@ppisljar The bug in the data table is very likely related to this change - as the rest of the context isn't spreaded anymore, negate is lost.

ppisljar · 2020-10-13T05:35:19Z

x-pack/plugins/canvas/public/components/datatable/datatable.tsx

-type IconType = 'string' | 'number' | 'date' | 'boolean' | 'null';
-
-const getIcon = (type: IconType) => {
+const getIcon = (type: DatatableColumnType | null) => {


i think this is the only change relevant to @elastic/kibana-canvas

kibanamachine · 2020-10-13T08:13:55Z

💚 Build Succeeded

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

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`data`	571	569	-2
`expressions`	105	104	-1
total			-3

async chunks size

id	before	after	diff
`lens`	1.0MB	1.0MB	-37.0B

distributable file count

id	before	after	diff
`default`	48453	48452	-1
`oss`	29194	29193	-1

page load bundle size

id	before	after	diff
`data`	1.1MB	1.1MB	+1.7KB
`expressions`	194.6KB	192.4KB	-2.3KB
`lens`	79.8KB	79.8KB	+65.0B
`regionMap`	49.9KB	49.9KB	-7.0B
`tileMap`	49.2KB	49.2KB	+5.0B
`urlDrilldown`	18.7KB	18.4KB	-370.0B
`visTypeMarkdown`	15.5KB	15.5KB	+18.0B
`visTypeMetric`	27.1KB	27.1KB	+11.0B
`visTypeTable`	20.2KB	20.1KB	-7.0B
`visTypeTagcloud`	22.0KB	22.1KB	+8.0B
`visTypeTimelion`	36.1KB	36.1KB	+18.0B
`visTypeVislib`	222.5KB	222.5KB	-14.0B
`visualizations`	272.7KB	272.8KB	+62.0B
total			-779.0B

History

💔 Build #81238 failed 8864959
💚 Build #81092 succeeded 77f0367
💚 Build #81056 succeeded 0090a06
💔 Build #81037 failed 2888c75
💔 Build #81013 failed 71a4b57

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

flash1293

Tested in Chrome and Lens parts LGTM - left one nit, but not a blocker

flash1293 · 2020-10-13T08:42:43Z

x-pack/plugins/lens/public/datatable_visualization/expression.test.tsx

@@ -106,7 +141,7 @@ describe('datatable_expression', () => {
          },
        ],
        negate: true,
-        timeFieldName: undefined,
+        timeFieldName: 'a',


This doesn't look correct - a is a string field but it ends up as the "timeFieldName".
Seems like the logic here https://github.com/elastic/kibana/pull/75184/files#diff-9579ca4b06d409542e1c027008518abe610abdd3bf7a3b897f408fec7a324e44R177 is flawed. Not blocking this PR as it doesn't seem like it got introduced here, but we should probably fix it.

flash1293 · 2020-10-13T08:44:48Z

x-pack/plugins/lens/public/utils.ts

@@ -36,4 +36,8 @@ export const desanitizeFilterContext = (
        : point
    ),
  };
+  if (context.timeFieldName) {


Do we still need this? seems like the spread takes care of it.

crob611

Canvas changes look fine 👍

This reverts commit 255b865.

spalger · 2020-10-13T15:54:45Z

@ppisljar sorry, needed to revert this as it includes port numbers in snapshots that change based on different deployment scenarios: https://kibana-ci.elastic.co/job/elastic+kibana+master/8872/execution/node/462/log/

…otphase-to-formlib * 'master' of github.com:elastic/kibana: (59 commits) [Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard (elastic#80193) [Security Solution] Reduce initial bundle size (elastic#78992) [Security Solution][Resolver] Fix Resize node box-shadow bug (elastic#80223) Move observability content (elastic#79978) skip flaky suite (elastic#79389) removing kibana_datatable` in favor of `datatable` (elastic#75184) [ML] Fixes for anomaly swim lane (elastic#80299) [Lens] Smokescreen lens test unskip (elastic#80190) Improved AlertsClient tests structure by splitting a huge alerts_client.tests.ts file into a specific files defined by its responsibility. (elastic#80088) [APM] React key warning when opening popover with external resources (elastic#80328) [Step 1] use Observables on server search API (elastic#79874) Apply back pressure in Task Manager whenever Elasticsearch responds with a 429 (elastic#75666) [Lens] Leverage original http request error (elastic#79831) [Security Solution][Case] Improve ServiceConnectorCaseParams type (elastic#80109) [SECURITY_SOLUTION] Fix query on alert histogram (elastic#80219) [DOCS] Update ingest node pipelines doc (elastic#79187) [Ingest Manager] Split up OpenAPI spec file (elastic#80107) [SECURITY_SOLUTION][ENDPOINT] Fix label on Trusted App create name field (elastic#80001) [Ingest Manager] Fix agent policy bump revision to create only one POLICY_CHANGE action (elastic#80081) Grid layout fixes (elastic#80305) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared/data_tier_allocation_field.tsx # x-pack/plugins/index_lifecycle_management/public/shared_imports.ts

ppisljar added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) v8.0.0 Team:AppArch v7.10.0 labels Aug 17, 2020

ppisljar requested a review from a team August 17, 2020 16:39

ppisljar requested a review from a team as a code owner August 17, 2020 16:39

wylieconlon reviewed Aug 31, 2020

View reviewed changes

src/plugins/data/public/search/expressions/esaggs.ts Show resolved Hide resolved

lukeelmers mentioned this pull request Sep 1, 2020

[data.search.tabify] Allow aggs to pass metadata to response (3) #76342

Closed

7 tasks

ppisljar force-pushed the expressions/datatable2 branch from 2e505f2 to 54d856d Compare September 22, 2020 13:29

ppisljar requested a review from a team as a code owner September 22, 2020 13:29

botelastic bot added the Feature:Embedding Embedding content via iFrame label Sep 22, 2020

ppisljar added release_note:skip Skip the PR/issue when compiling release notes WIP Work in progress labels Sep 22, 2020

ppisljar force-pushed the expressions/datatable2 branch 2 times, most recently from 5d54df4 to 972a2bc Compare September 24, 2020 14:48

botelastic bot added the Feature:Drilldowns Embeddable panel Drilldowns label Sep 24, 2020

ppisljar force-pushed the expressions/datatable2 branch from dbd57f4 to c44c509 Compare September 28, 2020 10:07

lukeelmers reviewed Sep 28, 2020

View reviewed changes

ppisljar added 5 commits September 29, 2020 00:04

removing kibana_datatable

b3612d4

updating

e93e530

fixing

c281334

fixing

436f977

fixes

9453941

ppisljar force-pushed the expressions/datatable2 branch from c44c509 to 9453941 Compare September 29, 2020 07:17

ppisljar removed the WIP Work in progress label Sep 29, 2020

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

c25ef8f

wylieconlon requested review from lukeelmers and flash1293 October 7, 2020 18:26

update snapshots

03ca92a

flash1293 force-pushed the expressions/datatable2 branch from d609c64 to 03ca92a Compare October 8, 2020 08:49

flash1293 reviewed Oct 8, 2020

View reviewed changes

fix snapshots

b9e4287

lukeelmers added v7.10.1 v7.11.0 and removed v7.10.0 v7.10.1 labels Oct 8, 2020

ppisljar added 4 commits October 12, 2020 02:21

Merge branch 'master' into expressions/datatable2

71a4b57

# Conflicts: # src/plugins/vis_type_table/public/table_vis_fn.ts

catching the error on renderer and displaying it

2888c75

emitting done when no data inside visualization container

0090a06

adding comments

77f0367

lukeelmers approved these changes Oct 12, 2020

View reviewed changes

flash1293 requested changes Oct 12, 2020

View reviewed changes

flash1293 reviewed Oct 12, 2020

View reviewed changes

ppisljar commented Oct 13, 2020

View reviewed changes

fixing lens regression

df06935

ppisljar force-pushed the expressions/datatable2 branch from 8864959 to df06935 Compare October 13, 2020 06:38

flash1293 approved these changes Oct 13, 2020

View reviewed changes

crob611 approved these changes Oct 13, 2020

View reviewed changes

ppisljar merged commit 255b865 into elastic:master Oct 13, 2020

ppisljar mentioned this pull request Oct 13, 2020

[7.x] removing kibana_datatable in favor of datatable` (#75184) #80358

Closed

ppisljar added a commit to ppisljar/kibana that referenced this pull request Oct 13, 2020

removing kibana_datatable in favor of datatable` (elastic#75184)

b2a37fc

spalger added a commit that referenced this pull request Oct 13, 2020

Revert "removing kibana_datatable in favor of datatable` (#75184)"

6eeab00

This reverts commit 255b865.

spalger added the reverted label Oct 13, 2020

ppisljar mentioned this pull request Oct 14, 2020

removing kibana_datatable in favor of datatable #80548

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removing kibana_datatable `in favor of` datatable` #75184

removing kibana_datatable `in favor of` datatable` #75184

ppisljar commented Aug 17, 2020 •

edited

Loading

elasticmachine commented Aug 17, 2020

lukeelmers left a comment

wylieconlon commented Oct 7, 2020

flash1293 left a comment

flash1293 Oct 8, 2020

flash1293 commented Oct 8, 2020

flash1293 left a comment

flash1293 Oct 12, 2020

flash1293 Oct 12, 2020

ppisljar Oct 13, 2020

kibanamachine commented Oct 13, 2020

flash1293 left a comment

flash1293 Oct 13, 2020

flash1293 Oct 13, 2020

crob611 left a comment

spalger commented Oct 13, 2020

removing kibana_datatable in favor of datatable` #75184

removing kibana_datatable in favor of datatable` #75184

Conversation

ppisljar commented Aug 17, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 17, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

wylieconlon commented Oct 7, 2020

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Oct 8, 2020

Choose a reason for hiding this comment

flash1293 commented Oct 8, 2020

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Oct 12, 2020

Choose a reason for hiding this comment

flash1293 Oct 12, 2020

Choose a reason for hiding this comment

ppisljar Oct 13, 2020

Choose a reason for hiding this comment

kibanamachine commented Oct 13, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

distributable file count

page load bundle size

History

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Oct 13, 2020

Choose a reason for hiding this comment

flash1293 Oct 13, 2020

Choose a reason for hiding this comment

crob611 left a comment

Choose a reason for hiding this comment

spalger commented Oct 13, 2020

removing kibana_datatable `in favor of` datatable` #75184

removing kibana_datatable `in favor of` datatable` #75184

ppisljar commented Aug 17, 2020 •

edited

Loading