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

[Monitoring] Metricbeat migration improvements #42600

Merged

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Aug 5, 2019

This PR is step 4 of the Metricbeat setup wizard.

This PR is addressing issues that came out of testing and the other PRs, specifically the following items:

Add better UX for ES migration if the ES cluster is the one Kibana is talking to

This is working now. To test, first setup internal collection for Elasticsearch, then migrate (using the UI) to Metricbeat. After setting up monitoring through Metricbeat, there will be a callout and button at the top of the Elasticsearch nodes listing page (this isn't new in this PR, fyi). This button will automatically disable internal collection if the monitored cluster is the same one as the one Kibana is talking to. If it isn't, it will open the flyout with instructions. Please test both ways.

Share code better, but ensure it's consistent if possible with our areas of the codebase

This is a little dated, but we now have two routes instead of three. Please let me know if this is not sufficient and you think we should be using a single route.

We should fetch setup data at the same interval as the other data for the page. This will allow the setup mode UIs to automatically update periodically which is more seamless experience for the user. As a result of changing this, we may not need a Check Data button in the flyout anymore. This comment started the discussion.

This is done as well. The Check Data button has been removed and the experience is much more seamless now. Thanks @ycombinator!

Ensure this PR is applied to logstash, beats and APM panels

Done as well.

@ycombinator
Copy link
Contributor

Looks good from the code point of view (with exception of some comments).

Going to test it functionally

@igoristic FWIW I've found that testing the PR functionally first and then reviewing the code changes second is often more efficient because any code changes from the functional review will need to be reviewed anyway.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ycombinator
Copy link
Contributor

ycombinator commented Aug 12, 2019

I've started reviewing this PR. So far the only item I've been able to test is:

Add better UX for ES migration if the ES cluster is the one Kibana is talking to

This works as expected! 👍

However, while trying to test this item, I found two bugs unrelated to this PR: #43144 and #43145.

I will continue to test the rest of this PR but just wanted to report on my progress so far.

@ycombinator
Copy link
Contributor

Share code better, but ensure it's consistent if possible with our areas of the codebase

This change LGTM as well.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ycombinator
Copy link
Contributor

We should fetch setup data at the same interval as the other data for the page.

Tested this and it works as expected. 👍

@ycombinator
Copy link
Contributor

Ensure this PR is applied to logstash, beats and APM panels

This didn't work as expected for Logstash. I ran Logstash with a simple pipeline (input { stdin {} } output { elasticsearch {} }) which ended up creating an index names logstash-2019.08.12-000001. When I go the Cluster Overview page in Setup Mode, I expected to see a flag icon in the Logstash box, but there was none.

@chrisronline
Copy link
Contributor Author

This didn't work as expected for Logstash. I ran Logstash with a simple pipeline (input { stdin {} } output { elasticsearch {} }) which ended up creating an index names logstash-2019.08.12-000001. When I go the Cluster Overview page in Setup Mode, I expected to see a flag icon in the Logstash box, but there was none.

Nice find! I updated the way we detect these which should fix this!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline
Copy link
Contributor Author

@igoristic @ycombinator This is ready for another round!

@ycombinator
Copy link
Contributor

Nice find! I updated the way we detect these which should fix this!

I'm still seeing this issue. Could you double check on your end please? 😄

@chrisronline
Copy link
Contributor Author

@ycombinator This should be addressed now in 7684403

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

Functionally this PR LGTM now. Over to @igoristic for code review.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@igoristic
Copy link
Contributor

Thanks for making the changes! LGTM. Great job!

@chrisronline chrisronline merged commit cb2dd9f into elastic:master Aug 15, 2019
@chrisronline chrisronline deleted the monitoring/mb_setup_wizard_step4 branch August 15, 2019 16:14
chrisronline added a commit to chrisronline/kibana that referenced this pull request Aug 15, 2019
* Support for logstash

* Beats support

* Fix cherry-pick api issue

* Support for logstash

* Updates for beats and logstash

* APM migration working

* Tweaks for beats migration

* Update copy for setup new button

* If on cloud, disable setup mode

* Handle new beat flow better

* Better phrasing for APM

* Add beat type to disable step

* Fix i18n issue

* Fix jest tests

* Fix api tests

* PR feedback

* Update copy

* Remove unnecessary code

* Support shortcut to finish ES migration if we are on the connected cluster

* Undo changes that are now in a separate PR

* Share code better by exposing a single route with an optional parameter

* Disable more links

* Fix overview link for logstash

* PR feedback

* Fix tests

* PR feedback

* Fix tests

* We still need this route

* Updates

* Only show if there are instances too

* Change how we set the newly discovered cluster uuid

* Move this to support beats/apm weirdness

* Fix tests

* Remove out of date translations

* PR feedback

* Detect products a different way, to detect empty indices

* Look against the production cluster instead of the monitoring one

* When disabling ES internal collection, only disable ES - do not disable all internal collection
chrisronline added a commit that referenced this pull request Aug 15, 2019
* Support for logstash

* Beats support

* Fix cherry-pick api issue

* Support for logstash

* Updates for beats and logstash

* APM migration working

* Tweaks for beats migration

* Update copy for setup new button

* If on cloud, disable setup mode

* Handle new beat flow better

* Better phrasing for APM

* Add beat type to disable step

* Fix i18n issue

* Fix jest tests

* Fix api tests

* PR feedback

* Update copy

* Remove unnecessary code

* Support shortcut to finish ES migration if we are on the connected cluster

* Undo changes that are now in a separate PR

* Share code better by exposing a single route with an optional parameter

* Disable more links

* Fix overview link for logstash

* PR feedback

* Fix tests

* PR feedback

* Fix tests

* We still need this route

* Updates

* Only show if there are instances too

* Change how we set the newly discovered cluster uuid

* Move this to support beats/apm weirdness

* Fix tests

* Remove out of date translations

* PR feedback

* Detect products a different way, to detect empty indices

* Look against the production cluster instead of the monitoring one

* When disabling ES internal collection, only disable ES - do not disable all internal collection
@chrisronline
Copy link
Contributor Author

Backport:

7.x: 3434d05

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.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants