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] Add flag to enable/disable CCR monitoring UI #28840

Merged
merged 17 commits into from
Feb 4, 2019

Conversation

chrisronline
Copy link
Contributor

Resolves https://github.com/elastic/stack-monitoring/issues/21

This PR simply adds a config xpack.monitoring.ui.ccr.enabled that, when set to false, will not show the CCR tab under the Elasticsearch area of the Monitoring UI.

@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@ycombinator
Copy link
Contributor

ycombinator commented Jan 16, 2019

I'm not sure we need another flag on the Kibana side here. Cloud is disabling CCR in ES already via the xpack.ccr.enabled flag. So I'd prefer if the Monitoring UI just did a GET _cluster/settings?include_defaults&filter_path=defaults.xpack.ccr.enabled and read the enabled/disabled value from there (it's okay to look directly under defaults for this setting as it's not dynamically update-able).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline
Copy link
Contributor Author

@ycombinator Will do. I tried to mess with that setting in my elasticsearch.yml, but there are errors at startup that will be fixed by elastic/elasticsearch#37432

@ycombinator
Copy link
Contributor

ycombinator commented Jan 16, 2019

I have CCR enabled in ES (default settings, didn't change anything), and I see this weird behavior in the UI:

jan-16-2019 10-19-08

Specifically:

  1. Notice the slight delay for the CCR tab to show up when the page loads initially. Not sure if there's something you can do about this but it's probably not a big deal if you can't.

  2. The bigger issue: the CCR tab disappears after clicking on it. The cluster name disappears from the breadcrumbs too. If you click back to the Overview tab, the CCR tab appears again.

@chrisronline
Copy link
Contributor Author

I think for 1 we have to either accept this delay (in the current code architecture, which is also related to #28245) in cloud or in the standalone kibana. If we accept the delay on cloud, the CCR link will appear for a brief second, then disappear. I think the current code and UX is the better of the two so I'm inclined to leave as is.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

@ycombinator Did some refactoring in 76add94 to correct the UX issue. Thanks for pointing that out!

@ycombinator
Copy link
Contributor

The janky UX issues I found earlier have gone away now. 👍

However, I'm now running into elastic/elasticsearch#37432 when trying to test this PR with setting xpack.ccr.enabled: false in my elasticsearch.yml. Added the blocked label.

@elasticmachine
Copy link
Contributor

💔 Build Failed

}

export async function checkCcrEnabled(req) {
const { callWithRequest } = req.server.plugins.elasticsearch.getCluster('monitoring');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the right cluster to check against? Or is it admin? @pickypg

Copy link
Contributor

@ycombinator ycombinator Jan 16, 2019

Choose a reason for hiding this comment

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

Ah crap, of course! We can't just call this API against the production or monitoring clusters 🤦‍♂️. What we need is to know via monitoring data if CCR is enabled or disabled on a particular production cluster. I'm not sure we have the data on whether CCR is enabled or not in .monitoring-es-*.

Copy link
Contributor

@ycombinator ycombinator Jan 16, 2019

Choose a reason for hiding this comment

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

We will automatically start collecting this information in cluster_stats documents under the stack_stats.xpack.ccr.* field once elastic/elasticsearch#37256 is merged. Until then we can update the code in this PR to look at this field to make the determination whether to show the CCR tab or not.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -83,6 +83,7 @@ uiModule.directive('monitoringMain', (breadcrumbs, license, kbnUrl, config) => {
config.watch('k7design', (val) => scope.showPluginBreadcrumbs = !val);

function getSetupObj() {
console.log(attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to leave this in? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all! Thanks!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ycombinator
Copy link
Contributor

image

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cachedout
Copy link
Contributor

@chrisronline This looks like it's ready to go for 6.7 :)

@chrisronline
Copy link
Contributor Author

Unfortunately, not yet. I need to test the code changes I made in response to @ycombinator's comment: #28840 (comment). Should be ready soon

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline chrisronline merged commit 4dab26a into elastic:master Feb 4, 2019
@chrisronline chrisronline deleted the monitoring/ccr_enabled_flag branch February 4, 2019 21:15
chrisronline added a commit to chrisronline/kibana that referenced this pull request Feb 4, 2019
chrisronline added a commit that referenced this pull request Feb 5, 2019
@chrisronline
Copy link
Contributor Author

Backport:
6.x: fbe7667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants