[Logs UI] Fix match phrase and not match phrase alerting comparators

[Kerry350]

Summary

This fixes #71828.

The MATCH_PHRASE and NOT_MATCH_PHRASE comparators weren't being accounted for in the registration validation schema. This validation schema is used by the alerting framework before handing parameters off to the executor.

It's important to note that these comparators were being handled by the runtime validation within the executor itself, here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.ts#L48 and here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/infra/common/alerting/logs/types.ts#L25. Of course, this was no use failing at the level beforehand.

Similarly, there was a test for these: https://github.com/elastic/kibana/blob/master/x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.test.ts#L280, but that wouldn't have caught this happening before hitting the executor.

Based on this information, I'd like to suggest several things (for 7.10):

• The alerting validation is optional: You may also have the parameters validated before they are passed to the executor function or created as an alert saved object. In order to do this, provide a @kbn/config-schema schema that we will use to validate the params attribute.. Given that we have strict validation within our executor, we could remove the additional registration validation, this would stop the two becoming out of sync. That validation option has to use a config-schema currently.

• We should implement [Logs / Metrics UI] [Alerting] Add functional tests #69162, this would introduce functional tests that could catch problems with interacting with the alerting framework itself.

(There is a 7.8.2 scheduled so I've targetted that as well).

Testing

• It should be possible to create alerts using matches phrase and does not match phrase for the criteria comparators.

[elasticmachine]

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

[weltenwort]

👀 Reviewing on behalf of @elastic/logs-metrics-ui...

[weltenwort]

I agree that having two sources of truth for the validation is not ideal. But validating during the alert submission has the advantage that it fails early and loudly in the UI.

That validation option has to use a config-schema currently.

Does it? The type suggests it could be anything with a validate function:

kibana/x-pack/plugins/alerts/server/types.ts

Line 76 in 8bcecc0

params?: { validate: (object: unknown) => AlertExecutorOptions['params'] };

Shouldn't we be able to create an adapter similar to createValidationFunction for routes, that allows us to use the same io-ts runtime type for both the registration and within the executor (in a later PR 😉)?

[weltenwort]

I was able to create alerts using the "matches phrase" and "not matches phrase" operators. 👍

[weltenwort]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 847cee8

Build metrics

✅ unchanged

History

• 💚 Build #61885 succeeded 11e3cba

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

[Kerry350]

But validating during the alert submission has the advantage that it fails early and loudly in the UI.

Yeah, true.

Does it? The type suggests it could be anything with a validate function

Good point 🤔 I was going off of the docs, where a schema config is the only thing mentioned. But looking at the types it looks like we can use our own function. I'll file a ticket (which can also include updating the Alerting README).

[EamonnTP]

Hi @Kerry350 Should this be included in the 7.9.0 release notes? I don't see it listed here: https://github.com/elastic/kibana/blob/0afaba21b39709739086e2aaa3a88a8a1bea855b/docs/CHANGELOG.asciidoc