-
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
[Metrics UI] Performance improvements for Observability Homepage #70869
[Metrics UI] Performance improvements for Observability Homepage #70869
Conversation
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
💚 Build SucceededBuild metrics
To update your PR or re-run it, just comment with: |
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.
Just a note about naming of the endpoint/key, otherwise I tested this and it works great and fast:
$ time curl -u redacted:redacted http://localhost:5601/api/metrics/source/default/metrics/hasData
{"hasData":true}
real 0m0.055s
user 0m0.006s
sys 0m0.007s
framework.registerRoute( | ||
{ | ||
method: 'get', | ||
path: '/api/metrics/source/{sourceId}/{type}/hasData', |
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.
Can we call this endpoint "indicesExist" or "hasIndices" ... something to distinguish it from checking for actual documents?
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 also noticed that this returns "hasData: true" so long as just one of the configured indices exist. Maybe the endpoint should be named accordingly? "hasAnyConfiguredIndices" or "hasAnySourceIndices"?
Whatever we go with we should make sure the key in the response matches, too.
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.
This only returns true if the type
passed in the URL has data. For example, if you don't have any logs data but you call /api/metrics/source/logs/hasData
then it returns false
. The {type}
in the URL controls which indices it looks at.
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.
Does it check for actual documents though? I thought it was only checking if the indices exist?
Update: Maybe these two concepts aren't really that different in ES? If so, hasData is fine.
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.
It looks like InfraElasticsearchSourceStatusAdapter:hasIndices()
sends a query and checks to see if at least one shard has a document using the terminate_after: 1
to ensure it's as performant as possible.
return await this.framework
.callWithRequest(requestContext, 'search', {
ignore_unavailable: true,
allow_no_indices: true,
index: indexNames,
size: 0,
terminate_after: 1,
})
.then(
(response) => response._shards.total > 0,
(err) => {
if (err.status === 404) {
return false;
}
throw err;
}
);
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.
Sounds good. I saw that query and thought it was returning whether or not at least one shard returned having found the index, but if this is looking up at least one document, then hasData
works here.
: await libs.sourceStatus.hasLogIndices(requestContext, sourceId); | ||
|
||
return response.ok({ | ||
body: { hasData }, |
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.
We should make this key match the endpoint whatever we decide.
Summary
This PR attempts to improve the performance for the Metrics UI section of the observability homepage. This addresses issues found while reviewing #69141
Checklist
Delete any items that are not applicable to this PR.