[SLOs] In Embeddables, show all related instances option #175503

shahzad31 · 2024-01-25T07:38:48Z

Summary

in SLOs embeddable, added the ability show all related instances Overviews and alerts !!

if selected SLO has group-by, it will give an option to show all related instances from the group-by field

All related instances will appear as grid

…d-config

elasticmachine · 2024-01-25T07:38:54Z

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

apmmachine · 2024-01-25T07:39:02Z

🤖 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!)

mgiota · 2024-01-25T07:49:56Z

That looks super cool!

.../plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table_state.tsx

…/alerts_table/alerts_table_state.tsx

mgiota · 2024-01-30T07:37:56Z

@shahzad31 I like the idea! I tested it locally and here are a few comments before doing the code review:

Here's how it looks like when I added the overview embeddable for group by instances. Can you change the default panel dimensions of the grouped embeddable? Here's a PR that might help.

I would like some input from @maciejforcone regarding Show all related group-by instances label
The new toggle doesn't appear on the alerts embeddable. Yesterday it didn't appear on the overview embeddable as well, although I had some group by instances. I didn't have any data for this SLO though. I mean I created an SLO using the kbn-data-forge package (previously high cardinality indexer) and while creating it, in the SLI preview I could see there was no data. So in case there is no data flowing, the group by toggle won't be visible.
Do we need to consider any migration? For example if user had previously added an SLO overview embeddable of a group by instance, and now selects to include another group by instance of the same SLO, having the toggle on, he will just see this specific group by instance twice. I don't think anything would break, right? I will test it and report back

Screen.Recording.2024-01-30.at.08.28.52.mov

mgiota · 2024-01-30T08:38:05Z

x-pack/plugins/observability/public/embeddable/slo/alerts/slo_configuration.tsx

+
+  const onConfirmClick = () => onCreate({ slos: selectedSlos, showAllGroupByInstances });
+
+  const hasGroupBy = selectedSlos?.some((slo) => slo.instanceId !== '*');


Better use the ALL_VALUE here

…d-config

…into slo-card-config

x-pack/plugins/observability/public/embeddable/slo/alerts/components/slo_alerts_table.tsx

mgiota · 2024-01-30T09:03:49Z

x-pack/plugins/observability/public/embeddable/slo/alerts/components/slo_alerts_table.tsx

            },
          },
        ],
      },
    };

    return query;
-  }, [slos, timeRange]);
+  }, [showAllGroupByInstances, slos, timeRange.from]);


Not related to current changes, but I was wondering why we don't have the timeRange.to in the range query. If user selects to see alerts from a specific date to another date, then we don't take the to into consideration. Do we need to create a ticket for this?

range: { '@timestamp': { gte: timeRange.from, }, },

@shahzad31 If user selects to time range in the timepicker, it won't be applied right? Shouldn't the range contain the timeRange.to as well? Do you think we can do this in this PR? Otherwise a follow up PR is fine as well

mgiota · 2024-01-30T09:05:54Z

x-pack/plugins/observability/public/embeddable/slo/alerts/slo_configuration.tsx

