[ResponseOps] Move custom threshold rule params to the package

[guskovaue]

Fixes: #195191

Move log threshold rule type params to the new package.

P.S.: I've moved function validateKQLStringFilter and test for it in my previous PR: #205507

[guskovaue]

/ci

[elasticmachine]

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

[elasticmachine]

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

[adcoelho]

It is really strange for me to have this logic here. If we never use the element COUNT of Aggregators why not just do:

enum Aggregators { // or call it AllowedAggregators instead
  // COUNT = 'count',
  AVERAGE = 'avg',
  SUM = 'sum',
  MIN = 'min',
  MAX = 'max',
  CARDINALITY = 'cardinality',
  RATE = 'rate',
  P95 = 'p95',
  P99 = 'p99',
  LAST_VALUE = 'last_value',
}

Right now we are defining an enum that is not used anywhere else and then in the only place we need it we always remove an element. Feels wrong.

[guskovaue]

Agree. Made changes according comments here: 0652609

[maryam-saeidi]

I see this part has changed:

  const allowedAggregators = Object.values(Aggregators);
  allowedAggregators.splice(Object.values(Aggregators).indexOf(Aggregators.COUNT), 1);

Do we still have the Aggregators types somewhere? I am trying to remember why we had two list and excluded one from the allowed list.

[maryam-saeidi]

So, it seems we have this type still in common (Aggregator), and we use it in a lot of places.

I like the idea of having this aggregator shared everywhere (i.e., moving the common aggregator type here), but then this will increase the scope of this PR. (There is no issue in using this package both in FE and server-side, right?)

@cnasikas thoughts?

[cnasikas]

We decided not to change the way solutions use the types and the schemas due to a lack of domain knowledge. Also, some of the schemas are made with io-ts and we need the schemas to be in kbn-config. When the task is finished, we plan to inform you that you can use the package for types/schemas. It is up to the solutions to make the changes and use the types/schemas from the package if they want.

[maryam-saeidi]

Here is an idea that minimizes the number of changes, let me know what you think:

1. Moving Aggregators enum to this package.
2. Use it as before here:

   const allowedAggregators = Object.values(Aggregators);
   allowedAggregators.splice(Object.values(Aggregators).indexOf(Aggregators.COUNT), 1);
   

3. Instead of aggType: schema.literal('count'),, use

   aggType: schema.literal(Aggregators.COUNT)
   

   to only rely on the enum.
4. Import Aggregators from here in the observability > common, so we don't need to change anything else.

Would that work?

[adcoelho]

(There is no issue using this package on both the FE and server sides, right?)

I thought there was an issue using this in the FE since rule_params uses the config-schema which relies on some library(hapi?) which is not usable on the front end. Maybe @cnasikas can confirm?

If so we could move this to a different rule_params_constants package and then import it like you say.

As for the allowedAggregators values + splice logic, I don't have a better alternative :(

[maryam-saeidi]

I checked again, and I don't see a usage other than related to the custom threshold rule. The AI assistant usage is also related to creating a custom threshold rule.

Is there another package related to rule types from which we can expose the Aggregators enum and use it in both places?

[cnasikas]

If we have a centralized package where we expose the Aggregators enum and the rules params package uses it, we can accidentally introduce breaking changes if someone modifies the enum because the Rules APIs use it. Collocating it in the rule params avoids this issue. Also, who will own the centralized package Aggregators enum?

[maryam-saeidi]

I think the confusion comes from considering this enum separate from being a rule param. For us, there is no difference between this enum and the Aggregators that we use in the rule, and defining it as an enum helps us to keep the value in sync in all the usages.

Who will own the centralized package Aggregators enum?

This would be the same as this current package for exposing rule parameters, we just separate it to use it on FE as well due to the bundle limitation.

It would be good to have this improvement if we have a package that we can expose this enum, but I don't see it as a blocker, so I'll approve this PR.

[cnasikas]

Thanks Maryam! I get your points. Let's talk about it offline.

[adcoelho]

I created an issue for a rule_params_constants package. #209547

[js-jankisalvi]

LGTM 👍

[cnasikas]

@elasticmachine merge upstream

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 6e50ea8

Metrics [docs]

✅ unchanged

History

• 💚 Build #272352 succeeded e0deba1
• 💚 Build #271653 succeeded fb6afa4
• 💚 Build #271519 succeeded 6053db6

cc @guskovaue

[kibanamachine]

Starting backport for target branches: 9.0

https://github.com/elastic/kibana/actions/runs/13143813102

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	9.0

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation