[Custom threshold] Add prefill functionality to the custom threshold rule

[maryam-saeidi]

Closes #170295

Summary

This PR adds prefill functionality to the custom threshold rule. Since we don't have any usage for it right now (it will be used in logs and infra apps), the only way to check it is by looking at tests.

[apmmachine]

🤖 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!)

[elasticmachine]

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

[fkanout]

The Metric Explorer provides three more aggregation types (p99, p95, and Rate) than the Custom threshold rule (here is the issue that tracks adding these Agg types #172945).
We should add the missing Aggs to the rule first, then provide the pre-fill feature. Otherwise, we will miss the goal of this PR and deliver an uncompleted feature/bug, as we don't offer or handle these Aggs yet.

[fkanout]

@maryam-saeidi, why can't we manually test the PR?
I found that the Metric Explorer already had the option to create a Custom Threshold Rule based on defined parameters like the Metric one; we only need to enable the FF.
However, I enabled the FF, and the prefilling is not working as expected, as the screen recording shows. e.g., the groupBy is always host.name
What do you think the issue is?

Screen.Recording.2023-12-09.at.11.08.31.mov

Regarding the Feature Flag, I think as we deprecated it, it should be enabled by default/deleted; what do you think, @benakansara?

[benakansara]

Regarding the Feature Flag, I think as we deprecated it, it should be enabled by default/deleted; what do you think, @benakansara?

Good point. I checked the history of xpack.infra.featureFlags.* feature flags. Most recently the "Alerts and rules" dropdown was put behind a feature flag for #169339 as Custom threshold rule was behind a feature flag. So it makes sense now to remove this feature flag since we have Custom threshold rule available without feature flag. But opening the rule flyout from infra or any other app would be more useful when we have prefill functionality implemented. @maryam-saeidi wdyt?

[maryam-saeidi]

But opening the rule flyout from infra or any other app would be more useful when we have prefill functionality implemented. @maryam-saeidi wdyt?

@benakansara I agree. Actually, the reason for this PR is to give the infra and logs team a chance to show and prefill this rule in their flyout when they want to, so this is a prerequisite for that effort. We can help when one of these apps uses the prefill functionality in case of an issue/missing functionality. I didn't want to put effort into implementing that logic as the app team would know better what to pass, and we could support them along the way.

[maryam-saeidi]

We should add the missing Aggs to the rule first, then provide the pre-fill feature. Otherwise, we will miss the goal of this PR and deliver an uncompleted feature/bug, as we don't offer or handle these Aggs yet.

@fkanout I disagree, we are providing the base functionality that can be used with the current set of aggs. When the new aggs are added, they will be included in the prefilling out-of-the-box, and if there is a bug, we can fix it then instead of blocking this PR. I don't see the current implementation as an incomplete feature or a bug.

[maryam-saeidi]

@maryam-saeidi, why can't we manually test the PR?

@fkanout Because the related logic to pass prefill information hasn't been implemented for the custom threshold rule yet.

I found that the Metric Explorer already had the option to create a Custom Threshold Rule based on defined parameters like the Metric one; we only need to enable the FF. However, I enabled the FF, and the prefilling is not working as expected, as the screen recording shows. e.g., the groupBy is always host.name What do you think the issue is?

This is because, for now, only the host.name is passed for prefilling in the custom threshold rule. You can check the related logic here. So your test shows the current logic is working as expected for the groupBy field.

[benakansara]

@maryam-saeidi, why can't we manually test the PR?

@fkanout Because the related logic to pass prefill information hasn't been implemented for the custom threshold rule yet.

I found that the Metric Explorer already had the option to create a Custom Threshold Rule based on defined parameters like the Metric one; we only need to enable the FF. However, I enabled the FF, and the prefilling is not working as expected, as the screen recording shows. e.g., the groupBy is always host.name What do you think the issue is?

This is because, for now, only the host.name is passed for prefilling in the custom threshold rule. You can check the related logic here. So your test shows the current logic is working as expected for the groupBy field.

I agree that host.name is shown because it is pre-filled by default at the moment. And in the test Faisal did, it would make more sense to add cluster.id in rule flyout and not host.name. The reason mentioned for adding host.name was to add host context in the alert, but this would also cause alerts per host which user may or may not have wanted.

I don't believe this is impacted by this PR though (as host.name was already there). To my understanding, apps team would decide how they want to prefill the rule flyout.

[fkanout]

We should add the missing Aggs to the rule first, then provide the pre-fill feature. Otherwise, we will miss the goal of this PR and deliver an uncompleted feature/bug, as we don't offer or handle these Aggs yet.

@fkanout I disagree; we are providing the base functionality that can be used with . When the new ggs are added, they will be included in the prefilling out-of-the-box, and if there is a bug, we can fix it then instead of blocking this PR. I don't see the current implementation as an incomplete feature or a bug.

@maryam-saeidi, it's about something other than how the team will implement it or if it's out of the box. I will illustrate the issue differently:

Imagine we merged the PR before adding the missing Aggs, and then the Infra team uses it to pass the prefill info, a.k.a, the user selection in the Metric Explorer, e.g., Agg type, filters, groupBy, etc...

If a user selects one of the missing Aggs (p99, p95, or Rate) as mentioned here #172783 (review), which is not supported by the Custom Threshold rule, it will be a bug.

We can't map/prefill the Aggs part from the Metric Explorer as the Aggs list of the Custom threshold rule creation form and the Metric Explorer are different.

How can we guarantee that the user or the Infra team will not use the missing Aggs till we implement them? And that would be more critical to handle with Serverless.

I propose either :

• Sync with the Infra team to remove the missing Aggs from their list until we add them. I think it's not possible, as they use the same Aggs list to create the Infra rule.

• Add the missing Aggs first, in this PR, or a separate one.

[maryam-saeidi]

@maryam-saeidi, it's about something other than how the team will implement it or if it's out of the box. I will illustrate the issue differently:

Imagine we merged the PR before adding the missing Aggs, and then the Infra team uses it to pass the prefill info, a.k.a, the user selection in the Metric Explorer, e.g., Agg type, filters, groupBy, etc...

If a user selects one of the missing Aggs (p99, p95, or Rate) as mentioned here #172783 (review), which is not supported by the Custom Threshold rule, it will be a bug.

We can't map/prefill the Aggs part from the Metric Explorer as the Aggs list of the Custom threshold rule creation form and the Metric Explorer are different.

How can we guarantee that the user or the Infra team will not use the missing Aggs till we implement them? And that would be more critical to handle with Serverless.

I propose either :

• Sync with the Infra team to remove the missing Aggs from their list until we add them. I think it's not possible, as they use the same Aggs list to create the Infra rule.
• Add the missing Aggs first, in this PR, or a separate one.

@fkanout In case the Infra team wants to use this feature, they should remove passing aggs that are not supported by the Custom threshold rule, and types should warn them about using an agg that is not supported. So I don't see an issue about this topic, and the Infra team can easily filter out unsupported aggs while preparing prefill data for the Custom threshold rule.

On another note, I don't think the Infra team wants to prefill this rule for the Metric Explorer page; rather, they wanted to have it for the Host view, so the issue you mentioned is less relevant there.

[fkanout]

As discussed, the priority is to unblock the Logs' use cases.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: b036186

Failed CI Steps

• x-pack/test_serverless/functional/test_suites/security/common_configs/config.group6.ts

Metrics [docs]

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	+907.0B

History

• 💔 Build #183490 failed 48aef85
• 💛 Build #182981 was flaky 3f11a6d
• 💚 Build #182406 succeeded 3b8ad67
• 💔 Build #182369 failed 7cab6d3

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

cc @maryam-saeidi