+            <EuiSpacer />
+            <EuiSwitch
+              label={i18n.translate(
+                'xpack.observability.sloConfiguration.euiSwitch.showAllGroupByLabel',


@shahzad31 How about we prefix the copy with xpack.observability.sloAlertsEmbeddable to be consistent with rest i18n copies? It would become xpack.observability.sloAlertsEmbeddable.sloConfiguration.euiSwitch.showAllGroupByLabel that long though..

mgiota · 2024-01-30T09:07:28Z

x-pack/plugins/observability/public/embeddable/slo/overview/slo_embeddable_factory.ts

-        initialInput,
-        parent
-      );
+      const [coreStart, pluginStart] = await this.getStartServices();


mgiota · 2024-01-30T09:14:46Z

The new toggle doesn't appear on the alerts embeddable.

Regarding my comment, I was able to see the toggle, once I selected a group by instance. So this was your intention, to display it only if user selects a group by instance, right?

mgiota · 2024-01-30T09:20:17Z

@shahzad31 I have an alerts embeddable. I have enabled the Show all related group by instances, but I would expect to see:

4 SLOs included on the top right link of the embeddable
in the SLO popup configuration I would expect to see the rest 3 group by instances as well

Screen.Recording.2024-01-30.at.10.17.09.mov

Also let's think more about the workflow, when user edits the configuration and disables the toggle. In this case we should exclude the alerts from the related group by instances.

mgiota · 2024-01-30T09:30:54Z

The alert notification does not appear in the group panel for the instance the-post.box. As you can see, this SLO instance has one alert

…d-config

mgiota · 2024-01-30T11:10:09Z

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

@@ -82,7 +82,7 @@ export function useFetchActiveAlerts({
                  bool: {
                    filter: [
                      { term: { 'slo.id': sloId } },
-                      { term: { 'slo.instanceId': instanceId } },
+                      ...(instanceId === '*' ? [] : [{ term: { 'slo.instanceId': instanceId } }]),


This part is used in SLO list page and SLO detail page. Let's make sure it still works fine there. Do we have any tests for this?

One reminder, we use different query for calculating active alerts in SLO pages vs SLO embeddable. I will create a ticket to unify the query in both places. Linking this for reference #172448

cc @kdelemme since you wrote this part, I am tagging you

Technically, instanceId can be '*' and the alert stores the instanceId as * as well, so I'm not sure this filter adds something? but I need to try the query locally

…d-config

mgiota · 2024-01-30T12:20:35Z

x-pack/plugins/observability/public/embeddable/slo/overview/slo_configuration.tsx

@@ -57,6 +65,21 @@ export function SloConfiguration({ onCreate, onCancel }: SloConfigurationProps)
            />
          </EuiFlexItem>
        </EuiFlexGroup>
+        {selectedSlo?.sloInstanceId !== '*' && (


Let's use ALL_VALUE here

mgiota · 2024-01-31T08:06:36Z

@shahzad31 I have an alerts embeddable. I have enabled the Show all related group by instances, but I would expect to see:

4 SLOs included on the top right link of the embeddable

in the SLO popup configuration I would expect to see the rest 3 group by instances as well

Screen.Recording.2024-01-30.at.10.17.09.mov

Also let's think more about the workflow, when user edits the configuration and disables the toggle. In this case we should exclude the alerts from the related group by instances.

PR looks good. What is still pending though for me to approve it is input from @maciejforcone regarding this one.

x-pack/plugins/observability/public/embeddable/slo/overview/slo_configuration.tsx

…o_configuration.tsx

x-pack/plugins/observability/public/embeddable/slo/overview/slo_configuration.tsx

…o_configuration.tsx

maciejforcone

Looks good to me

mgiota · 2024-01-31T19:40:29Z

Just updating here the status: According to our discussion we will display 1 SLO (5 instances)

…d-config

mgiota

Do you plan to add the 1 SLO (5 instances) as part of this PR or in a follow up?

…d-config

…into slo-card-config

kibana-ci · 2024-02-01T09:42:20Z

💚 Build Succeeded

Buildkite Build
Commit: 0972b0a

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	597	599	+2

Async chunks

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

id	before	after	diff
`observability`	626.5KB	627.0KB	+532.0B

History

💔 Build #190659 failed 00e347a
💚 Build #190386 succeeded d63412d
💚 Build #190151 succeeded 53113ba
💚 Build #189809 succeeded 325d59a
💛 Build #189217 was flaky 5d526d8

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

mgiota

I am trying to test your PR locally, but for some reason when I create a Group by SLO the Transform becomes unavailable.

To not block you, I will approve the PR. Before merging, please make sure that you don't have any issue with transforms in your branch and verify that counting works well with a few scenarios we discussed.

User selects 2 SLOs (different SLO id) that both have group by instances. Then we should sum up all group by instances from the different SLO ids
user manually selects 2 SLOs (both same SLO id, but different instances) and enables the toggle. If total group by instances were 5 for example, we should make sure we display 2 SLOs with (3 instances) and not 2 SLOs (with 5 instances)

I also leave a small note regarding the change in the activeFetchAlerts hook. A bit more testing is required to verify nothing broke in the SLO pages.

shahzad31 · 2024-02-01T17:01:12Z

@elasticmachine run elasticsearch-ci/doc

) ## Summary in SLOs embeddable, added the ability show all related instances Overviews and alerts !! if selected SLO has group-by, it will give an option to show all related instances from the group-by field <img width="829" alt="image" src="https://github.com/elastic/kibana/assets/3505601/f57ad76b-4170-4fc0-8335-c0f0b2a8807e"> All related instances will appear as grid <img width="1728" alt="image" src="https://github.com/elastic/kibana/assets/3505601/c43f058a-1b43-4e44-807d-4157e1f4a2be">

shahzad31 added 3 commits January 25, 2024 07:45

update tests

6f08f41

Merge branch 'main' of https://github.com/elastic/kibana into slo-car…

6848f2e

…d-config

update to show all related instances

180de9e

shahzad31 requested review from a team as code owners January 25, 2024 07:38

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

shahzad31 added the release_note:skip Skip the PR/issue when compiling release notes label Jan 25, 2024

shahzad31 commented Jan 25, 2024

View reviewed changes

.../plugins/triggers_actions_ui/public/application/sections/alerts_table/alerts_table_state.tsx Outdated Show resolved Hide resolved

Update x-pack/plugins/triggers_actions_ui/public/application/sections…

5d526d8

…/alerts_table/alerts_table_state.tsx

shahzad31 removed the request for review from a team January 25, 2024 08:58

Merge branch 'main' into slo-card-config

325d59a

mgiota self-requested a review January 30, 2024 07:16

mgiota reviewed Jan 30, 2024

View reviewed changes

shahzad31 added 3 commits January 30, 2024 09:44

Merge branch 'main' of https://github.com/elastic/kibana into slo-car…

2e30069

…d-config

use ALL_VALUE

c0d0d07

Merge branch 'slo-card-config' of https://github.com/shahzad31/kibana …

dded4ff

…into slo-card-config

mgiota requested changes Jan 30, 2024

View reviewed changes

shahzad31 added 2 commits January 30, 2024 11:39

fix alerts query

c974f12

Merge branch 'main' of https://github.com/elastic/kibana into slo-car…

5e76b34

…d-config

mgiota reviewed Jan 30, 2024

View reviewed changes

shahzad31 added 2 commits January 30, 2024 12:38

reload on save

246ecfd

Merge branch 'main' of https://github.com/elastic/kibana into slo-car…

53113ba

…d-config

mgiota reviewed Jan 30, 2024

View reviewed changes

shahzad31 requested a review from mgiota January 30, 2024 18:25

mgiota requested a review from maciejforcone January 31, 2024 08:06

shahzad31 commented Jan 31, 2024

View reviewed changes

x-pack/plugins/observability/public/embeddable/slo/overview/slo_configuration.tsx Outdated Show resolved Hide resolved

Update x-pack/plugins/observability/public/embeddable/slo/overview/sl…

07c02b4

…o_configuration.tsx

shahzad31 commented Jan 31, 2024

View reviewed changes

x-pack/plugins/observability/public/embeddable/slo/overview/slo_configuration.tsx Show resolved Hide resolved

Update x-pack/plugins/observability/public/embeddable/slo/overview/sl…

d63412d

…o_configuration.tsx

maciejforcone approved these changes Jan 31, 2024

View reviewed changes

shahzad31 added 2 commits January 31, 2024 21:17

Merge branch 'main' of https://github.com/elastic/kibana into slo-car…

fc88574

…d-config

instances

f990859

mgiota reviewed Jan 31, 2024

View reviewed changes

shahzad31 added 4 commits February 1, 2024 08:25

Merge branch 'main' of https://github.com/elastic/kibana into slo-car…

1fe1d49

…d-config

Merge branch 'slo-card-config' of https://github.com/shahzad31/kibana …

7f375c8

…into slo-card-config

update

00e347a

fix 1i18n

0972b0a

mgiota self-requested a review February 1, 2024 13:55

mgiota approved these changes Feb 1, 2024

View reviewed changes

shahzad31 merged commit 4f7402b into elastic:main Feb 1, 2024
17 checks passed

shahzad31 deleted the slo-card-config branch February 1, 2024 18:06

kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Feb 1, 2024

mgiota mentioned this pull request Feb 20, 2024

[SLO embeddable] Adding items to SLO alert overview can lose items #177267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SLOs] In Embeddables, show all related instances option #175503

[SLOs] In Embeddables, show all related instances option #175503

shahzad31 commented Jan 25, 2024 •

edited

Loading

elasticmachine commented Jan 25, 2024

apmmachine commented Jan 25, 2024

mgiota commented Jan 25, 2024

mgiota commented Jan 30, 2024 •

edited

Loading

mgiota Jan 30, 2024

mgiota Jan 30, 2024

mgiota Jan 31, 2024

mgiota Jan 30, 2024

mgiota Jan 30, 2024

mgiota commented Jan 30, 2024

mgiota commented Jan 30, 2024 •

edited

Loading

mgiota commented Jan 30, 2024

mgiota Jan 30, 2024 •

edited

Loading

mgiota Jan 30, 2024

kdelemme Jan 31, 2024

mgiota Jan 30, 2024

mgiota commented Jan 31, 2024

maciejforcone left a comment

mgiota commented Jan 31, 2024

mgiota left a comment

kibana-ci commented Feb 1, 2024

mgiota left a comment

shahzad31 commented Feb 1, 2024


		const onConfirmClick = () => onCreate({ slos: selectedSlos, showAllGroupByInstances });

		const hasGroupBy = selectedSlos?.some((slo) => slo.instanceId !== '*');

[SLOs] In Embeddables, show all related instances option #175503

[SLOs] In Embeddables, show all related instances option #175503

Conversation

shahzad31 commented Jan 25, 2024 • edited Loading

Summary

elasticmachine commented Jan 25, 2024

apmmachine commented Jan 25, 2024

🤖 GitHub comments

mgiota commented Jan 25, 2024

mgiota commented Jan 30, 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

mgiota commented Jan 30, 2024

mgiota commented Jan 30, 2024 • edited Loading

mgiota commented Jan 30, 2024

mgiota Jan 30, 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

mgiota commented Jan 31, 2024

maciejforcone left a comment

Choose a reason for hiding this comment

mgiota commented Jan 31, 2024

mgiota left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 1, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

mgiota left a comment

Choose a reason for hiding this comment

shahzad31 commented Feb 1, 2024

shahzad31 commented Jan 25, 2024 •

edited

Loading

mgiota commented Jan 30, 2024 •

edited

Loading

mgiota commented Jan 30, 2024 •

edited

Loading

mgiota Jan 30, 2024 •

edited

Loading