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

[Stack Monitoring] support custom ccs remote prefixes #129806

Closed

Conversation

neptunian
Copy link
Contributor

@neptunian neptunian commented Apr 7, 2022

Resolves #120384
Adds the ability for the user to specify a remote prefix instead of searching cross cluster with the default "*" query which gets all available remotes.

  • add monitoring.ui.ccs.remotePatterns to kibana config
  • rename function prefixIndexPattern to prefixIndexPatternWithCcs
  • exposes to browser monitoring.ui.ccs.remotePatterns in order to use the KQL filter in the rules

Changes in behavior when using the monitoring.ui.ccs.remotePatterns and not using *

  • if monitoring.ui.ccs.enabled is true (default), a cross cluster search will only look for remotes in monitoring.ui.ccs.remotePatterns instead of all available.
  • If the user doesn't have access to some specified remote they will get this error when visiting Stack Monitoring and not be able to view any of their data. This is also the case if the remote doesn't exist or doesn't exist anymore.
    Screen Shot 2022-04-13 at 10 11 29 AM
  • If the user has rules and the remote pattern does not exist, all rule executions will fail for all clusters ( Error: security_exception: [no_such_remote_cluster_exception] Reason: no such remote cluster: [idontexist], caused by: "no such remote cluster: [idontexist]"). Previously we used * and before that fetchAvailableCcs when CCS was enabled so we did not have this issue, but that created a problem when there were too many remotes.
    Screen Shot 2022-04-13 at 10 17 48 AM

Test

  • I locally setup three ES clusters to test this and added 2 as remotes, myremote and myremote2. Using monitoring.ui.ccs.remotePatterns I was able to "filter" out my remotes by adding "myremote" or ["myremote"] and only that remote showed up in the cluster listing. When i added the 2nd remote ["myremote", "myremote2"], both of them showed up in the cluster listing. When I set monitoring.ui.ccs.enabled: false, neither showed up.
  • new unit tests should pass

@neptunian neptunian added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.3.0 labels Apr 7, 2022
@neptunian neptunian self-assigned this Apr 7, 2022
* @return {String} The index pattern with the {@code cluster} prefix appropriately prepended.
*/
export function prefixIndexPattern(config: MonitoringConfig, indexPattern: string, ccs?: string) {
const ccsEnabled = getConfigCcs(config);
export function prefixIndexPatternWithCcs(
Copy link
Contributor Author

@neptunian neptunian Apr 12, 2022

Choose a reason for hiding this comment

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

I changed the name to have "WithCcs" because previously this function also handled appending metricbeat-*. Now it only does CCS so want to clarify that.

@neptunian neptunian changed the title [Stack Monitoring] support custom css remote prefixes [Stack Monitoring] support custom ccs remote prefixes Apr 13, 2022
@neptunian neptunian force-pushed the 120384-allow-ccs-remote-pattern branch 2 times, most recently from 08f1cb1 to 834da9e Compare April 13, 2022 20:25
@@ -114,6 +148,12 @@ export function createConfig(config: MonitoringConfigSchema): MonitoringConfig {
...config,
ui: {
...config.ui,
ccs: {
...config.ui.ccs,
remotePatterns: Array.isArray(config.ui.ccs.remotePatterns)
Copy link
Contributor Author

@neptunian neptunian Apr 13, 2022

Choose a reason for hiding this comment

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

is user specifies a string value put it in an array we only have to work with array type.

@neptunian
Copy link
Contributor Author

@jasonrhodes @ravikesarwani This PR is about ready, but @jportner mentioned we should consider putting this in Kibana Advanced Settings UI instead of in the yaml config. Given the impact putting in a non existent remote could have on alerts and viewing the SM UI, what do you think? We'd also be able to have it by space if that's something we possibly see implementing in the future.

Screen Shot 2022-04-14 at 10 44 12 AM

@jportner
Copy link
Contributor

We'd also be able to have it by space if that's something we possibly see implementing in the future.

Just to be clear, everything in Advanced Settings is space-isolated by design. In other words, if you change an Advanced Setting in one space, it only affects that space, there's no additional dev effort to make that happen 👍

@neptunian
Copy link
Contributor Author

Something to consider is that so long as the user selects "skip if unavailable" in the remote (which is not on by default), the errors mentioned above will not happen.

Screen Shot 2022-04-14 at 10 51 48 AM

When the remote goes down, the alert will recover and the user can still access SM UI
Screen Shot 2022-04-14 at 10 53 01 AM

Of course if the user enters a remote name in the kibana config that never existed to begin with they will still get the errors.

@ravikesarwani
Copy link
Contributor

Was trying to understand the context:

  • This will be used by users to see stack monitoring data on the production cluster Kibana when it's being shipped to remote monitoring cluster?
  • How does it work in cloud/ESS?

@neptunian neptunian force-pushed the 120384-allow-ccs-remote-pattern branch from 834da9e to 58f3f3a Compare April 14, 2022 17:05
@kibana-ci
Copy link
Collaborator

kibana-ci commented Apr 14, 2022

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] OSS Misc Functional Tests / core plugins rendering service exposes plugin config settings to authenticated users
  • [job] [logs] OSS Misc Functional Tests / core plugins rendering service exposes plugin config settings to authenticated users

Metrics [docs]

Async chunks

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

id before after diff
monitoring 471.1KB 471.2KB +73.0B

History

  • 💔 Build #38045 failed 834da9e71f27e715ebe7c6b434a6a09d4b7153e6
  • 💛 Build #37708 was flaky 9099bc3aa77fabd5711586bfc6800a9833f55219
  • 💚 Build #37680 succeeded 226ab2c00dbfca6e53b8aa1ac578a6f612a6db92
  • 💔 Build #36979 failed dd8b89e5e76628803e7640ef543a748d4ebe012d

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

cc @neptunian

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

Just pointing to this note here #120384 (comment)

@neptunian and I decided we'll rework this to read the value from a constant for now instead of from a new config flag. That will maintain all of the consolidation work done here without introducing a flag we have to support for a long time, so that we can wait a bit longer to decide on whether we want this flag or not.

@neptunian
Copy link
Contributor Author

Closing in place of #130466 for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Monitoring] Allow users to configure CCS pattern
5 participants