[Infra UI] Use new snapshot endpoint #34938
Conversation
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
skh
force-pushed
the
infraui-use-snapshot-endpoint
branch
from
2113749 to
d613e9c
Compare
💔 Build Failed |
skh
force-pushed
the
infraui-use-snapshot-endpoint
branch
from
91962ed to
381ddfb
Compare
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
💚 Build Succeeded |
|
Pinging @elastic/infrastructure-ui |
There was a problem hiding this comment.
Everything looks good except for one note I left about adding a -58s offset to the date_histogram aggregation. And...
@makwarth While I was playing around with the filtering and grouping I noticed this behavior:
@makwarth Is this the intended UX? Should the filtering be applied to the metric values? or just the groupings. In the Metric Explorer, I choose to only apply the filtering to the groupings but left the values separate. Either way we should make the behavior consistent across both.
Good catch again, I agree the behaviour should be consistent, and the only way to always show metrics is to not apply the filter query on the metric values. The way it is now there's no way for the user to understand why the metric values go away from one click to the next. |
|
Good catch @simianhacker. I agree the filter shouldn't apply to the metric values. |
💚 Build Succeeded |
|
@simianhacker Thanks for the review! This is ready for another look once CI is through. |
💚 Build Succeeded |
|
For the sake of completeness, not applying the filter on the metrics query means we always query for the metrics of all nodes, which might become problematic in really large infrastructures. We will need to tackle this once we start making the snapshot view more resilient. For now I think it would be premature optimization, but we shouldn't forget it. |
There was a problem hiding this comment.
Good job...
,--, ,----,
,---.'| ,/ .`| ____
| | : ,----.. ,` .' : ,' , `.
: : | / / \ ; ; / ,-+-,.' _ |
| ' : | : :.'___,/ ,' ,-+-. ; , ||
; ; ' . | ;. /| : | ,--.'|' | ;|
' | |__ . ; /--` ; |.'; ;| | ,', | ':
| | :.'|; | ; __`----' | || | / | | ||
' : ;| : |.' .' ' : ;' | : | : |,
| | ./ . | '_.' : | | '; . | ; |--'
; : ; ' ; : \ | ' : || : | | ,
| ,/ ' | '/ .' ; |.' | : ' |/
'---' | : / '---' ; | |`-'
\ \ .' | ;/
`---` '---'
* Use new snapshot endpoint * Remove old nodes endpoint * Reintroduce NAME_FIELDS for displayable names. * Use camelCase consistently. * Distinguish node name and node id correctly. * Adjust functional tests. * Make prettier. * Use exact same date histogram as before. * Enable test for metric values again. * Add test for new groupBy behaviour. * Add 'Service Type' to groupBy fields. * Fix date histogram offset and adjust tests. * Always query for all metrics.
* Use new snapshot endpoint * Remove old nodes endpoint * Reintroduce NAME_FIELDS for displayable names. * Use camelCase consistently. * Distinguish node name and node id correctly. * Adjust functional tests. * Make prettier. * Use exact same date histogram as before. * Enable test for metric values again. * Add test for new groupBy behaviour. * Add 'Service Type' to groupBy fields. * Fix date histogram offset and adjust tests. * Always query for all metrics.
Summary
Second part of the implementation of #31405.
Changes the snapshot view to use the new
snapshotgraphql endpoint introduced in #34264 . Also, adds tests for this new endpoint, and removes the oldnodesendpoint that is no longer used.For testing I used the Observability 8.0.0-SNAPSHOT cluster. The main purpose of this change is that it is now possible for all node types to group by
Service Typeand see metrics in all groups:During testing I noticed a bug in the waffle map where it's not possible to scroll to the top in certain situations:
I will look into this, but I can also reproduce it inmasterso it's not related to this change.This has most likely been fixed by #34881
Checklist
- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately