-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] compatibility for agent data streams #119112
[Stack Monitoring] compatibility for agent data streams #119112
Conversation
@@ -66,28 +66,17 @@ export function prefixIndexPattern( | |||
} | |||
|
|||
if (!ccsEnabled || !ccs) { | |||
return appendMetricbeatIndex( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stop appending the metricbeat-*
pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a thought here that there's a possibility people might be using monitoring.ui.metricbeat.index
to append custom index patterns. Should we add a deprecation warning if that config key is specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we ever supported the user using this as it wasn't documented anywhere and we didn't technically support metricbeat-*
. if that's the case would deprecating it be confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, okay if it's not in the docs I guess it's not bad to just let it go away silently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#120384 has me wondering if we should re-think this approach. I get the feeling there's more than 1 customer problem we've solved by reaching for monitoring.ui.metricbeat.index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we kept the config value there, we aren't using it anymore because we aren't querying metricbeat index anymore. If we recommended that kind of hack, hopefully we let the user know that. We probably should have discussed a fix whether we should be setting ccs
to default to *
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I guess we could say merging this PR raises the importance of having a proper config (which is #120384)
return indexPatterns; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new function similar to getIndexPatterns
but returns 1. There will be a case for each type (kibana, elasticsearch, logstash, etc). Since the datastreams use elasticsearch
instead of es
(used in .monitoring-es
) we have to map them.
} | ||
return `${type}-${datasetsPattern}-${namespace}`; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gives you both datastream and legacy index patterns for a given moduleType
(elasticsearch, kibana, etc)
8511fe8
to
597be18
Compare
597be18
to
0f389fb
Compare
1764d6d
to
8f8d8b1
Compare
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
x-pack/plugins/monitoring/server/lib/cluster/get_clusters_from_request.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/monitoring/server/lib/cluster/get_clusters_stats.ts
Outdated
Show resolved
Hide resolved
@matschaffer thanks for the heads up. We'll see what @richkuz says as @kovyrin is on holiday for a while. I think this PR should continue to be merged, since we don't officially support |
I'm OK with merging this into 8.1.0 and breaking Ent Search temporarily. As long as this PR is only targeting 8.1.0, and not 8.0.0, we should have enough time to find and implement a resolution for Enterprise Search in a follow-up PR (tracking in #121975 ). |
@matschaffer @klacabane What do you think about trying to get this in so we can start testing? |
Sounds good to me :) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…kibana into 119109-es-integration-queries
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @neptunian |
👋 Hi, I'm wondering if these changes have broken stack monitoring on Elastic 8.x when using Metricbeat. From what I can see with this change, Kibana is now expecting documents with terms for Elastic cluster stats However the metricbeat elasticsearch module (when used for stack monitoring) does not include these fields in the documents it emits? |
Closes #104271
Updates queries to include
metrics-{dataset}-{namespace}
for usage with Agent and integrations. These changes don't require testing of the actual packages, we are mainly wanting to look at the changes in the queries and make sure existing functionality does not break because nearly every query has been modified. If you find issues trying to load the app when using the Agent with packages, please add them to the separate issues for each package listed here https://github.com/elastic/observability-dev/issues/1660monitoring.ui.metricbeat.index
and all references to itmetricbeat-*
in a new function calledgetNewIndexPatterns
(to replacegetIndexPatterns
)data_stream.dataset
to the filter when targeting a particular datasetfilebeat-*
patterns to includelogs-*
. This will be done in a separate PR.Test
.monitoring
) Turn on internal monitoring and everything should work as normal. Ideally setup all possible modules (ES, Logstash, APM, Beats, Kibana) and setup CCR and ML jobs. @matschaffer used the Docker testing cluster with some tweaks to make it use internal collection https://github.com/elastic/observability-dev/pull/1831/files.monitoring
with-mb
) prefix . This is the default behavior when spinning up the Docker testing cluster, but I don't think this is possible until we resolve the mapping issueExample query change:
Cluster stats call before
cluster stats call now
metrics aggregation query before
metrics aggregation query after
disk usage alerts before
disk usage alerts after