[SLO] use terms aggregation to group by tags #174268

mgiota · 2024-01-04T14:15:56Z

Fixes #174328, #174332, #174335

MVP for grouping by tags for the different view modes

Screen.Recording.2024-01-16.at.23.29.22.mov

apmmachine · 2024-01-04T14:16:08Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

x-pack/plugins/observability/public/hooks/slo/use_fetch_slo_groups.ts

dominiqueclarke · 2024-01-05T17:08:41Z

x-pack/plugins/observability/public/hooks/slo/use_fetch_slo_groups.ts

+        })
+        .subscribe({
+          next: (response) => {
+            if (!isRunningResponse(response)) {


What is isRunningResponse? Seems like a TIL moment for me.

I think eventually we will get rid of this once we work on the paginated groups API , but let me summarize what it is about (or why I used it and what my inspiration was)

I wanted to use the ES search function similar to how we do it here, but for some reason I don't recall right now, I didn't have access to esClient or I didn't know how to use it. Then I started looking into alternatives on how I can use ES search method and I got inspired by this example.

I read more about ES search service here and here

But once we implement the group api, we will be able to simply use await this.esClient.search({...}) and get rid of the low lever search implementation I have here.

Does it make sense?

x-pack/plugins/observability/public/hooks/slo/use_fetch_slo_groups.ts

x-pack/plugins/observability/public/pages/slos/components/grouped_slos/group_view.tsx

mgiota · 2024-01-08T07:25:22Z

@elasticmachine merge upstream

elasticmachine · 2024-01-08T07:25:34Z

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

kdelemme

Overall looks good to me. Nothing blocking that can't be done later in follow-up PRs.
I've just one thought about reusing in the grouping component. It should be possible don't you think? But again, can be done in a follow-up PR

kdelemme · 2024-01-16T20:50:43Z

x-pack/plugins/observability/public/pages/slos/components/slo_list_group_by.tsx

+                  isSelected={isGroupByPopoverOpen}
+                >
+                  {i18n.translate('xpack.observability.slo.list.groupByType', {
+                    defaultMessage: 'Group by {type}',


🍰 nit: instead of "Group by ungrouped" we could use "none" or we can keep type: "ungrouped" but change the text when ungrouped is selected: "No group selected", "none".
Anyway this can be done later.

Yep this should be covered here cc @dominiqueclarke

According to the design it should say No grouping

kdelemme · 2024-01-16T20:53:20Z

x-pack/plugins/observability/public/pages/slos/components/grouped_slos/group_view.tsx

+    <>
+      {data &&
+        Object.keys(data).map((group) => {
+          return <GroupListView group={group} kqlQuery={kqlQuery} isCompact={isCompact} />;


Don't you need a "key" prop to please react?

I made the change and now group is part of the key for the slo items.

kdelemme · 2024-01-16T20:55:39Z

x-pack/plugins/observability/public/pages/slos/components/grouped_slos/group_list_view.tsx

+        <>
+          <SloListItems
+            activeAlertsBySlo={activeAlertsBySlo}
+            sloList={results}
+            loading={isLoading || isRefetching}
+            error={isError}
+            isCompact={isCompact}
+            rulesBySlo={rulesBySlo}
+          />
+          <EuiTablePagination
+            pageCount={Math.ceil(total / ITEMS_PER_PAGE)}
+            activePage={page}
+            onChangePage={handlePageClick}
+            itemsPerPage={ITEMS_PER_PAGE}
+          />


Couldn't you reuse component directly?

<SlosView sloList={results} loading={isLoading || isRefetching} error={isError} isCompact={isCompact} sloView={view} />

Yep good idea! I was actually refactoring this as part of Group by and view modes and your suggestion works like a charm. I pushed the change

Screen.Recording.2024-01-16.at.23.01.13.mov

kdelemme · 2024-01-16T20:56:58Z

x-pack/plugins/observability/public/hooks/slo/use_fetch_slo_groups.ts

+  } = useKibana().services;
+
+  const { data, isLoading, isFetching } = useQuery({
+    queryKey: ['slos'],


Like we talked about, you can use a unique key like ['slos', 'groups'] added in the x-pack/plugins/observability/public/hooks/slo/query_key_factory.ts

mgiota · 2024-01-16T23:04:34Z

x-pack/plugins/observability/public/hooks/slo/query_key_factory.ts

@@ -19,6 +19,7 @@ export const sloKeys = {
  all: ['slo'] as const,
  lists: () => [...sloKeys.all, 'list'] as const,
  list: (filters: SloListFilter) => [...sloKeys.lists(), filters] as const,
+  groups: () => [...sloKeys.all, 'groups'],


the groupBy filter to this function will be added as part of this #174329

…-fix'

kibana-ci · 2024-01-17T10:43:17Z

💛 Build succeeded, but was flaky

Failed CI Steps

Jest Tests #6

Test Failures

[job] [logs] Jest Tests #6 / RegisteredAttachmentsPropertyActions renders the correct number of actions

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	581	585	+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`observability`	600.7KB	604.4KB	+3.6KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`observability`	101.2KB	101.3KB	+89.0B

History

💔 Build #187813 failed b4148030d65a5a99af2d86cd59249117cf500ca9
💛 Build #187790 was flaky be65bfe
💔 Build #187685 failed 71f6b4ae274cc675daa12cd153a0987d26b6a9c5
💔 Build #187646 failed fb10fb894d57371cbbcffc68ce462a04f052eb29
💔 Build #187617 failed 664e53238a474a1fd60dfa2e49ed822901434818

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mgiota

dominiqueclarke

MVP LGTM! 🚢

kdelemme · 2024-01-17T19:17:39Z

x-pack/plugins/observability/public/pages/slos/components/grouped_slos/group_list_view.tsx

+          <SlosView
+            sloList={results}
+            loading={isLoading || isRefetching}
+            error={isError}
+            isCompact={isCompact}
+            sloView={sloView}
+            group={group}
+          />
+
+          <EuiTablePagination
+            pageCount={Math.ceil(total / ITEMS_PER_PAGE)}
+            activePage={page}
+            onChangePage={handlePageClick}
+            itemsPerPage={ITEMS_PER_PAGE}
+          />


Awesome that we can reuse components

kdelemme

LGTM !

mgiota self-assigned this Jan 4, 2024

mgiota mentioned this pull request Jan 5, 2024

[SLOs] Unified Search #174054

Merged

dominiqueclarke requested changes Jan 5, 2024

View reviewed changes

mgiota force-pushed the slo_group_by_tags branch from 3f888b5 to b958438 Compare January 5, 2024 22:18

mgiota marked this pull request as ready for review January 5, 2024 22:54

mgiota requested a review from a team as a code owner January 5, 2024 22:54

mgiota requested a review from dominiqueclarke January 5, 2024 22:54

botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Jan 8, 2024

mgiota linked an issue Jan 15, 2024 that may be closed by this pull request

MVP functionality for group by tags #174328

Closed

mgiota force-pushed the slo_group_by_tags branch from 5f6ebc4 to b958438 Compare January 16, 2024 09:18

mgiota requested review from a team and shahinakmal as code owners January 16, 2024 09:35

mgiota force-pushed the slo_group_by_tags branch 2 times, most recently from f4ad29a to e698305 Compare January 16, 2024 13:26

kdelemme reviewed Jan 16, 2024

View reviewed changes

mgiota linked an issue Jan 16, 2024 that may be closed by this pull request

Group by and view modes #174332

Closed

mgiota force-pushed the slo_group_by_tags branch from bc1093f to b6f1e83 Compare January 16, 2024 22:27

mgiota assigned mgiota and unassigned mgiota Jan 16, 2024

mgiota added the v8.13.0 label Jan 16, 2024

mgiota commented Jan 16, 2024

View reviewed changes

mgiota requested a review from kdelemme January 16, 2024 23:44

mgiota force-pushed the slo_group_by_tags branch from 4b7dd20 to 374ba7c Compare January 16, 2024 23:48

mgiota and others added 10 commits January 17, 2024 00:52

use terms aggregation to group by tags

ff36e5e

clean up use_fetch_slo_groups and group view

dcf383e

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

e9c2db5

…-fix'

fix typescript errors

84fa86d

pass rulesBySlo info to sloListItems

cbf60d6

fix CI

77950ae

refactor group by view modes

db9f38d

refactor queryKey to include groups

4a96342

fix tests

402939b

group is added to the key

be65bfe

mgiota force-pushed the slo_group_by_tags branch from 374ba7c to be65bfe Compare January 16, 2024 23:57

mgiota linked an issue Jan 17, 2024 that may be closed by this pull request

Group by with sorting and searching #174335

Closed

pass sort and direction to the group view

2950725

mgiota force-pushed the slo_group_by_tags branch from b414803 to 2950725 Compare January 17, 2024 09:24

dominiqueclarke approved these changes Jan 17, 2024

View reviewed changes

kdelemme reviewed Jan 17, 2024

View reviewed changes

kdelemme approved these changes Jan 17, 2024

View reviewed changes

mgiota merged commit 9b84690 into elastic:slo_group_feature Jan 17, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SLO] use terms aggregation to group by tags #174268

[SLO] use terms aggregation to group by tags #174268

mgiota commented Jan 4, 2024 •

edited

Loading

apmmachine commented Jan 4, 2024

dominiqueclarke Jan 5, 2024

mgiota Jan 5, 2024

mgiota Jan 5, 2024

mgiota commented Jan 8, 2024

elasticmachine commented Jan 8, 2024

kdelemme left a comment

kdelemme Jan 16, 2024

mgiota Jan 16, 2024 •

edited

Loading

kdelemme Jan 16, 2024

mgiota Jan 16, 2024

kdelemme Jan 16, 2024

mgiota Jan 16, 2024

kdelemme Jan 16, 2024

mgiota Jan 16, 2024

mgiota Jan 16, 2024

kibana-ci commented Jan 17, 2024

dominiqueclarke left a comment

kdelemme Jan 17, 2024

kdelemme left a comment

[SLO] use terms aggregation to group by tags #174268

[SLO] use terms aggregation to group by tags #174268

Conversation

mgiota commented Jan 4, 2024 • edited Loading

apmmachine commented Jan 4, 2024

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgiota commented Jan 8, 2024

elasticmachine commented Jan 8, 2024

kdelemme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgiota Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jan 17, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

dominiqueclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdelemme left a comment

Choose a reason for hiding this comment

mgiota commented Jan 4, 2024 •

edited

Loading

mgiota Jan 16, 2024 •

edited

Loading