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

Convert to using new status, and remove legacy plugin #75806

Merged
merged 7 commits into from
Aug 31, 2020

Conversation

chrisronline
Copy link
Contributor

Refers to #75503

Now that the status API is now available, we can deprecate the legacy monitoring plugin entirely and leverage the new status API to get status information.

@chrisronline chrisronline added review Team:Monitoring Stack Monitoring team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 24, 2020
@chrisronline chrisronline requested a review from a team August 24, 2020 18:24
@chrisronline chrisronline self-assigned this Aug 24, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring (Team:Monitoring)

this.kibanaStatusGetter = getter;
this.kibanaStatus = null;
this.kibanaStatusGetter$ = statusGetter$.subscribe((nextStatus) => {
this.kibanaStatus = nextStatus.level.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: instead of converting to a string, you could import ServiceStatusLevels from core/server and do comparisons below:

if (this.kibanaStatus === ServiceStatusLevels.available) { }
// etc

This will make refactoring easier if we ever decide to change the names of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, great point. I had assumed incorrectly I couldn't import it because it was a typescript interface but it's just a constant.

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

distributable file count

id value diff baseline
total 44371 -2 44373

History

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


setKibanaStatusGetter(getter) {
this.kibanaStatusGetter = getter;
this.kibanaStatus = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think you need to do this, since the constructor is only executed once (and the default value is already undefined)

Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

Happy to see this change 👍

@chrisronline chrisronline merged commit 5492652 into elastic:master Aug 31, 2020
@chrisronline chrisronline deleted the monitoring/remove_legacy branch August 31, 2020 15:42
chrisronline added a commit that referenced this pull request Aug 31, 2020
* Convert to using new status, and remove legacy plugin

* PR feedback

* Fix test

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	.github/CODEOWNERS
@chrisronline
Copy link
Contributor Author

Backport:

7.x: ed93d27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes review Team:Monitoring Stack Monitoring team v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants