[Vislib] Removes old implementation of xy chart #110786

stratoula · 2021-09-01T11:53:19Z

Summary

Bugs related to the vislib implementation:
Closes #94314
Closes #78113
Closes #63685
Closes #31071
Closes #26752

This PR removes the vislib implementation of the XY axis charts. Specifically:

Removes the deprecated advanced setting Legacy charts library
Cleans up the xy plugin
Cleans up the vislib plugin
Refactors all the functional tests that ran with the vislib implementation to work with the es-charts
Cleans up the functional tests helpers
Fixes a bug with shard_delay to the es-charts implementation

In 7.15 all the users that were using the vislib implementation were notified that they should switch to the new one.

Even if they haven't done it, their charts will be automatically use the new implementation.

Release note

We removed the legacy charts library for area, bar and line charts of the aggregation-based visualizations.

Checklist

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

stratoula · 2021-09-03T06:00:14Z

@elasticmachine merge upstream

stratoula · 2021-09-06T07:44:50Z

@KOTungseth @gchaps I have added the breaking change label. Do you think that the release note is ok? Do you want me to do anything else regarding the docs? Just a note here, although we remove this implementation (and the advanced setting) this is not a breaking change. Even if the users use the old implementation, their charts will automatically change to use the new one. But still, I think that we should label it as a breaking change and inform the users about this removal.

@nickofthyme I have added you as a reviewer mostly for another 👁️ on the xy plugin.

stratoula · 2021-09-06T07:49:05Z

@elasticmachine merge upstream

elasticmachine · 2021-09-06T11:41:13Z

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

src/plugins/vis_types/vislib/public/vislib/VISLIB.md

test/functional/apps/dashboard/dashboard_state.ts

mbondyra · 2021-09-06T13:38:32Z

Tested by:

checking if advanced setting is not displayed in UI
importing a chart from 6.8 (through intermediate step for 7.13 because JSON lost support in the meantime). So 6.8 -> 7.13 -> master
importing a chart with legacy check on from 7.14.
importing a chart with legacy check off from 7.14.
checked a dashboard with 4 legacy charts from 7.13

All works as expected, the charts are gone and I couldn't break the UI in any way. Great job! 🎉

...ck/test/search_sessions_integration/tests/apps/dashboard/async_search/save_search_session.ts

ppisljar

app services changes LGTM

nickofthyme

FINALLY!! No more xy ~~vislib~~ 🎉 🍾 🥳 . I bet it felt amazing to deleting all that code. Well done.

It looks like you captured all the flag conditions that I remember, hopefully I left enough breadcrumbs for you to find them all.

What a relief to have all the PageObjects.visChart.getExpectedValue 💩 out of the function tests!

I left a few remarks and suggestions but all code changes LGTM.

nickofthyme · 2021-09-07T17:34:24Z

src/plugins/vis_types/vislib/public/vislib/lib/dispatch.test.js

@@ -8,6 +8,7 @@

 import _ from 'lodash';
 import d3 from 'd3';
+import $ from 'jquery';


nickofthyme · 2021-09-07T17:35:52Z

src/plugins/vis_types/vislib/public/vislib/lib/types/index.js

-  histogram: pointSeries.column,
-  horizontal_bar: pointSeries.column,
-  line: pointSeries.line,
  pie: vislibPieConfig,
-  area: pointSeries.area,
-  point_series: pointSeries.line,


src/plugins/vis_types/xy/public/editor/common_config.tsx

src/plugins/vis_types/xy/public/vis_types/index.ts

test/functional/apps/visualize/_line_chart_split_series.ts

test/functional/apps/visualize/index.ts

nickofthyme · 2021-09-07T18:07:16Z

test/functional/page_objects/visualize_chart_page.ts

-  public async getExpectedValue<T>(vislibValue: T, elasticChartsValue: T): Promise<T> {
-    if (await this.isNewLibraryChart(xyChartSelector)) {
-      return elasticChartsValue;
-    }
-
-    return vislibValue;
-  }


❤️❤️❤️❤️❤️❤️❤️❤️

test/functional/page_objects/visualize_editor_page.ts

test/functional/page_objects/visualize_page.ts

KOTungseth · 2021-09-07T20:51:03Z

@KOTungseth @gchaps I have added the breaking change label. Do you think that the release note is ok? Do you want me to do anything else regarding the docs? Just a note here, although we remove this implementation (and the advanced setting) this is not a breaking change. Even if the users use the old implementation, their charts will automatically change to use the new one. But still, I think that we should label it as a breaking change and inform the users about this removal.

