[Infra UI] Convert terms aggregation to composite for field selection filtering #47500
Conversation
simianhacker
added
release_note:fix
Feature:Metrics UI
|
Pinging @elastic/infra-logs-ui (Team:infra-logs-ui) |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
|
@elasticmachine update branch |
💚 Build Succeeded |
There was a problem hiding this comment.
This does work and fixes the original issue, so I've approved this in the hope it makes the FF cutoff.
However, the response times are still very long for me testing against the shared Observability cluster. I was seeing this sort of average:
17 seconds is a long time in general, but given this affects the entirety of Infra or Logs loading it feels a lot worse 😬
Otherwise just some small comments.
| @@ -63,6 +69,24 @@ export class InfraSnapshot { | |||
| } | |||
| } | |||
|
|
|||
| // This is used by the getAllCompositeData to select the bucket to return. | |||
There was a problem hiding this comment.
I love comments, but I'm not sure we need these given the parameters for getAllCompositeData() are fully typed and explain themselves?
| ) => (response.aggregations && response.aggregations.nodes.buckets) || []; | ||
|
|
||
| // This is used by getAllCompositeData to add the after key to the subsequent requests | ||
| const handleAfterKey = ( |
There was a problem hiding this comment.
As the two uses of handleAfterKey in this PR are very, very similar - with only the actual keys and portions of the response to be used changing - is it worth making this into a helper function too? I imagine the
if (!response.aggregations) {
return options;
}
check and the merging of the options will always be the same.
| InfraDatabaseSearchResponse, | ||
| } from '../lib/adapters/framework'; | ||
|
|
||
| export const getAllCompositeData = async < |
|
@Kerry350 We could add: Which would limit the data to the last 24 hours (according to the server). |
|
Here is what the query looks like on my machine without filtering for the last 24 hours:
Here is what it looks like with filtering for the last 24 hours
And here is what it looks like for the last 4 hours
I was hoping for a more dramatic change by limiting to the last 24 hours but I think this server is just severely under powered. I'm not comfortable with a time frame shorter then 24 hours because some metric sets only report in 24 hour intervals. |
💚 Build Succeeded |
💚 Build Succeeded |
Yep, makes sense. Thanks for the changes, they LGTM 👍 |
… filtering (elastic#47500) * Convert from large terms agg to multiple smaller composite * Fixing bug in snapshot * Reducing duplicate code for composite agg requests * Adding filter for last 24 hours * Creating a helper for the afterKey handlers
… filtering (elastic#47500) * Convert from large terms agg to multiple smaller composite * Fixing bug in snapshot * Reducing duplicate code for composite agg requests * Adding filter for last 24 hours * Creating a helper for the afterKey handlers
Summary
This PR fixes #47048 by converting the terms aggregation to a composite aggregation to improve performance on slower clusters. This also fixes a bug with the snapshot aggregation where the additional queries for pagination didn't include the after key.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsUnit or functional tests were updated or added to match the most common scenariosThis was checked for keyboard-only and screenreader accessibility