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

[RAM] add observability feature for server less #168636

Merged
merged 42 commits into from
Oct 31, 2023

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Oct 11, 2023

Summary

FIX => #168034

Checklist

@mgiota mgiota self-requested a review October 11, 2023 19:46
@XavierM XavierM added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.12.0 labels Oct 12, 2023
@fkanout fkanout self-requested a review October 16, 2023 07:08
Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

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

After an initial review with local testing, the rule appeared on Serverless, and it fires alerts. 🎊
Screenshot 2023-10-16 at 09 51 52

Screenshot 2023-10-16 at 09 51 35

Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

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

@XavierM, I wanted to check the Alert doc via DevTool, but I couldn't find the index. We are hiding these indices on serverless?

@XavierM
Copy link
Contributor Author

XavierM commented Oct 16, 2023

@XavierM, I wanted to check the Alert doc via DevTool, but I couldn't find the index. We are hiding these indices on serverless?

I will just use .alerts*, that's the simple way.

@mgiota
Copy link
Contributor

mgiota commented Oct 16, 2023

@fkanout I see in your screenshot you are testing Custom threshold rule. Based on #167782, shouldn't Custom threshold be disabled on serverless? I am asking, since I want to write some tests for this PR and I want to know, what makes sense to test and what not.

@fkanout
Copy link
Contributor

fkanout commented Oct 17, 2023

@fkanout I see in your screenshot you are testing Custom threshold rule. Based on #167782, shouldn't Custom threshold be disabled on serverless? I am asking, since I want to write some tests for this PR and I want to know, what makes sense to test and what not.

@mgiota, that's true, It's disabled by default for serverless. But as this PR is serverless-related only. In order to test it, you need to switch the feature flag on as I did.
The PR is solving the issue that even after turning the feature flag ON, the rule was not appearing.

@fkanout
Copy link
Contributor

fkanout commented Oct 17, 2023

also fixes #167053

@XavierM

This comment was marked as resolved.

@XavierM XavierM marked this pull request as ready for review October 18, 2023 14:13
@XavierM XavierM requested review from a team as code owners October 18, 2023 14:13
@elasticmachine
Copy link
Contributor

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

@kdelemme kdelemme self-assigned this Oct 18, 2023
Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

ResponseOps changes look good

Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the active alerts hooks 👍🏻 LGTM

@XavierM XavierM enabled auto-merge (squash) October 31, 2023 20:13
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 488 489 +1

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
@kbn/rule-data-utils 111 112 +1

Async chunks

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

id before after diff
apm 3.7MB 3.7MB +6.7KB
discover 587.0KB 587.2KB +195.0B
observability 1.1MB 1.1MB +101.0B
securitySolution 12.9MB 12.9MB +756.0B
stackConnectors 525.4KB 525.5KB +185.0B
triggersActionsUi 1.4MB 1.4MB +276.0B
total +8.2KB

Page load bundle

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

id before after diff
apm 38.5KB 36.3KB -2.2KB
cases 155.0KB 155.2KB +185.0B
infra 106.7KB 106.9KB +189.0B
observability 103.9KB 104.1KB +208.0B
stackAlerts 23.7KB 23.9KB +186.0B
synthetics 19.4KB 19.6KB +189.0B
triggersActionsUi 97.0KB 97.2KB +189.0B
uptime 22.7KB 22.8KB +188.0B
total -906.0B
Unknown metric groups

API count

id before after diff
@kbn/rule-data-utils 114 115 +1

ESLint disabled line counts

id before after diff
triggersActionsUi 125 126 +1

Total ESLint disabled count

id before after diff
triggersActionsUi 131 132 +1

History

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

@XavierM XavierM merged commit a35f91e into elastic:main Oct 31, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 31, 2023
@mgiota
Copy link
Contributor

mgiota commented Nov 3, 2023

@XavierM I think we need to add "observability" under feature_ids in this hook as well:
https://github.com/kdelemme/kibana/blob/b154371821f1ccdcd2336c43b87f41e92517afdf/x-pack/plugins/observability/public/hooks/slo/use_fetch_active_alerts.ts#L81-L85

@XavierM Kevin's valid comment put me into thoughts regarding the feature_ids we pass to other observability rule types. These will need to be changed too, otherwise generated alerts will probably not be displayed in the respective apps on serverless, if my understanding is correct,right?

One example is here apm latency alerts history chart. I can gather the places that need to be changed and fix them. Can you please verify that I need to make changes to rest observability places that specify feature ids? Maybe it is not needed, I need to revisit again how the alert authorization works. I need your input for verification.

delanni pushed a commit to delanni/kibana that referenced this pull request Nov 6, 2023
## Summary

FIX => elastic#168034


### Checklist

- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: mgiota <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
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:APM All issues that need APM UI Team support Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.