@nickofthyme I have added you as a reviewer mostly for another 👁️ on the xy plugin.

@stratoula I put this under Deprecations in the 7.15.0 release notes. Does this work?

stratoula · 2021-09-08T06:09:21Z

@elasticmachine merge upstream

stratoula · 2021-09-08T06:10:11Z

@KOTungseth @gchaps I have added the breaking change label. Do you think that the release note is ok? Do you want me to do anything else regarding the docs? Just a note here, although we remove this implementation (and the advanced setting) this is not a breaking change. Even if the users use the old implementation, their charts will automatically change to use the new one. But still, I think that we should label it as a breaking change and inform the users about this removal.
@nickofthyme I have added you as a reviewer mostly for another 👁️ on the xy plugin.

@stratoula I put this under Deprecations in the 7.15.0 release notes. Does this work?

@KOTungseth what do we mention in the 7.15 docs? The removal will happen in 7.16.
From my point of view, 7.15 should mention the removal that will happen in 7.16 and 7.16 should mention the actual removal or something like that :D

ThomThomson

Presentation team changes LGTM! Code only review

kibanamachine · 2021-09-08T15:30:42Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: b21a62c
Storybooks Preview
Documentation Changes
Flaky suites:
- xpack-kibana-ciGroup5

Test Failures

Kibana Pipeline / general / X-Pack Alerting API Integration Tests.x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/get_alert_state·ts.alerting api integration security and spaces enabled Alerts alerts getAlertState "after each" hook for "should handle getAlertState alert request appropriately when unauthorized"

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

[00:00:00]       │
[00:00:00]         └-: alerting api integration security and spaces enabled
[00:00:00]           └-> "before all" hook in "alerting api integration security and spaces enabled"
[00:04:03]           └-: Alerts
[00:04:03]             └-> "before all" hook in "Alerts"
[00:05:28]             └-: alerts
[00:05:28]               └-> "before all" hook in "alerts"
[00:05:28]               └-> "before all" hook in "alerts"
[00:05:28]                 │ debg creating space
[00:05:29]                 │ debg created space
[00:05:29]                 │ debg creating space
[00:05:30]                 │ debg created space
[00:05:30]                 │ debg creating space
[00:05:31]                 │ debg created space
[00:05:31]                 │ debg creating user no_kibana_privileges
[00:05:31]                 │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [no_kibana_privileges]
[00:05:31]                 │ debg created user no_kibana_privileges
[00:05:31]                 │ debg creating role no_kibana_privileges
[00:05:31]                 │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [no_kibana_privileges]
[00:05:31]                 │ debg creating user superuser
[00:05:31]                 │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [superuser]
[00:05:31]                 │ debg created user superuser
[00:05:31]                 │ debg creating user global_read
[00:05:31]                 │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [global_read]
[00:05:31]                 │ debg created user global_read
[00:05:31]                 │ debg creating role global_read_role
[00:05:31]                 │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [global_read_role]
[00:05:31]                 │ debg creating user space_1_all
[00:05:31]                 │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [space_1_all]
[00:05:31]                 │ debg created user space_1_all
[00:05:31]                 │ debg creating role space_1_all_role
[00:05:31]                 │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [space_1_all_role]
[00:05:31]                 │ debg creating user space_1_all_with_restricted_fixture
[00:05:31]                 │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [space_1_all_with_restricted_fixture]
[00:05:31]                 │ debg created user space_1_all_with_restricted_fixture
[00:05:31]                 │ debg creating role space_1_all_with_restricted_fixture_role
[00:05:31]                 │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [space_1_all_with_restricted_fixture_role]
[00:05:31]                 │ debg creating user space_1_all_alerts_none_actions
[00:05:32]                 │ info [o.e.x.s.a.u.TransportPutUserAction] [node-01] added user [space_1_all_alerts_none_actions]
[00:05:32]                 │ debg created user space_1_all_alerts_none_actions
[00:05:32]                 │ debg creating role space_1_all_alerts_none_actions_role
[00:05:32]                 │ info [o.e.x.s.a.r.TransportPutRoleAction] [node-01] added role [space_1_all_alerts_none_actions_role]
[00:12:34]               └-: getAlertState
[00:12:34]                 └-> "before all" hook in "getAlertState"

Stack Trace

