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

removing kibana_datatable in favor of datatable #80548

Merged
merged 23 commits into from
Oct 16, 2020

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Oct 14, 2020

Summary

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

followup:

reopens #75184, meta params are now removed from serialized field format.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ppisljar ppisljar requested a review from a team October 14, 2020 15:57
@ppisljar ppisljar requested review from a team as code owners October 14, 2020 15:57
@botelastic botelastic bot added Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Oct 14, 2020
@ppisljar ppisljar added release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.11.0 and removed Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame labels Oct 14, 2020
@elasticmachine
Copy link
Contributor

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

@ppisljar ppisljar changed the title Expressions/datatable2 removing kibana_datatable in favor of datatable` Oct 14, 2020
@ppisljar ppisljar changed the title removing kibana_datatable in favor of datatable` removing kibana_datatable in favor of datatable Oct 14, 2020
@botelastic botelastic bot added Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame labels Oct 14, 2020
@ppisljar
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Overall code LGTM here (I focused mainly on the app arch files and briefly reviewed the rest).

I updated the baselines for the interpreter functional tests, and as usual there are a few weird things in the screengrabs. Specifically:

test/interpreter_functional/screenshots/baseline/metric_invalid_data.png

  • It feels like we had the wrong baseline here originally, as the added one from this PR looks like what I would expect.

test/interpreter_functional/screenshots/baseline/partial_test_3.png

  • Map controls are now missing from this screengrab, not sure why?

Also on all of the screengrabs, the dimensions changed slightly after running on my machine. Not sure if this is correct; I guess we can wait and see if they pass.

Based on how finicky the screengrabs have been historically, I think we should continue to avoid adding new ones and prefer the snapshots instead whenever possible.

const { indexPatternId, ...aggConfigs } = column.meta.sourceParams;
const indexPattern = await getIndexPatterns().get(indexPatternId);
const aggConfigsInstance = getSearchService().aggs.createAggConfigs(indexPattern, [
aggConfigs as AggConfigSerialized,
Copy link
Member

Choose a reason for hiding this comment

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

So I guess we need to cast here because the only constraints on the sourceParams is that it's serializable?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe down the road we could look into introducing more specific typings for different sources. So that someone could do something like:

column.meta.sourceParams as EsaggsDatatableSourceParams

I guess this doesn't matter a ton though with the plan of moving toward utility functions, as most folks won't need to worry about these types at all.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
data 568 566 -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 48493 48492 -1
oss 29211 29210 -1

page load bundle size

id before after diff
data 1.1MB 1.1MB +1.7KB
expressions 198.5KB 196.2KB -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.2KB 272.2KB +62.0B
total -749.0B

History

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and still seems to work fine in Lens

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Canvas changes look good 👍

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

I guess the updated baselines worked! Code LGTM

@ppisljar ppisljar merged commit c90daba into elastic:master Oct 16, 2020
ppisljar added a commit to ppisljar/kibana that referenced this pull request Oct 16, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
* master: (43 commits)
  [ML] Transforms: Fix tab ids for expanded row. (elastic#80666)
  server logs config paths to use for runner (elastic#52980)
  Fix audit logger logging to console even when disabled (elastic#80928)
  skip flaky suite (elastic#80929)
  Added Enterprise Search config to kibana-docker (elastic#80872)
  skip flaky suite (elastic#80914)
  [keystore_cli] parse values as JSON before adding to keystore (elastic#80848)
  [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742)
  ECS audit logging (elastic#74640)
  [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215)
  [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854)
  Move renderHeaderActions back into mount useEffect + update tests (elastic#80861)
  [Reporting] Document Network Policy configuration (elastic#80431)
  [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782)
  Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757)
  [Actions] Back Button on Add Connector Flyout (elastic#80160)
  removing `kibana_datatable` in favor of `datatable`  (elastic#80548)
  [Alerting UI] Updating 'Add new' wording (elastic#80509)
  [Docs] Document Encrypted Saved Objects functionality. (elastic#80183)
  [Discover] fix auto-refresh (elastic#80635)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
* master: (23 commits)
  [ML] Transforms: Fix tab ids for expanded row. (elastic#80666)
  server logs config paths to use for runner (elastic#52980)
  Fix audit logger logging to console even when disabled (elastic#80928)
  skip flaky suite (elastic#80929)
  Added Enterprise Search config to kibana-docker (elastic#80872)
  skip flaky suite (elastic#80914)
  [keystore_cli] parse values as JSON before adding to keystore (elastic#80848)
  [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742)
  ECS audit logging (elastic#74640)
  [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215)
  [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854)
  Move renderHeaderActions back into mount useEffect + update tests (elastic#80861)
  [Reporting] Document Network Policy configuration (elastic#80431)
  [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782)
  Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757)
  [Actions] Back Button on Add Connector Flyout (elastic#80160)
  removing `kibana_datatable` in favor of `datatable`  (elastic#80548)
  [Alerting UI] Updating 'Add new' wording (elastic#80509)
  [Docs] Document Encrypted Saved Objects functionality. (elastic#80183)
  [Discover] fix auto-refresh (elastic#80635)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants