Remove nested table splits from table vis #26057

lukeelmers · 2018-11-22T00:29:57Z

Closes #24560
In preparation for #16639

This restricts the data table vis to only allow one split bucket agg, making tables consistent with other vis types in Kibana. There are a number of reasons that making this change desirable, perhaps most notably that we no longer need to maintain a bunch of custom code for one specific vis type (for example, the table vis uses its own response handler rather than the response handler shared by other vis types).

A saved object migration is included so that existing saved visualizations won't be broken. Rather, they'll be converted so that any non first level split table bucket becomes a split row bucket. This update qualifies as a breaking change because it will change the structure & look of a table vis for any users who are using visualizations with nested tables.

QA

To test the migration, I've been using the following steps:

Create a new table vis using kibana_sample_data_logs & note the vis ID in the URL... you will need this later.
Navigate to Mangement > Saved Objects and find the vis you just saved.
Replace the contents of the visState box with the following & hit "Save":

{
  "title": "[Test] Deeply Nested Table",
  "type": "table",
  "params": {
    "perPage": 10,
    "showPartialRows": false,
    "showMetricsAtAllLevels": false,
    "sort": {
      "columnIndex": null,
      "direction": null
    },
    "showTotal": false,
    "totalFunc": "sum"
  },
  "aggs": [
    {
      "id": "1",
      "enabled": true,
      "type": "count",
      "schema": "metric",
      "params": {}
    },
    {
      "id": "2",
      "enabled": true,
      "type": "terms",
      "schema": "split",
      "params": {
        "field": "tags.keyword",
        "size": 5,
        "order": "desc",
        "orderBy": "1",
        "otherBucket": false,
        "otherBucketLabel": "Other",
        "missingBucket": false,
        "missingBucketLabel": "Missing",
        "row": true
      }
    },
    {
      "id": "3",
      "enabled": true,
      "type": "terms",
      "schema": "split",
      "params": {
        "field": "machine.os.keyword",
        "size": 5,
        "order": "desc",
        "orderBy": "1",
        "otherBucket": false,
        "otherBucketLabel": "Other",
        "missingBucket": false,
        "missingBucketLabel": "Missing",
        "row": false
      }
    },
    {
      "id": "4",
      "enabled": true,
      "type": "terms",
      "schema": "bucket",
      "params": {
        "field": "geo.src",
        "size": 5,
        "order": "desc",
        "orderBy": "1",
        "otherBucket": false,
        "otherBucketLabel": "Other",
        "missingBucket": false,
        "missingBucketLabel": "Missing"
      }
    }
  ]
}

Note how in the list of aggs above, there are two with schema: "split"... that's what we are changing here: only allowing one schema: "split" in a table vis, and changing the others to schema: "bucket".

With ES still running, restart the kibana server.
Navigate back to Management > Saved Objects and note that the visState has been changed to only contain one agg with schema: "split".

You can also go to the dev tools console and look up the vis object by id:
GET .kibana/_doc/visualization:{id}
The migrationVersion in the response should read visualization: 7.0.0

To retest, repeat steps 2 & 3 above, then use the dev tools to change the migrationVersion back to anything pre-7.0:

POST .kibana/_update/visualization:{id}
{
  "doc": {
    "migrationVersion": {
      "visualization": "6.6.0"
    }
  }
}

Note: Once #23650 is resolved, the doc property should be omitted from the body of the _update request above.

Restarting the kibana server will now re-run the migration.

Remaining tasks

