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

[Actionable Observability] - Formate Actual value and the Expected value in Alert Details #147025

Conversation

fkanout
Copy link
Contributor

@fkanout fkanout commented Dec 5, 2022

Summary

Closes #145409 by formatting the Actual value and the Expected value.

@fkanout fkanout self-assigned this Dec 5, 2022
@fkanout fkanout added v8.7.0 Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" release_note:skip Skip the PR/issue when compiling release notes labels Dec 5, 2022
@fkanout fkanout changed the title Move the AlertSummary to the shared folder in o11y [Actionable Observability] Move the AlertSummary into the AlertDetailsAppSection in the solutions Dec 5, 2022
@fkanout fkanout marked this pull request as ready for review December 15, 2022 16:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/actionable-observability (Team: Actionable Observability)

Copy link
Contributor

@CoenWarmer CoenWarmer left a comment

Choose a reason for hiding this comment

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

Looks good already, small remark.

ruleTypeId: string
): AlertEvaluationUnitType => {
switch (ruleTypeId) {
case 'apm.transaction_duration':
Copy link
Member

Choose a reason for hiding this comment

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

Just thinking out loud:
Does it make sense that the Observability plugin knows about specific details of the APM rule types? I am wondering if this logic is something that should be provided by each app or if we should handle it like this in the Observability plugin. (Especially if we will have similar scenarios for each rule type)
@fkanout @simianhacker What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maryam-saeidi, it's a good question. The goal was to make the AlertSummary sharable, and then each App will import it and then use their logic/formatter.

However, when I started implementation, I realized that

  • APM (most likely other Apps) don't have formatters, AND they are importing and using Observability formatter.
  • Doing the formatting at the Rule level is not ideal, as we only have three types of values that need to be formatted across all rule types.

Copy link
Member

@maryam-saeidi maryam-saeidi Dec 16, 2022

Choose a reason for hiding this comment

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

Interesting.

It makes sense that Observability shares formatter since the formatting logic can be re-used in different rule types.

The concern here is to spread the logic related to rules in multiple plugins and make the Observability plugin unnecessarily aware of details of each rule type and as a result, in case of any change or addition, to always update this plugin as well. So basically, we are affecting the separation of concern here.

Doing the formatting at the Rule level is not ideal, as we only have three types of values that need to be formatted across all rule types.

Also, can you please elaborate on why it is not ideal? What issues it might cause?

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.

Tested locally, and looks nice! 👍🏻
image

@fkanout fkanout requested a review from CoenWarmer December 16, 2022 11:18
@fkanout fkanout enabled auto-merge (squash) December 16, 2022 11:53
@fkanout fkanout merged commit c3a8b26 into elastic:main Dec 16, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #48 / APM API tests basic no data Service group counts with alerts "before all" hook for "returns the correct number of alerts"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 460 462 +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 502.7KB 503.0KB +288.0B

Page load bundle

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

id before after diff
observability 79.5KB 79.6KB +108.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 61 67 +6
osquery 109 115 +6
securitySolution 440 446 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 70 76 +6
osquery 110 117 +7
securitySolution 517 523 +6
total +21

History

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

cc @fkanout

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 16, 2022
@fkanout fkanout changed the title [Actionable Observability] Move the AlertSummary into the AlertDetailsAppSection in the solutions [Actionable Observability] Formatting Actual value and the Expected value in Alert Details Dec 21, 2022
@fkanout fkanout changed the title [Actionable Observability] Formatting Actual value and the Expected value in Alert Details [Actionable Observability] - Formate Actual value and the Expected value in Alert Details Dec 21, 2022
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 release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Actionable Observability] - Format Actual value and the Expected value in the Alert details page of APM rule
6 participants