-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Transform] Transforms health alerting rule type #112277
[Transform] Transforms health alerting rule type #112277
Conversation
...ugins/transform/public/alerting/transform_health_rule_type/register_transform_health_rule.ts
Outdated
Show resolved
Hide resolved
...ugins/transform/public/alerting/transform_health_rule_type/transform_health_rule_trigger.tsx
Show resolved
Hide resolved
...ugins/transform/public/alerting/transform_health_rule_type/transform_health_rule_trigger.tsx
Outdated
Show resolved
Hide resolved
...ugins/transform/public/alerting/transform_health_rule_type/transform_health_rule_trigger.tsx
Outdated
Show resolved
Hide resolved
.../plugins/transform/public/alerting/transform_health_rule_type/transform_selector_control.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/transform/server/lib/alerting/register_transform_alerting_rules.ts
Outdated
Show resolved
Hide resolved
...ugins/transform/public/alerting/transform_health_rule_type/register_transform_health_rule.ts
Outdated
Show resolved
Hide resolved
...ugins/transform/public/alerting/transform_health_rule_type/register_transform_health_rule.ts
Show resolved
Hide resolved
...plugins/transform/server/lib/alerting/transform_health_rule_type/transform_health_service.ts
Show resolved
Hide resolved
…health-alerting-rule
…health-alerting-rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems generally good to me. It might be good for @mgiota or @miltonhultgren to cross check given they've been doing more in the infra UI alerting space.
@@ -267,6 +271,8 @@ export class MonitoringPlugin | |||
} | |||
|
|||
registerPluginInUI(plugins: PluginsSetup) { | |||
const alertingRules = [...RULES, TRANSFORM_RULE_TYPE.TRANSFORM_HEALTH]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're not adding TRANSFORM_RULE_TYPE.TRANSFORM_HEALTH
to the RULES
constant directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it at first but RULES
are used by the factory here. So it requires extending the BaseRule
class which I assume is redundant for the Transform alerting rule.
@elasticmachine merge upstream |
@@ -128,6 +130,8 @@ export class MonitoringPlugin | |||
for (const alert of alerts) { | |||
plugins.alerting?.registerType(alert.getRuleType()); | |||
} | |||
plugins.alerting?.registerType(getTransformHealthRuleType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any specific reason to do not use a factory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed it and realized it's not a right fit for the Stack Monitoring rules. We're going to include it in the Stack Rules instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should update the rules telemetry list x-pack/plugins/alerting/server/usage/alerts_usage_collector.ts
as well and create a proper update in the telemetry repository, for example: https://github.com/elastic/telemetry/pull/530
thanks, @YulNaumenko, I've updated mappings in de0a7d9. Also realized there were some issues with the ML related mappings 😓 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: cc @darnautov |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Dima Arnautov <[email protected]>
Summary
Part of #111945
Checklist