-
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
[Alerts][License] Define minimum license required for each alert type #84997
[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) |
@@ -45,6 +47,7 @@ export const ALERT_TYPES_CONFIG = { | |||
}), | |||
actionGroups: [THRESHOLD_MET_GROUP], | |||
defaultActionGroupId: 'threshold_met', | |||
minimumLicenseRequired: 'basic', |
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.
This needs a platinum license I think, as it is based on Machine Learning. @sqren?
@@ -67,6 +67,7 @@ export function registerTransactionDurationAnomalyAlertType({ | |||
], | |||
}, | |||
producer: 'apm', | |||
minimumLicenseRequired: 'basic', |
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.
Same here.
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 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 |
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
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.
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
.
@@ -3,7 +3,7 @@ | |||
"server": true, | |||
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I added this by mistake.
@@ -54,6 +56,7 @@ describe('register()', () => { | |||
}, | |||
], | |||
defaultActionGroupId: 'default', | |||
minimumLicenseRequired: 'basic' as LicenseType, |
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.
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.
@@ -9,6 +9,7 @@ import { ComponentType } from 'react'; | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think Pick will be better here
@@ -15,5 +15,5 @@ interface RegisterParams { | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
x-pack/plugins/stack_alerts/server/alert_types/geo_threshold/alert_type.ts
Show resolved
Hide resolved
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.
Geo alert changes lgtm!
- code review
- tested locally in chrome
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.
Changes LGTM! Looks like one .orig
file slipped and should be deleted.
@@ -0,0 +1,452 @@ | |||
/* |
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 this file was committed by accident?
const alertType = { | ||
id: 'test', | ||
name: 'My test alert', | ||
actionGroups: [{ id: 'default', name: 'Default' }, RecoveredActionGroup], | ||
defaultActionGroupId: 'default', | ||
minimumLicenseRequired: 'basic' as LicenseType, |
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.
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 Trace
Creates 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 Trace
and 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 <[email protected]>
Current PR contains the first part of the alert types licensing functionality:
Resolve #84954