-
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
[SLO] paginated api for groups #175190
[SLO] paginated api for groups #175190
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
df9e252
to
6061030
Compare
6061030
to
0537591
Compare
de7f3b6
to
48642b1
Compare
6061030
to
5f2f2d9
Compare
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 one note on the api versioning. Otherwise LGTM
@@ -123,6 +133,8 @@ const sloWithSummaryResponseSchema = t.intersection([ | |||
t.type({ summary: summarySchema }), | |||
]); | |||
|
|||
const sloGroupsResponseSchema = t.record(t.string, t.number); |
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.
Since this is behind a feature flag, it's fine to start with the result being a Record<group, number of element in group>
but when we introduce more stats in the response, we'll have to change the results to either an array or a Record<group, Stat Object>.
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.
Yep definitely the type will change when we introduce a) the stats and b) I guess when we introduce more group by options. So it will be more like:
{
"by_tags": {
"test": {
total: 5,
worstValue: ...
...
},
},
"by_status": {
"healthy": StatsObject,
"violated": StatsObject
}
},
"by_sli_indicator": {
...
}
Is it fine to keep it like this for now? Once this is merged, I will work on the summary stats, so this change very soon
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'm not sure returning all the group options (by tags, by indicator, by sttus_ at once is ideal since you'll need to handle pagination within each, and also based on the API input parameter, we explicitly ask for the group we want, i.e. groupBy: groupBySchema
I'd imagine the API being something like:
> Request
GET /api/observability/slos/groups?page=2&perPage=10&groupBy=tags
< Response
{
total: 13
page: 2
perPage: 10,
results: [
{ group: "tag-value-ccc", stats: { healthy: 82, violated: 1, degrading: 3, total: 86 } },
{ group: "tag-value-aaa", stats: { healthy: 4, violated: 0, degrading: 0, total: 4 } },
{ group: "tag-value-bbb", stats: { healthy: 0, violated: 10, degrading: 0, total: 10 } },
]
}
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.
Ok so in case of grouping by status for example, you say we would still have the same response like this:
> Request
GET /api/observability/slos/groups?page=2&perPage=10&groupBy=status
< Response
{
total: 2
page: 1
perPage: 10,
results: [
{ group: "healthy", stats: { healthy: 82, violated: 0, degrading: 0, total: 82 } },
{ group: "violated", stats: { healthy: 0, violated: 10, degrading: 0, total: 10 } },
]
}
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.
Yep indeed the API accepts groupBySchema
. At the moment I don't make use of it, but eventually we will as part of #174329
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.
Correct. For status groups the stats are not very useful (except the total) but as we add more stats they will make sense.
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
@shahzad31 You recommended to fetch all tags in one terms aggregation and still have the UI pagination. How would I do this? I tried to remove the bucket_sort aggregation, but going to the next page didn't work well for me. It showed me the same tags with 1st page. Can you be more specific how you would do this? |
Oh I see I think I know what you mean. Use slice to display the items I need to show based on the active page |
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
1cfc766
to
6146ada
Compare
💔 Build Failed
Failed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: cc @mgiota |
48642b1
to
df7f967
Compare
6146ada
to
30706f8
Compare
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.
Let's iterate on the API response shape in a follow-up PRs
Ok cool I'm merging this one and will continue with the summary data |
Fixes #174330
This PR creates the paginated API for groups, without summary for now. Summary data will come in a follow up PR