Onboard Inventory Metric Threshold rule type with FAAD #178506

ersin-erdal · 2024-03-12T14:08:53Z

This PR onboards Inventory Metric Threshold rule type with FAAD.

To verify.

I used data-generator to generate metric data.

Then created an Inventory Threshold rule with actions (alert and recovered),
conitions: For Hosts, When CPU usage is above 10.
Inventory Threshold uses the following formula to calculate the result:

(system.cpu.user.pct + system.cpu.system.pct) / system.cpu.cores

Set
system.cpu.user.pct = 1
system.cpu.system.pct = 1
system.cpu.cores = 4
in the cpu-001. This makes the CPU usage 0.5 (50%) for the host-1

and run the generator with ./generate metrics

Your rule should create an alert and should saved it in .internal.alerts-observability.metrics.alerts-default-000001

Then set system.cpu.user.pct=0 and system.cpu.system.pct=0.

The alert should be recovered and the AAD in the above index should be updated kibana.alert.status: recovered.

ersin-erdal · 2024-03-12T14:09:01Z

/ci

ersin-erdal · 2024-03-13T15:11:02Z

/ci

elasticmachine · 2024-03-13T17:11:25Z

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

ymao1 · 2024-03-13T17:36:12Z

.../infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts

-  evaluationValues?: Array<number | null>,
-  thresholds?: Array<number | null>
-) => InventoryMetricThresholdAlert;
-
 export const createInventoryMetricThresholdExecutor = (libs: InfraBackendLibs) =>
  libs.metricsRules.createLifecycleRuleExecutor<


we need to remove this usage of createLifecycleRuleExecutor so that this no longer goes through the rule registry

I totally missed that! Thanks for pointing out.
Fixed with 5c5315f

pmuellr

LGTM; noted some potential source changes to reduce casts. Also left a question about the threshold values when using a "between" comparator, but the behaviour here (not writing threshold values to the alerts docs) seems the same as es query, so perhaps it's as good as we can do today.

pmuellr · 2024-03-14T19:46:28Z

...ver/lib/alerting/inventory_metric_threshold/register_inventory_metric_threshold_rule_type.ts

+    alerts: {
+      ...MetricsRulesTypeAlertDefinition,
+      shouldWrite: true,
+    } as IRuleTypeAlerts<InventoryMetricThresholdAlert>,


I tried to get rid of this cast by changing MetricsRulesTypeAlertDefinition from IRuleTypeAlerts to IRuleTypeAlerts<InventoryMetricThresholdAlert> in x-pack/plugins/observability_solution/infra/server/lib/alerting/register_rule_types.ts, and it seemed to like it - I was able to remove the cast here completely anyway. Had to import InventoryMetricThresholdAlert in the register_rule_types.ts module ...

Though looking at that MetricsRulesTypeAlertDefinition, it has shouldWrite: false. Can we change the definition to shouldWrite: true and then remove the override here?

There are multiple rule types using the MetricsRulesTypeAlertDefinition. If all of them are onboarded to the framework AAD and no longer using the lifecycle executor, then we can set shouldWrite: true on the MetricsRulesTypeAlertDefinition and use it as is without the override.

We already talked about it with Alexi.
Whoever merges their PR last, can remove this line :)

This done now.

pmuellr · 2024-03-14T20:02:18Z

.../infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts

