-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Update /api/stats to use core.metrics #60974
[Monitoring] Update /api/stats to use core.metrics #60974
Conversation
Pinging @elastic/stack-monitoring (Team:Monitoring) |
x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_ops_stats_collector.ts
Outdated
Show resolved
Hide resolved
@@ -22,7 +23,15 @@ export function getOpsStatsCollector( | |||
metrics$: Observable<OpsMetrics> | |||
) { | |||
let lastMetrics: MonitoringOpsMetrics | null = null; | |||
metrics$.subscribe(metrics => { | |||
metrics$.subscribe(_metrics => { | |||
const metrics: any = cloneDeep(_metrics); |
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.
nit: I don't think we need to use cloneDeep
here (unless metrics
is somehow a reference), and looks like _metrics
is not used anywhere else. Don't know if Observables hold references though, in which case maybe shallow might be good enough?:
metrics$.subscribe(({ ...metrics }) => {
Only mentioning because I know cloneDeep
can be pretty expensive
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.
Yea I tried it without it and any delete
or re-setting the object caused issues with the other subscriber.
x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_ops_stats_collector.ts
Outdated
Show resolved
Hide resolved
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.
Looks good 👍
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Backport: 7.x: e439836 |
Relates to #56675
After migration the Stack Monitoring server to new platform, we had a set of parity tests fails: https://internal-ci.elastic.co/job/elastic+estf-monitoring-snapshots+master+multijob-kibana/157/console
This is because internal collection (of monitoring data) goes through the monitoring code base, which was updated to use the new
core.metrics
API. Metricbeat collection goes through the/api/stats
endpoint, which still exists in legacy.This PR updates the
/api/stats
endpoint to use the newcore.metrics
API.It also fixes some other issues with parity between Metricbeat collection of
kibana_stats
andkibana_settings
documents.kibana_stats
Internal
Metricbeat
kibana_settings
Internal
Metricbeat
I ran the parity tests with a locally built version of Kibana from this PR and they are passing!