Test with embedded visualizations in dashboards
Write unit tests for the migration
Update existing tests
Remove table vis custom response handler (Edit: will be done in Pipeline field formatting #28746)
Investigate URL params in state_management: if you save the url to a nested table vis, and navigate to it after the migration is run, you still see the old nested table since the values are read from the URL params. Re-loading the vis via the vis selection screen will load the proper visualization. (Edit: Resolved by migrating app state, similar to the dashboard implementation).

Checklist

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Release notes

release-note: Removes nested table splits from data table visualizations.

Previously a user was able to create multiple "split table" aggregations in one table on arbitrary levels. This required nesting tables within table cells, within other table cells. The potential for deeply nested tables posed maintenance and accessibility challenges, and was inconsistent with other visualizations which only allow one table split.

This change does not limit the number of split rows a user can put into a table; it only restricts the number of split tables to 1.

To prevent breaking existing visualizations, saved objects containing nested table splits have been migrated to contain only one split table, converting any existing split tables beyond the first to split rows. This preserves all existing visualizations, but will cause data table visualizations with multiple split tables to display differently than they had previously.

elasticmachine · 2018-11-22T00:33:49Z

Pinging @elastic/kibana-app

This makes the behavior of table vis consistent with other vis types.

This migration modifies any table visualizations that have more than one split agg ("nested tables") to ensure they only have a maximum of 1 split. Other aggs are converted to bucket aggs.

elasticmachine · 2019-01-15T22:54:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisdavies

I almost gave this an LGTM w/ tweaks, because I like it. Good commenting throughout, and code that is easy to read. But it seems like there are some changes that probably need to be made, particularly around failed migrations.

src/legacy/core_plugins/kibana/public/visualize/editor/editor.js

src/legacy/core_plugins/kibana/public/visualize/editor/lib/migrate_app_state.js

src/legacy/core_plugins/table_vis/index.js

src/legacy/core_plugins/table_vis/migrations.js

elasticmachine · 2019-01-16T20:47:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers · 2019-01-18T00:26:07Z

After discussion with @chrisdavies and @timroes, we decided to intentionally throw an error if something goes wrong during the migration.

We determined there are pros and cons to both approaches (failing silently vs failing fast). On the one hand, this migration is primarily for data consistency, as a vis will still render as expected (with nesting deprecated) regardless of whether the migration is successfully applied.

However, we felt the most likely cause of a migration failure for this specific instance would be a malformed visState string (and trouble parsing it to JSON). Since that would cause a host of other issues in Kibana, we felt it best to fail during server startup when the migration is running, with a clear error message pointing to the problematic object.

I've pushed an update with this change; here is an example of the error output you would receive during a failed migration (note that the migrations process already logs a warning, but then we throw an error with a stack trace to the specific migration):

elasticmachine · 2019-01-18T01:07:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

trevan · 2019-01-22T17:29:13Z

src/legacy/core_plugins/kibana/public/visualize/editor/lib/migrate_app_state.js

+ *
+ * @param appState {AppState} AppState class to instantiate
+ */
+export function migrateAppState(appState) {


It would be nice if there was a registry so plugins that create visualizations can migrate their app state as well.

@trevan Your timing is perfect; I just opened #29122 to discuss this exact issue 😃

Oh wait, you're talking about appState -- I misread. That's a good idea, we have only used this one other place (dashboards), but it's good to hear this is something you feel would be useful to expose to plugins too!

Perhaps we could implement a solution that mimics whatever resolution we come to for #29122.

elasticmachine · 2019-01-22T18:26:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisdavies

LGTM. I didn't pull down or test, just mainly looked over the migrations side of it, which seems reasonable to me.

timroes

I think we can still delete the legacy_response_handler file for the table vis, it shouldn't be used anymore? Otherwise that LGTM. I tried creating a table vis with nested splits on master and than switched to the PR. Looks like the migration and the app state migrations are working properly.

elasticmachine · 2019-01-25T17:11:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

gchaps · 2019-02-11T18:45:21Z

@lukeelmers Could you please add the description of this issue to the breaking changes doc?

…lastic#30734)

#30886)

lukeelmers added release_note:breaking WIP Work in progress Feature:Data Table Data table visualization feature Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Nov 22, 2018

This comment has been minimized.

Sign in to view

lukeelmers force-pushed the feat/deprecate-table-nesting branch from 9f15bb8 to 7d4ba44 Compare December 12, 2018 00:41

This comment has been minimized.

Sign in to view

lukeelmers added the v7.0.0 label Dec 12, 2018

lukeelmers changed the title ~~Remove nested table splits from table vis~~ [WIP] Remove nested table splits from table vis Dec 12, 2018

This comment has been minimized.

Sign in to view

lukeelmers force-pushed the feat/deprecate-table-nesting branch from 3ff4856 to 5cfe5cc Compare January 2, 2019 21:28

This comment has been minimized.

Sign in to view

lukeelmers added 9 commits January 15, 2019 10:59

Enforce a max of 1 split agg in table vis.

cd703c2

This makes the behavior of table vis consistent with other vis types.

7.0 migration for saved table visualizations.

5e407a3

This migration modifies any table visualizations that have more than one split agg ("nested tables") to ensure they only have a maximum of 1 split. Other aggs are converted to bucket aggs.

Add unit tests.

3c8b094

Fix failing API integration tests.

ec32e82

Add migrationVersion to a few more failing tests.

b31174c

Update another saved object test.

cc55e16

Switch table vis to using default legacy response handler.

81e1479

Remove unneeded migrationVersion from a couple tests.

9e4207b

Remove nested split table functional test.

f01f3ac

lukeelmers force-pushed the feat/deprecate-table-nesting branch from 5cfe5cc to f01f3ac Compare January 15, 2019 19:39

lukeelmers changed the title ~~[WIP] Remove nested table splits from table vis~~ Remove nested table splits from table vis Jan 15, 2019

lukeelmers added review and removed WIP Work in progress labels Jan 15, 2019

This comment has been minimized.

Sign in to view

lukeelmers added 2 commits January 15, 2019 15:13

Ensure bookmarked URLs do not render invalid appState.

fd7f515

Fix test.

2990447

chrisdavies suggested changes Jan 16, 2019

View reviewed changes

Address first set of feedback.

f56046d

This comment has been minimized.

Sign in to view

Return early if not split agg.

275f273

lukeelmers requested a review from timroes January 17, 2019 19:21

Throw error when vis migrations fail.

55660b1

lukeelmers mentioned this pull request Jan 22, 2019

[Discuss] Solution for multiple plugins using same migration type. #29122

Closed

trevan reviewed Jan 22, 2019

View reviewed changes

Resolve merge conflicts with master

7f4de81

chrisdavies approved these changes Jan 23, 2019

View reviewed changes

timroes approved these changes Jan 25, 2019

View reviewed changes

Remove legacy response handler.

8107b28

lukeelmers merged commit c6f0595 into elastic:master Jan 25, 2019

lukeelmers deleted the feat/deprecate-table-nesting branch January 25, 2019 17:13

lukeelmers mentioned this pull request Feb 11, 2019

Add breaking changes documentation for table nesting (#26057) #30734

Merged

lukeelmers added a commit that referenced this pull request Feb 12, 2019

Add breaking changes documentation for table nesting (#26057) (#30734)

bb20120

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Feb 12, 2019

Add breaking changes documentation for table nesting (elastic#26057) (e…

2f6c9a9

…lastic#30734)

lukeelmers mentioned this pull request Feb 12, 2019

[7.0] Add breaking changes documentation for table nesting (#26057) (#30734) #30886

Merged

lukeelmers added a commit that referenced this pull request Feb 13, 2019

Add breaking changes documentation for table nesting (#26057) (#30734) (

c38a299

#30886)

tylersmalley mentioned this pull request Dec 10, 2019

Moving saved object management to an ES plugin #49764

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove nested table splits from table vis #26057

Remove nested table splits from table vis #26057

lukeelmers commented Nov 22, 2018 •

edited

Loading

elasticmachine commented Nov 22, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Jan 15, 2019

chrisdavies left a comment

This comment has been minimized.

elasticmachine commented Jan 16, 2019

lukeelmers commented Jan 18, 2019

elasticmachine commented Jan 18, 2019

trevan Jan 22, 2019

lukeelmers Jan 22, 2019

lukeelmers Jan 22, 2019 •

edited

Loading

elasticmachine commented Jan 22, 2019

chrisdavies left a comment

timroes left a comment

elasticmachine commented Jan 25, 2019

gchaps commented Feb 11, 2019

Remove nested table splits from table vis #26057

Remove nested table splits from table vis #26057

Conversation

lukeelmers commented Nov 22, 2018 • edited Loading

QA

Remaining tasks

Checklist

For maintainers

Release notes

elasticmachine commented Nov 22, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Jan 15, 2019

💚 Build Succeeded

chrisdavies left a comment

Choose a reason for hiding this comment

This comment has been minimized.

elasticmachine commented Jan 16, 2019

💚 Build Succeeded

lukeelmers commented Jan 18, 2019

elasticmachine commented Jan 18, 2019

💚 Build Succeeded

trevan Jan 22, 2019

Choose a reason for hiding this comment

lukeelmers Jan 22, 2019

Choose a reason for hiding this comment

lukeelmers Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jan 22, 2019

💚 Build Succeeded

chrisdavies left a comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

elasticmachine commented Jan 25, 2019

💚 Build Succeeded

gchaps commented Feb 11, 2019

lukeelmers commented Nov 22, 2018 •

edited

Loading

lukeelmers Jan 22, 2019 •

edited

Loading