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.

Sign up for GitHub

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

Jump to bottom

[Timeseries] remove unused configuration properties #62543

Merged
merged 15 commits into from
Apr 16, 2020
Merged

[Timeseries] remove unused configuration properties #62543

merged 15 commits into from
Apr 16, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Apr 4, 2020 •
edited
Loading

Dev-Docs

Mark chartResolution and minimumBucketSize as deprecated configuration properties for TSVB 👋

In #9725 TSVB visualization was added to Kibana. Also in this PR we can find that 2 configuration properties were declared: chartResolution and minimumBucketSize. The problem is that until today no one has started using these properties, an implementation has not been added

@alexwizp alexwizp self-assigned this Apr 4, 2020
@alexwizp alexwizp added the Feature:TSVB TSVB (Time Series Visual Builder) label Apr 4, 2020
@alexwizp alexwizp added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Apr 4, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@alexwizp alexwizp added v7.8.0 v8.0.0 release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Apr 4, 2020
@flash1293
Copy link
Contributor

I don't feel comfortable removing these in a minor version - if an installation still has these settings set, Kibana won't start for them. My suggestion:

This will have the following effect when a user still has these settings: Kibana will still start but log a warning that the setting is unused. With 8.0 we can remove the setting completely and add it to the breaking changes list.

@alexwizp alexwizp marked this pull request as ready for review April 6, 2020 12:22
@alexwizp alexwizp requested a review from a team as a code owner April 6, 2020 15:43
@alexwizp
Copy link
Contributor Author

alexwizp commented Apr 6, 2020

@elasticmachine merge upstream

@cjcenizal cjcenizal self-requested a review April 6, 2020 17:47
cjcenizal
cjcenizal approved these changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Tested the changes to the Rollup plugin locally, code LGTM.

@flash1293
Copy link
Contributor

This test failure is probably legit, the infra app has a dependency on the metrics/vis_type_timeseries plugin. The reference has to be renamed.

@alexwizp alexwizp requested a review from a team as a code owner April 7, 2020 07:03
@alexwizp alexwizp mentioned this pull request Apr 7, 2020
Merged
1 task
@alexwizp
Copy link
Contributor Author

alexwizp commented Apr 7, 2020

ping @elastic/logs-metrics-ui

@timroes timroes mentioned this pull request Apr 8, 2020
Open
@alexwizp
Copy link
Contributor Author

alexwizp commented Apr 8, 2020

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

flash1293 commented Apr 9, 2020 •
edited
Loading

I tested this PR and it looks and works fine - one thing I noticed though is that if you just use visTypeTimeseries.enabled: false, then the server will be gone but the visualization will still show up because it's still in the legacy plugin (you would have to use it along with metrics.enabled: false to disable everything).

@alexwizp what do you think about parking this PR till we moved the rest of timeseries over? This will very likely happen next week as there are no more blockers.

@alexwizp
Copy link
Contributor Author

alexwizp commented Apr 9, 2020

@flash1293 np, let's parking it =)

@alexwizp
Merge remote-tracking branch 'upstream/master' into config_tsvb
90c6b05 
# Conflicts:
#	src/legacy/core_plugins/vis_type_timeseries/index.ts
#	x-pack/legacy/plugins/rollup/server/plugin.ts
@alexwizp alexwizp requested a review from a team April 15, 2020 16:26
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

flash1293
flash1293 approved these changes Apr 16, 2020
View reviewed changes
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.

LGTM, bye bye metrics plugin

Zacqary
Zacqary approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

Looks good from the infra side

@alexwizp alexwizp merged commit 871f720 into elastic:master Apr 16, 2020
@alexwizp alexwizp mentioned this pull request Apr 16, 2020
Merged
alexwizp added a commit to alexwizp/kibana that referenced this pull request Apr 16, 2020
@alexwizp @elasticmachine
[Timeseries] remove unused configuration properties (elastic#62543)
b504ade 
* [Timeseries] remove unused configuration properties

* Fix PR comments

* update id of vis_type_timeseries plugin

* metrics -> vis_type_timeseries

* fix wrong plugin id

* update requiredPliugins for infra/kibana.json

* change id

* update plugin id in infra folder

Co-authored-by: Elastic Machine <[email protected]>
alexwizp added a commit that referenced this pull request Apr 16, 2020
@alexwizp @elasticmachine
[Timeseries] remove unused configuration properties (#62543) (#63728)
7e60404 
* [Timeseries] remove unused configuration properties

* Fix PR comments

* update id of vis_type_timeseries plugin

* metrics -> vis_type_timeseries

* fix wrong plugin id

* update requiredPliugins for infra/kibana.json

* change id

* update plugin id in infra folder

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flash1293 flash1293 flash1293 approved these changes

@Zacqary Zacqary Zacqary approved these changes

@cjcenizal cjcenizal cjcenizal approved these changes

@VladLasitsa VladLasitsa Awaiting requested review from VladLasitsa

Assignees

@alexwizp alexwizp

Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexwizp @elasticmachine @flash1293 @kibanamachine @cjcenizal @Zacqary