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

[ResponseOps][Rules] Use rule form instead of rule flyout in observability solution #206774

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

js-jankisalvi
Copy link
Contributor

@js-jankisalvi js-jankisalvi commented Jan 15, 2025

Summary

Resolves #195574

This PR updates observability solution to use new rule form to create and edit rules same as stack management > rules page.
It removes usage of rule flyout form o11y solution. Also updated functional tests.

Checklist

How to test

  • Create a rule in o11y, verify it works as expected
  • Edit rule in o11y via different options (from rule details page, rule list table, alert details page etc.) verify it works as expected
  • Verify the same in serverless o11y project

Release Note

Use rule form to create or edit rules in observability.

@js-jankisalvi js-jankisalvi self-assigned this Jan 15, 2025
@js-jankisalvi js-jankisalvi changed the title [ResponseOps][Rules] Use new rule form in Observability solution [ResponseOps][Rules] Use rule form instead of rule flyout in observability solution Jan 20, 2025
@js-jankisalvi js-jankisalvi added v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.18.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:enhancement labels Jan 20, 2025
@js-jankisalvi js-jankisalvi marked this pull request as ready for review January 20, 2025 18:06
@js-jankisalvi js-jankisalvi requested review from a team as code owners January 20, 2025 18:06
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-management Observability Management User Experience Team labels Jan 20, 2025
@elasticmachine
Copy link
Contributor

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

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.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@maryam-saeidi maryam-saeidi self-requested a review January 22, 2025 08:26
@maryam-saeidi
Copy link
Member

Thanks for making this change 🙏🏻

I haven't finished the review, just an early feedback:

  1. When editing the rule in observability, there is an extra Edit title, can we remove that?

    Observability Stack manegemnt
    image image
  2. When clicking on the breadcrumb to navigate to Rules, in observability, it will refresh the whole page as shown below:

    Screen.Recording.2025-01-22.at.10.05.27.mov

@js-jankisalvi
Copy link
Contributor Author

  1. When editing the rule in observability, there is an extra Edit title, can we remove that?
  2. When clicking on the breadcrumb to navigate to Rules, in observability, it will refresh the whole page as shown below:

Fixed both issues in 134313d

breadcrumb_obs.mov

@@ -53,7 +55,7 @@ export const getInitialMultiConsumer = ({
}

// If o11y is in the valid consumers, just use that
if (validConsumers.includes(AlertConsumers.OBSERVABILITY)) {
if (isServerless && validConsumers.includes(AlertConsumers.OBSERVABILITY)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because validConsumers of o11y solution always has AlertConsumers.OBSERVABILITY, however we don't want it for some rules like (custom threshold, esql etc.) which have other consumers like logs, metrics etc.
However if it is serverless, we want all rules to have AlertConsumers.OBSERVABILITY consumer.

Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Just to add the decision here: (based on discussion with @maciejforcone)
We decided to only have the rule form in the rule creation flow on the rules page. We will keep the flyout for other scenarios (i.e., edit and rule creation on the app pages.)

@shahzad31
Copy link
Contributor

Just to add the decision here: (based on discussion with @maciejforcone) We decided to only have the rule form in the rule creation flow on the rules page. We will keep the flyout for other scenarios (i.e., edit and rule creation on the app pages.)

@maryam-saeidi that sounds logical, but i think we should make those flyouts at least resizeable, i think the actual place like expression and graphs on the flyout is usually very congested, either we can increase the size or make the fields above expression compressed to give maximum space to the content which matters on flyouts. I think purpose of this initiative was to address these things but if we still kept those as flyouts, it kinda defeats the purpose.

@elasticmachine
Copy link
Contributor

elasticmachine commented Jan 23, 2025

💚 Build Succeeded

  • Buildkite Build
  • Commit: 0113eec
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-206774-0113eec316d0

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 1150 1269 +119

Async chunks

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

id before after diff
observability 482.1KB 1.3MB ⚠️ +814.8KB
triggersActionsUi 1.7MB 1.7MB +171.0B
total +815.0KB

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.9KB 102.0KB +128.0B
Unknown metric groups

async chunk count

id before after diff
observability 20 21 +1

miscellaneous assets size

id before after diff
observability 888.1KB 1.2MB ⚠️ +379.0KB

History

cc @js-jankisalvi

@maryam-saeidi
Copy link
Member

@maryam-saeidi that sounds logical, but i think we should make those flyouts at least resizeable, i think the actual place like expression and graphs on the flyout is usually very congested, either we can increase the size or make the fields above expression compressed to give maximum space to the content which matters on flyouts. I think purpose of this initiative was to address these things but if we still kept those as flyouts, it kinda defeats the purpose.

Good point. The plan is to have the option to resize the flyout but in a different step. @cnasikas shared that this option will be easier to add when @Zacqary 's work on the rule form is finished (I believe it is related to this PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:enhancement Team:obs-ux-management Observability Management User Experience Team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Observability] Use the new rule form page when creating rules from within the observability application
5 participants