Skip to content
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] Alerts embeddable #169910

Merged
merged 100 commits into from
Dec 5, 2023
Merged

Conversation

mgiota
Copy link
Contributor

@mgiota mgiota commented Oct 26, 2023

Resolves #171354
Resolves #170613
Resoves #172274
Resolves #172453

This PR adds the shareable Alerts table from the Response Ops team as an embeddable to the Dashboard app.

  • Users can view alerts from multiple SLOs,
  • Users can edit the configuration and update the selected SLOs
  • Users can interact with the alerts table (open flyout, select the SLOs and add to cases, create new case)
  • Shareable alerts table displays custom columns: status, rule name, duration, reason. Actions column is hidden.
  • It should link to o11y alerts page filtered for selected slo ids and instance ids
  • It should show number of selected SLOs. Link should open the edit configuration popup, showing the selected SLOs
  • Table gets updated upon datepicker selection
  • Embeddable is available only to users with platinum license and above
  • It should work on serverless
  • Screenshot reporting under Stack Management > Reporting works fine without any timeout error
Screen.Recording.2023-11-21.at.12.02.03.mov

🗒️ Notes for the reviewer

There are a few issues with the current integration of the sharable alerts table, that I will fix as a separate PR:

  • Sometimes on initial load there is a loading indicator on the alerts table that doesn't go away. If I refresh the page, it will most probably disappear. The loading indicator issue started after I used a custom alerts table configuration in order to customize the default table columns. When I used the already registered o11y AlertsTableConfiguration, there was no issue with the loading indicator. I will create a separate issue for this with more details.
  • When I hover on the Reason column, the whole row jumps.
  • Table displays rule name. Ideally we should display slo name. This will require more changes in the alerts as data technical field map.

@apmmachine
Copy link
Contributor

🤖 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 mgiota force-pushed the slo_embeddable_alerts_table branch 2 times, most recently from 7399016 to 8a1830e Compare October 27, 2023 08:14
@mgiota mgiota force-pushed the slo_embeddable_alerts_table branch from 8a1830e to 4f6dbe7 Compare October 27, 2023 08:16
@mgiota
Copy link
Contributor Author

mgiota commented Oct 27, 2023

I tried to integrate the alerts table from TriggersActionsUI plugin like this, but I get a blank page:

<AlertsStateTable
            query={{
              bool: {
                filter: [
                  // { term: { 'slo.id': sloId } },
                  // { term: { 'slo.instanceId': sloInstanceId ?? ALL_VALUE } },
                  { term: { 'slo.id': '7bd92700-743d-11ee-bd9f-0fb31b48b974' } }, // TEMP hardcode it, until I implement explicit input
                  { term: { 'slo.instanceId': ALL_VALUE } },
                ],
              },
            }}
            alertsTableConfigurationRegistry={alertsTableConfigurationRegistry}
            configurationId={AlertConsumers.OBSERVABILITY}
            featureIds={[AlertConsumers.SLO]}
            hideLazyLoader
            id={ALERTS_TABLE_ID}
            pageSize={ALERTS_PER_PAGE}
            showAlertStatusWithFlapping
          />
Screenshot 2023-10-27 at 10 17 10

What do I miss here? Is this even possible to integrate current AlertsStateTable as an embeddable in the dashboard app?

Note: I hard coded the slo id for now, because I haven't implemented yet the option to select a specific SLO.

UPDATE Above issue is apparently fixed by passing proper context to the alerts table embeddable.

@mgiota mgiota force-pushed the slo_embeddable_alerts_table branch from a761ea8 to 5a59d64 Compare November 10, 2023 11:59
@mgiota mgiota self-assigned this Nov 13, 2023
@shahzad31
Copy link
Contributor

@XavierM we have addressed all of your feedback, i don't think there is anything blocking anymore. Please give it another round of review.

@XavierM
Copy link
Contributor

XavierM commented Dec 5, 2023

@shahzad31 did you also fix that?
#169910 (comment)

@shahzad31
Copy link
Contributor

@shahzad31 did you also fix that? #169910 (comment)

Yes both have been accounted for.

  1. Refresh button now works
  2. For SLO configurations we now load 100 on first page and have improve search so that user can search against slo name or slo instance ID. We decided that load on scroll doesn't make sense.

Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 Can you make the changes we discussed for the query.

Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM... Tested multiple SLOs with the same instanceId to ensure they don't show up in the wrong visualization. The changes on the Overview side look good to.

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM => Everything is working as expected

I was able to search for different SLO configuration. I can also refresh the embeddable alert table from a dashboard. And the query to access alert from an SLO is working like a charm.

Thank you for bearing with me <3, I tried be more diligent in my testing now that main can be push at anytime in server-less.

@kibana-ci
Copy link
Collaborator

kibana-ci commented Dec 5, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 531 545 +14

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
observability 597 600 +3

Async chunks

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

id before after diff
observability 1.1MB 1.1MB +49.5KB
triggersActionsUi 1.4MB 1.4MB +227.0B
total +49.7KB

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5666 +5666
total size - 5.9MB +5.9MB

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.8KB 102.5KB +752.0B
Unknown metric groups

API count

id before after diff
observability 606 609 +3

async chunk count

id before after diff
observability 15 21 +6

ESLint disabled line counts

id before after diff
observability 38 37 -1

References to deprecated APIs

id before after diff
observability 4 6 +2

Total ESLint disabled count

id before after diff
observability 43 42 -1

History

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

cc @mgiota

@shahzad31 shahzad31 merged commit 52a7e50 into elastic:main Dec 5, 2023
43 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 5, 2023
@mgiota
Copy link
Contributor Author

mgiota commented Dec 5, 2023

Something doesn't work well for me with the slo selection

Screen.Recording.2023-12-05.at.19.14.41.mov

@mgiota
Copy link
Contributor Author

mgiota commented Dec 5, 2023

I restarted ES and issue is not reproducible any more.

@mgiota
Copy link
Contributor Author

mgiota commented Dec 5, 2023

/oblt-deploy-serverless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:build-serverless-image release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.12.0
Projects
None yet
10 participants