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

Timelion App removal #110255

Merged
merged 28 commits into from
Sep 10, 2021
Merged

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Aug 26, 2021

Closes: #100987, Closes #71624, Closes #14624, Closes #10532

Summary

What have been done:

  • Removed timelion plugin
  • Removed some field from configSchema in vis_type_timelion which related only to timelion app
  • Removed advanced settings which related only to timelion app
  • Removed timelion-sheet saved objects from json's which we use for functional tests
  • Rewrote functional tests related to expression typeahead so that we can test timelion vis
  • Removed timelion privilage because it using only for timelion app
  • Removed everthing which related to timelion app from API and different docs

Checklist

Delete any items that are not applicable to this PR.

  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor

alexwizp commented Aug 30, 2021

  • src/core/public/doc_links/doc_links_service.ts
    Should this be removed?
    image

@alexwizp
Copy link
Contributor

alexwizp commented Aug 30, 2021

  • src/plugins/vis_type_timelion/server/ui_settings.ts

@stratoula This message says that it should be removed in version 8.0. I think it's an issue cause if I'm not wrong we planned to do it in 8.1/2?

image

@stratoula
Copy link
Contributor

@alexwizp yes you are right. Can we change it in another PR?

@alexwizp
Copy link
Contributor

  • src/plugins/index_pattern_editor/public/components/rollup_beta_warning/rollup_beta_warning.tsx
    Should we update the following message?

image

@elastic elastic deleted a comment from alexwizp Aug 30, 2021
@alexwizp
Copy link
Contributor

alexwizp commented Aug 30, 2021

  • x-pack/test/functional/es_archives/action_task_params/mappings.json
    remove plz "timelion-sheet"

@alexwizp
Copy link
Contributor

  • src/dev/code_coverage/docs/teams_scripted_field.painless
    Please check that file

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

  • src/dev/code_coverage/docs/teams_scripted_field.painless
    Please check that file

We should left here timelion because here it as I understand related to vis_type_timelion too.

@elastic elastic deleted a comment from kibanamachine Sep 1, 2021
@VladLasitsa VladLasitsa requested a review from alexwizp September 1, 2021 12:56
Copy link
Contributor

@estermv estermv left a comment

Choose a reason for hiding this comment

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

Stack Monitoring changes LGTM

@stratoula stratoula requested a review from timductive September 6, 2021 15:44
Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

Ok for the Core changes

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Vis editors team code changes LGTM!

  • Although I had timelion-sheets, kibana didn't fail. I don't see the removed SO type to the SO management page but I can retrieve my timelion expression by the .kibana index.
  • The whitelisted timelion setting has been removed in cloud https://github.com/elastic/cloud/pull/88014
  • We are not going to update the docs with a section on how to retrieve your SOs from .kibana index. I will create a known-issue instead.

Very well done @VladLasitsa 👏 This PR feels good :)

@LeeDr
Copy link

LeeDr commented Sep 8, 2021

2 questions:

  1. I didn't see any mention here of Canvas. I'm assuming Timelion expressions are still valid in Canvas? Did anybody test to make sure it still works there?
  2. If this is a breaking change why do we have a v7.16.0 label? We would usually only make breaking changes in a major release.

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@watson watson left a comment

Choose a reason for hiding this comment

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

Went over all the import statements to see if any of the packages used by timelion was no longer needed after timelion had been removed

import 'angular-sanitize';
// required for ngRoute
import 'angular-route';
import 'angular-sortable-view';
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can see, this package isn't used anywhere else in Kibana and can be removed from the package.json file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@watson 'angular-sanitize' and 'angular-route' are using in x-pack\plugins\monitoring\public\angular\app_modules.ts. We cannot remove them for now. I can remove only 'angular-sortable-view'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, yean I meant just angular-sortable-view. I guess the diff above just shows a bit more than what I'm commenting on 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've removed angular-sortable-view

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

Reporting test archive changes LGTM

@stratoula
Copy link
Contributor

2 questions:

  1. I didn't see any mention here of Canvas. I'm assuming Timelion expressions are still valid in Canvas? Did anybody test to make sure it still works there?
  2. If this is a breaking change why do we have a v7.16.0 label? We would usually only make breaking changes in a major release.
  1. @LeeDr we don't remove timelion in general. Timelion viz and expressions will work as expected, Only the timelion app is been removed!
  2. The reason we decided to do it in 7.16 is due to its angular usage. But we took all the necessary steps to ensure that we are in sync with the #make-it-minor initiative. We hid the app from the app switcher on v7.0.0 Timelion/hide timelion #30131, we added a deprecation warning both in UI and server-side log output (when the app is enabled) on v7.10.0 Add Timelion deprecation warning to server log #75541 Timelion app deprecation warning #74660 and we also had documentation on how to migrate your sheets to dashboards. Moreover, telemetry indicates that a very small portion of users actually use the timelion app. :)

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM,

Looked through the code owners section and saw that it's just against one file of Cypress so we should be good as it's a mapping change. Cypress tests looked to have run so I think we are good here.

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
timelion 66 - -66

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
visTypeTimelion 21 2 -19

Async chunks

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

id before after diff
timelion 185.0KB - -185.0KB
visTypeTimelion 137.7KB 145.9KB +8.2KB
total -176.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
visTypeTimelion 5 2 -3

Page load bundle

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

id before after diff
core 408.4KB 408.4KB -48.0B
timelion 9.8KB - -9.8KB
visTypeTimelion 25.5KB 19.4KB -6.1KB
total -16.0KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
timelion-sheet 13 - -13
Unknown metric groups

API count

id before after diff
visTypeTimelion 22 2 -20

async chunk count

id before after diff
timelion 1 - -1

References to deprecated APIs

id before after diff
timelion 6 - -6

History

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

cc @VladLasitsa

@VladLasitsa VladLasitsa merged commit 70090e3 into elastic:master Sep 10, 2021
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Sep 10, 2021
* Remove timelion app and stuff which related to it

* Fix CI

* Fix lint

* Fix tests

* Fix tests

* Fis tests

* Fix some comments

* Clean up

* fix CI

* fix some comments

* Fix deprecation examples

* Return `enabled` property in config for timelion vis

* Remove unused angular lib

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
VladLasitsa added a commit that referenced this pull request Sep 10, 2021
* Timelion App removal (#110255)

* Remove timelion app and stuff which related to it

* Fix CI

* Fix lint

* Fix tests

* Fix tests

* Fis tests

* Fix some comments

* Clean up

* fix CI

* fix some comments

* Fix deprecation examples

* Return `enabled` property in config for timelion vis

* Remove unused angular lib

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>

* Fix CI

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet