[Alerts][License] Define minimum license required for each alert type#84997
Conversation
ec161c2 to
f426a70
Compare
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
|
Pinging @elastic/apm-ui (Team:apm) |
|
Pinging @elastic/uptime (Team:uptime) |
| }), | ||
| actionGroups: [THRESHOLD_MET_GROUP], | ||
| defaultActionGroupId: 'threshold_met', | ||
| minimumLicenseRequired: 'basic', |
There was a problem hiding this comment.
This needs a platinum license I think, as it is based on Machine Learning. @sqren?
| ], | ||
| }, | ||
| producer: 'apm', | ||
| minimumLicenseRequired: 'basic', |
chrisronline
left a comment
There was a problem hiding this comment.
LGTM for Stack Monitoring!
|
@YulNaumenko @dgieselaar @sqren I understand there is a question about alert type licensing. With the exception of a specific maps alert type, which had a very particular reasoning, all alert types are currently on Basic, based on the current alerting licensing strategy. ML-driven alerts need to be discussed indeed because ML is on Platinum, but this conversation needs to happen in the context of the broader strategy, not on a per individual alert type basis. In addition, if the question concerns an alert type which is already released on Basic, upgrading the required license in a subsequent release is something we generally do not do. |
Okay, then let's just leave the APM alerts as basic. I don't think it's super important for us that they are under platinum, other than it would align well with the ML licensing. |
|
Thanks for the response. It is a valid discussion that we need to have. There is a "generic" ML alert on the way and it makes sense to discuss the solution-specific ML-driven alerts as well, but the conversation needs to happen in context. For example, higher licenses may mean less adoption, and there is a number of other factors to consider. |
Makes sense 👍
True - although in this case, even if we make the alert available under basic, it will not be usable unless the user has Platinum (there won't be any ML data to alert on). |
|
Just to add to the conversation, Stack Monitoring also has the concept of certain alerts being only available in gold/platinum; however, we are currently creating these alerts "out of the box" (since this concept doesn't exist yet, we are creating these when the user first visits the Stack Monitoring UI) and I'm worried about applying rules for alert creation for a data point that can change over time. For example, if they are on the Basic license when they first visit the Stack Monitoring UI, then upgrade to Gold and don't revisit the Stack Monitoring UI, any gold-gated alerts will not exist. This might be something to consider when implementing #59813 cc @mikecote |
mikecote
left a comment
There was a problem hiding this comment.
Finished my review, looking good!
Before approving, I want to discuss the type safety with usage of as unknown) as AlertType since it is similar to using any.
| "version": "8.0.0", | ||
| "kibanaVersion": "kibana", | ||
| "requiredPlugins": ["alerts", "features", "triggersActionsUi", "kibanaReact", "savedObjects", "data"], | ||
| "requiredPlugins": ["alerts", "features", "triggersActionsUi", "kibanaReact", "savedObjects", "data", "licensing"], |
There was a problem hiding this comment.
I couldn't find any usage of the licensing plugin within stack_alerts. Was this added by mistake?
There was a problem hiding this comment.
Yes, I added this by mistake.
| }, | ||
| ], | ||
| defaultActionGroupId: 'default', | ||
| minimumLicenseRequired: 'basic' as LicenseType, |
There was a problem hiding this comment.
optional nit: Any place that requires casting to LicenseType can usually be fixed by making the alerType typed to AlertType. In this case, changing const alertType = { to const alertType: AlertType = { would do the trick.
| import { ActionGroup, AlertActionParam } from '../../alerts/common'; | ||
| import { ActionType } from '../../actions/common'; | ||
| import { TypeRegistry } from './application/type_registry'; | ||
| import { AlertType as AlertTypeApi } from '../../alerts/common'; |
There was a problem hiding this comment.
optional nit: for a clearer variable name
| import { AlertType as AlertTypeApi } from '../../alerts/common'; | |
| import { AlertType as CommonAlertType } from '../../alerts/common'; |
| id: string; | ||
| name: string; | ||
| actionGroups: ActionGroup[]; | ||
| export interface AlertType extends Omit<AlertTypeApi, 'actionVariables'> { |
There was a problem hiding this comment.
question: since this AlertType doesn't related directly with the common alert type, should we pick or omit attributes? Just thinking in the scenario a new property is added to the common alert type.
There was a problem hiding this comment.
Yeah, I think Pick will be better here
| export function register(params: RegisterParams) { | ||
| const { logger, alerts } = params; | ||
| alerts.registerType(getAlertType(logger)); | ||
| alerts.registerType((getAlertType(logger) as unknown) as AlertType); |
There was a problem hiding this comment.
Doing as unknown) as AlertType throughout would remove the TS type safety we have for alert type. I wonder what caused this to break?
@chrisronline good point. I think for pre-configured alerts, we should bypass the license check for those and handle it differently (ex: execution time). There's ongoing discussion about how we should handle expired licenses and the existing alerts. It seemed disabling the alert was the best option, but maybe not? thinking about your use case where you'd just want the alerts to "resume". This may be the same expectation with other alerts. cc @YulNaumenko |
x-pack/plugins/stack_alerts/server/alert_types/geo_threshold/alert_type.ts
Show resolved
Hide resolved
kindsun
left a comment
There was a problem hiding this comment.
Geo alert changes lgtm!
- code review
- tested locally in chrome
mikecote
left a comment
There was a problem hiding this comment.
Changes LGTM! Looks like one .orig file slipped and should be deleted.
| @@ -0,0 +1,452 @@ | |||
| /* | |||
There was a problem hiding this comment.
I think this file was committed by accident?
| name: 'My test alert', | ||
| actionGroups: [{ id: 'default', name: 'Default' }, RecoveredActionGroup], | ||
| defaultActionGroupId: 'default', | ||
| minimumLicenseRequired: 'basic' as LicenseType, |
There was a problem hiding this comment.
nit: casting to LicenseType instead of defining the alert type as : AlertType.
💛 Build succeeded, but was flaky
Test Failures"before each" hook for "Deletes one rule".Custom detection rules deletion and edition Deletion "before each" hook for "Deletes one rule"Stack TraceCreates and activates a new ml rule.Detection rules, machine learning Creates and activates a new ml ruleStack Trace"after all" hook for "Creates and activates a new ml rule".Detection rules, machine learning "after all" hook for "Creates and activates a new ml rule"Stack Traceand 19 more failures, only showing the first 3. Metrics [docs]Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
* [Alerts][License] Define minimum license required for each alert type (#84997) * Define minimum license required for each alert type * fixed typechecks * fixed tests * fixed tests * fixed due to comments * fixed due to comments * removed file * removed casting to LicenseType * [Alerts][License] Add license checks to alerts HTTP APIs and execution (#85223) * [Alerts][License] Add license checks to alerts HTTP APIs and execution * fixed typechecks * resolved conflicts * resolved conflicts * added router tests * fixed typechecks * added license check support for alert task running * fixed typechecks * added integration tests * fixed due to comments * fixed due to comments * fixed tests * fixed typechecks * [Alerting UI][License] Disable alert types in UI when the license doesn't support it. (#85496) * [Alerting UI][License] Disable alert types in UI when the license doesn't support it. * fixed typechecks * added licensing for alert list and details page * fixed multy select menu * fixed due to comments * fixed due to comments * fixed due to comments * fixed typechecks * fixed license error message * fixed license error message * fixed typechecks * fixed license error message Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Current PR contains the first part of the alert types licensing functionality:
Resolve #84954