+> & {
+  // Defining a custom type for this because the schema generation script doesn't allow explicit null values
+  [ALERT_EVALUATION_VALUES]?: Array<number | null>;
+  [ALERT_EVALUATION_THRESHOLD]?: Array<number | null>;


seems like THRESHOLD isn't omitted above ... I was able to add it to the Omit<> with no errors ...

Fixed it, thanks.

pmuellr · 2024-03-14T20:37:18Z

.../infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts

+          [ALERT_REASON]: reason,
+          [ALERT_ACTION_GROUP]: actionGroup,
+          [ALERT_EVALUATION_VALUES]: evaluationValues,
+          [ALERT_EVALUATION_THRESHOLD]: thresholds,


I tried a "between" comparator, which should be passing an array of values to thresholds. The rule works fine, but this field is not set in the alert document. For a single-value threshold (anything but between or not between), it printed the threshold fine.

This may be a pre-existing issue with this rule type - I noticed that es query seems to not populate this field if it's multi-value as well, perhaps that's the pattern for these?

I think this is intentional:
https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability_solution/infra/server/lib/alerting/common/get_values.ts#L34

We don't save threshold if at least one of the comparators is a multiple threshold one such as 'in between'

@pmuellr @ersin-erdal I've created a ticket for this topic to save the range threshold as an array. I also saw the the same behaviour in the es query rule, what do you think about adding the range threshold as an array in all rules (including es query)?

That would be good, but I suspect we will have pre-existing data with just a scalar value - so any typing on such fields in Typescript will have to be number | number[].

…67-inventory-faad

ymao1 · 2024-03-19T11:58:57Z

.../infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts

-          fields: {
+        alertsClient.setAlertData({
+          id: UNGROUPED_FACTORY_KEY,
+          payload: {
            [ALERT_REASON]: reason,
            [ALERT_ACTION_GROUP]: actionGroup,


we shouldn't need to report this back since it's set by the framework.

ymao1 · 2024-03-19T11:59:29Z

.../infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts

+
+    const esClient = services.scopedClusterClient.asCurrentUser;
+
+    const { savedObjectsClient, alertFactory: baseAlertFactory, alertsClient } = services;


do we need the baseAlertFactory anymore?

…67-inventory-faad

maryam-saeidi

Tested creating alerts and recover them + notification, everything seemed as expected! 💪🏻

Regarding code, I've checked it but I rely mostly on ResponseOps team review :)

kibana-ci · 2024-03-22T12:28:49Z

💚 Build Succeeded

Buildkite Build
Commit: 78e58c6

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`infra`	9	7	-2

History

💛 Build #198322 was flaky ccc902c
💚 Build #198085 succeeded e9508cb
💔 Build #198032 failed ca70dd5
💔 Build #197978 failed c8d3f2e
💚 Build #197537 succeeded 5c5315f
💔 Build #197513 failed e83ea2e

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

cc @ersin-erdal

Onboard Inventory Metric Threshold rule type with FAAD

42455ee

ersin-erdal 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.14.0 labels Mar 12, 2024

ersin-erdal self-assigned this Mar 12, 2024

Add a unit test

8bbcacf

ersin-erdal requested a review from umbopepato March 13, 2024 17:11

ersin-erdal marked this pull request as ready for review March 13, 2024 17:11

ersin-erdal requested a review from a team as a code owner March 13, 2024 17:11

ersin-erdal requested a review from pmuellr March 13, 2024 17:12

ymao1 reviewed Mar 13, 2024

View reviewed changes

ersin-erdal added 3 commits March 13, 2024 18:50

fix should write to scope only inventory rule type

3574f81

revert changes on register_rule_types.ts

e83ea2e

remove createLifecycleRuleExecutor

5c5315f

pmuellr approved these changes Mar 14, 2024

View reviewed changes

ersin-erdal and others added 5 commits March 18, 2024 13:57

omit threshold

c8d3f2e

Merge branch 'main' into 169867-inventory-faad

9cc4bc7

Fix shouldWrite

9c114c2

Merge remote-tracking branch 'origin/169867-inventory-faad' into 1698…

ca70dd5

…67-inventory-faad

Merge branch 'main' into 169867-inventory-faad

e9508cb

ymao1 reviewed Mar 19, 2024

View reviewed changes

ersin-erdal added 2 commits March 19, 2024 17:14

Fix nits

12a9d24

Merge remote-tracking branch 'origin/169867-inventory-faad' into 1698…

ccc902c

…67-inventory-faad

maryam-saeidi self-requested a review March 21, 2024 09:17

maryam-saeidi approved these changes Mar 21, 2024

View reviewed changes

Merge branch 'main' into 169867-inventory-faad

78e58c6

ersin-erdal merged commit f35b1ae into elastic:main Mar 22, 2024
24 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Mar 22, 2024

ersin-erdal deleted the 169867-inventory-faad branch March 22, 2024 12:58

thomheymann mentioned this pull request Mar 26, 2024

[Observability Onboarding] Link all “Add data” buttons in observability to onboarding landing page #179459

Merged

maryam-saeidi mentioned this pull request Mar 27, 2024

Save range thresholds in ALERT_EVALUATION_THRESHOLD for observability rules #178067

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Onboard Inventory Metric Threshold rule type with FAAD #178506

Onboard Inventory Metric Threshold rule type with FAAD #178506

ersin-erdal commented Mar 12, 2024 •

edited

Loading

ersin-erdal commented Mar 12, 2024

ersin-erdal commented Mar 13, 2024

elasticmachine commented Mar 13, 2024

ymao1 Mar 13, 2024

ersin-erdal Mar 13, 2024

pmuellr left a comment

pmuellr Mar 14, 2024

ymao1 Mar 14, 2024

ersin-erdal Mar 14, 2024

ersin-erdal Mar 18, 2024

pmuellr Mar 14, 2024

ersin-erdal Mar 18, 2024

pmuellr Mar 14, 2024

ersin-erdal Mar 18, 2024 •

edited

Loading

maryam-saeidi Mar 21, 2024

pmuellr Mar 21, 2024

ymao1 Mar 19, 2024

ersin-erdal Mar 19, 2024

ymao1 Mar 19, 2024

ersin-erdal Mar 19, 2024

maryam-saeidi left a comment •

edited

Loading

kibana-ci commented Mar 22, 2024

References to deprecated APIs


		const esClient = services.scopedClusterClient.asCurrentUser;

		const { savedObjectsClient, alertFactory: baseAlertFactory, alertsClient } = services;

Onboard Inventory Metric Threshold rule type with FAAD #178506

Onboard Inventory Metric Threshold rule type with FAAD #178506

Conversation

ersin-erdal commented Mar 12, 2024 • edited Loading

To verify.

ersin-erdal commented Mar 12, 2024

ersin-erdal commented Mar 13, 2024

elasticmachine commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuellr left a 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

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

ersin-erdal Mar 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryam-saeidi left a comment • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Mar 22, 2024

💚 Build Succeeded

Metrics [docs]

References to deprecated APIs

History

ersin-erdal commented Mar 12, 2024 •

edited

Loading

ersin-erdal Mar 18, 2024 •

edited

Loading

maryam-saeidi left a comment •

edited

Loading