Error: expected 204 "No Content", got 409 "Conflict"
    at Test._assertStatus (/dev/shm/workspace/parallel/9/kibana/node_modules/supertest/lib/test.js:268:12)
    at Test._assertFunction (/dev/shm/workspace/parallel/9/kibana/node_modules/supertest/lib/test.js:283:11)
    at Test.assert (/dev/shm/workspace/parallel/9/kibana/node_modules/supertest/lib/test.js:173:18)
    at assert (/dev/shm/workspace/parallel/9/kibana/node_modules/supertest/lib/test.js:131:12)
    at /dev/shm/workspace/parallel/9/kibana/node_modules/supertest/lib/test.js:128:5
    at Test.Request.callback (/dev/shm/workspace/parallel/9/kibana/node_modules/supertest/node_modules/superagent/lib/node/index.js:718:3)
    at /dev/shm/workspace/parallel/9/kibana/node_modules/supertest/node_modules/superagent/lib/node/index.js:906:18
    at IncomingMessage.<anonymous> (/dev/shm/workspace/parallel/9/kibana/node_modules/supertest/node_modules/superagent/lib/node/parsers/json.js:19:7)
    at endReadableNT (internal/streams/readable.js:1317:12)
    at processTicksAndRejections (internal/process/task_queues.js:82:21)

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visTypeVislib`	207	199	-8
`visualize`	83	82	-1
total			-9

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`visTypeXy`	63	51	-12

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`visTypeTimelion`	137.7KB	137.7KB	-1.0B
`visTypeVislib`	564.9KB	548.3KB	-16.5KB
`visTypeXy`	115.3KB	114.1KB	-1.2KB
`visualize`	89.0KB	87.0KB	-2.1KB
total			-19.8KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`visTypeXy`	6	5	-1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`visTypeVislib`	32.9KB	31.9KB	-1.0KB
`visTypeXy`	63.5KB	62.3KB	-1.2KB
total			-2.3KB

Unknown metric groups

API count

id	before	after	diff
`visTypeXy`	69	57	-12

History

💔 Build #151797 failed 8cb7ee7
💚 Build #151720 succeeded 9f72d6a
💚 Build #151713 succeeded 83613bc
💚 Build #151295 succeeded 6b1ca87
💚 Build #151258 succeeded a16a478

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

* [Vislib] Remove xy chart * Update i18n * Remove uncecessary file * Fix types * More fixes * Fix functional tests part 1 * Fix functional tests part 2 * Fix bug with shard-delay * Fix functional tests part 3 * fix functional tests part4 * Fix async_serch FT * Fix functional dashboard async test * REplace screenshot area chart image * Cleanup vislib from xy charts * Remove unused fixtures * Address PR comments * Remove miaou :D * Address PR comments * Fix i18n Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # docs/management/advanced-options.asciidoc # test/functional/screenshots/baseline/area_chart.png

) * [Vislib] Removes old implementation of xy chart (#110786) * [Vislib] Remove xy chart * Update i18n * Remove uncecessary file * Fix types * More fixes * Fix functional tests part 1 * Fix functional tests part 2 * Fix bug with shard-delay * Fix functional tests part 3 * fix functional tests part4 * Fix async_serch FT * Fix functional dashboard async test * REplace screenshot area chart image * Cleanup vislib from xy charts * Remove unused fixtures * Address PR comments * Remove miaou :D * Address PR comments * Fix i18n Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # docs/management/advanced-options.asciidoc # test/functional/screenshots/baseline/area_chart.png * Fixes * Remove setting from docs Co-authored-by: Kibana Machine <[email protected]>

* [Vislib] Remove xy chart * Update i18n * Remove uncecessary file * Fix types * More fixes * Fix functional tests part 1 * Fix functional tests part 2 * Fix bug with shard-delay * Fix functional tests part 3 * fix functional tests part4 * Fix async_serch FT * Fix functional dashboard async test * REplace screenshot area chart image * Cleanup vislib from xy charts * Remove unused fixtures * Address PR comments * Remove miaou :D * Address PR comments * Fix i18n Co-authored-by: Kibana Machine <[email protected]>

stratoula added 10 commits September 1, 2021 14:52

[Vislib] Remove xy chart

c5a2898

Update i18n

78bf790

Remove uncecessary file

a855318

Fix types

7932d17

More fixes

cd047e4

Fix functional tests part 1

9f61249

Fix functional tests part 2

9be6130

Fix bug with shard-delay

c122421

Fix functional tests part 3

ad936b5

fix functional tests part4

bd6229a

kibanamachine and others added 4 commits September 3, 2021 02:00

Merge branch 'master' into remove-vislib-xy

e9ebe06

Fix async_serch FT

5bdf7c5

Fix functional dashboard async test

7a7b8e8

REplace screenshot area chart image

a90c53a

stratoula changed the title ~~[Vislib] Remove xy chart~~ [Vislib] Removes xy chart Sep 3, 2021

stratoula added 2 commits September 6, 2021 09:19

Cleanup vislib from xy charts

7c7b947

Remove unused fixtures

8929316

stratoula added release_note:breaking Feature:Vislib Vislib chart implementation v7.16.0 v8.0.0 labels Sep 6, 2021

stratoula requested a review from nickofthyme September 6, 2021 07:44

stratoula changed the title ~~[Vislib] Removes xy chart~~ [Vislib] Removes old implementation of xy chart Sep 6, 2021

Merge branch 'master' into remove-vislib-xy

3401ccf

stratoula marked this pull request as ready for review September 6, 2021 10:05

stratoula requested review from a team as code owners September 6, 2021 10:05

stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Sep 6, 2021

mbondyra reviewed Sep 6, 2021

View reviewed changes

src/plugins/vis_types/vislib/public/vislib/VISLIB.md Outdated Show resolved Hide resolved

mbondyra reviewed Sep 6, 2021

View reviewed changes

test/functional/apps/dashboard/dashboard_state.ts Outdated Show resolved Hide resolved

Address PR comments

a16a478

mbondyra approved these changes Sep 6, 2021

View reviewed changes

ppisljar reviewed Sep 6, 2021

View reviewed changes

...ck/test/search_sessions_integration/tests/apps/dashboard/async_search/save_search_session.ts Outdated Show resolved Hide resolved

ppisljar approved these changes Sep 6, 2021

View reviewed changes

Remove miaou :D

6b1ca87

pgayvallet approved these changes Sep 7, 2021

View reviewed changes

MichaelMarcialis approved these changes Sep 7, 2021

View reviewed changes

nickofthyme approved these changes Sep 7, 2021

View reviewed changes

Merge branch 'master' into remove-vislib-xy

83613bc

Address PR comments

9f72d6a

stratoula added 2 commits September 8, 2021 15:58

Merge with master and resolve conflicts

8cb7ee7

Fix i18n

b21a62c

ThomThomson approved these changes Sep 8, 2021

View reviewed changes

stratoula merged commit 6f4d8a5 into elastic:master Sep 8, 2021

stratoula mentioned this pull request Sep 8, 2021

[7.x] [Vislib] Removes old implementation of xy chart (#110786) #111558

Merged

Mayons mentioned this pull request Apr 20, 2022

Support Elastic Stack 7.16.X and 7.17.X all minor versions wazuh/wazuh-dashboard-plugins#4054

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vislib] Removes old implementation of xy chart #110786

[Vislib] Removes old implementation of xy chart #110786

stratoula commented Sep 1, 2021 •

edited

Loading

stratoula commented Sep 3, 2021

stratoula commented Sep 6, 2021

stratoula commented Sep 6, 2021

elasticmachine commented Sep 6, 2021

mbondyra commented Sep 6, 2021 •

edited

Loading

ppisljar left a comment

nickofthyme left a comment

nickofthyme Sep 7, 2021

nickofthyme Sep 7, 2021

nickofthyme Sep 7, 2021

KOTungseth commented Sep 7, 2021

stratoula commented Sep 8, 2021

stratoula commented Sep 8, 2021 •

edited

Loading

ThomThomson left a comment

kibanamachine commented Sep 8, 2021

Standard Out

Stack Trace

API count

[Vislib] Removes old implementation of xy chart #110786

[Vislib] Removes old implementation of xy chart #110786

Conversation

stratoula commented Sep 1, 2021 • edited Loading

Summary

Release note

Checklist

stratoula commented Sep 3, 2021

stratoula commented Sep 6, 2021

stratoula commented Sep 6, 2021

elasticmachine commented Sep 6, 2021

mbondyra commented Sep 6, 2021 • edited Loading

ppisljar left a comment

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme Sep 7, 2021

Choose a reason for hiding this comment

nickofthyme Sep 7, 2021

Choose a reason for hiding this comment

nickofthyme Sep 7, 2021

Choose a reason for hiding this comment

KOTungseth commented Sep 7, 2021

stratoula commented Sep 8, 2021

stratoula commented Sep 8, 2021 • edited Loading

ThomThomson left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 8, 2021

💛 Build succeeded, but was flaky

Test Failures

Standard Out

Stack Trace

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

stratoula commented Sep 1, 2021 •

edited

Loading

mbondyra commented Sep 6, 2021 •

edited

Loading

stratoula commented Sep 8, 2021 •

edited

Loading