Skip to content
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

New alerting terminology #90375

Open
12 of 17 tasks
mikecote opened this issue Feb 5, 2021 · 20 comments
Open
12 of 17 tasks

New alerting terminology #90375

mikecote opened this issue Feb 5, 2021 · 20 comments
Labels
estimate:needs-research Estimated as too large and requires research to break down into workable issues Feature:Alerting Meta Project:UnifiedAlertingArchitectureAndExperience Alerting team project for developing a unified alerting experience. Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@mikecote
Copy link
Contributor

mikecote commented Feb 5, 2021

Current Problem Statement

The 8.0 release is the opportunity to break changes to support the new alerting terminology. @gmmorris started the research, but we need to find a new person to verify if anything is remaining before it is too late to change. For those cases, we should create issues and prioritize them.


There is overlap between alerting concepts in Observability and Security, but differences in terminology obscure the similarities, and it becomes a barrier. This issue summarizes the new proposed terminology and tracks progress at applying the changes to Kibana Alerting.

Proposed Elastic Term Definition Term used today in Elastic Security Term used today in Kibana Alerting
Rule A set of conditions that are checked periodically Detection Rule Alert
Alert Data/events generated from matching a set of conditions Detection Alert Alert Instance
Action An invocation of a system (internal or 3rd party ), such as calling an API Action Action
Connector A configuration to connect to a system (internal or 3rd party) Connector Connector

Implementation steps

@mikecote mikecote added Meta Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Feb 5, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@mikecote
Copy link
Contributor Author

mikecote commented Feb 5, 2021

One question I can think of as we move our HTTP APIs to use the new terminology is: what do we do with APIs that overlap? ex: _find, _aggregate, _health. I think we'll want to rename alertTypeId to ruleTypeId. Do we change those APIs to support both options, if possible? Do we need to start versioning our APIs? What about filter/KQL?

@pmuellr
Copy link
Member

pmuellr commented Feb 9, 2021

I did run across - I think monitoring - versioning their APIs with v1, v2 path prefixes. Not sure if we have a Kibana-side story for this kinda thing.

But I'm not sure I understand the "overlap" part of this. I can believe it though :-). Example?

@mikecote mikecote added this to Meta Issues in Kibana Alerting Feb 9, 2021
@mikecote
Copy link
Contributor Author

mikecote commented Feb 9, 2021

@pmuellr by overlap, I meant /api/alerts/_find. Do we have to make the same API support new and old parameter names (alertTypeId vs ruleTypeId)? Or is the plugin name now questionable and should be /api/rules/_find or /api/alerting/rules/_find 🤔 v1, v2 could work as well. I will leave it to @gmmorris to think about it.

@mikecote
Copy link
Contributor Author

@gmmorris, the new alerting app (step 4 in issue) is no longer planned for 7.x. This will mean the label changes will have to be done in the existing app at some point, and we should figure out what release is best to do so. cc @arisonl

@gmmorris
Copy link
Contributor

gmmorris commented Feb 11, 2021

Looking into the API changes first, here are the broad strokes of what would likely need changing:

Actions Plugin Api Paths

Important note: When introducing these new APIs we need to rename all fields in bodies need to be renamed from Camel Case to Snake Case as per our Style Guide. In the code they need to be renamed back to Camel Case.

  • POST /api/actions/action => /api/actions/connector
  • DELETE /api/actions/action/{id} => /api/actions/connector/{id}
  • GET /api/actions => /api/actions/connectors
  • GET /api/actions/action/{id} => /api/actions/connector/{id}
  • GET /api/actions/list_action_types => /api/actions/list_connector_types
  • PUT /api/actions/action/{id} => /api/actions/connector/{id}
  • POST /api/actions/action/{id}/_execute => /api/actions/connector/{id}/_execute
Alerts Plugin Api Paths

Important note: When introducing these new APIs we need to rename all fields in bodies need to be renamed from Camel Case to Snake Case as per our Style Guide. In the code they need to be renamed back to Camel Case.

This change will require changing the Alerts plugin back to it's original name of Alerting, as it becomes confusing with Alerts being renames Rules.

  • POST /api/alerts/alert => /api/alerting/rule
  • GET /api/alerts/_find => /api/alerting/rules/_find
  • GET /api/alerts/alert/{id} => /api/alerting/rule/{id}
  • GET /api/alerts/alert/{id}/state => /api/alerting/rule/{id}/state
  • GET /api/alerts/alert/{id}/_instance_summary => /api/alerting/rule/{id}/_alert_summary
  • GET /api/alerts/list_alert_types => /api/alerting/list_rule_types
  • DELETE /api/alerts/alert/{id} => E /api/alerting/rule/{id}
  • PUT /api/alerts/alert/{id} => /api/alerting/rule/{id}
  • POST /api/alerts/alert/{id}/_enable => /api/alerting/rule/{id}/_enable
  • POST /api/alerts/alert/{id}/_disable => /api/alerting/rule/{id}/_disable
  • POST /api/alerts/alert/{id}/_mute_all => /api/alerting/rule/{id}/_mute_all
  • POST /api/alerts/alert/{alert_id}/alert_instance/{alert_instance_id}/_mute => /api/alerting/rule/{rule_id}/alert/{alert_id}/_mute
  • POST /api/alerts/alert/{id}/_unmute_all => /api/alerting/rule/{rule_id}/_unmute_all
  • POST /api/alerts/alert/{alertId}/alert_instance/{alert_instance_id}/_unmute => /api/alerting/rule/{rule_id}/alert/{alert_id}/_unmute
  • POST /api/alerts/alert/{id}/_update_api_key => /api/alerting/rule/{id}/_update_api_key
End to End testing

We have e2e tests over all our Api endpoints.
My suggestion is that we duplicate the basic CRUD legacy tests and keep them along side a copy that gets updates for the new type.
That way CRUD will be tested against both sets of endpoints (Alerts and Rules, Actions and Connectors) but we'll keep the more complicated suites that test Business Logic and Lifecycle as only one copy, which we will migrate to the new Apis.

As long as the old Apis do nothing other than act as a shell that converts Old shape to New shape and back to Old, then this should be sufficient.

Actions Plugin Type changes

There are probably more field to rename and lots of types to rename, I'm just pointing to some obvious ones to give an idea of the complexity of the change.

// ActionResult => ConnectorResult
interface ActionResult {
 // ....
  actionTypeId: string; =>   connectorTypeId: string;
}

// ActionTypeExecutorResult => ActionExecutionResult
interface ActionExecutionResult<Data> {
 // ....
  actionId: string; => connectorId: string;
}

// ActionTypeExecutorOptions => ConnectorExecutorOptions
interface ActionTypeExecutorOptions<Config, Secrets, Params> {
 // ....
  actionId: string; => connectorId: string;
}
Alerts Plugin Type changes

There are probably more field to rename and lots of types to rename, I'm just pointing to some obvious ones to give an idea of the complexity of the change.

// Alert => Rule
interface Rule {
 // ....
  alertTypeId: string; =>   ruleTypeId: string;
  mutedAlertIds: string[];
}

// AlertAction => RuleAction
interface RuleAction {
 // ....
  id: string; => connectorId: string;
  actionTypeId: string; => connectorType: string;
}

// AlertType => RuleType
interface RuleType {
 // .... field all stay the same
}

// RawAlertAction => RawRuleAction
interface RawAlertAction extends SavedObjectAttributes {
 // ....
  actionTypeId: string; => connectorType: string;
}
Api migration notes

We need to keep the legacy Api paths until 8.0, so this need to be an incremental deprecation.
I don't think we need to version the Apis as we're adding new endpoints and if we can avoid keeping two internal implementations, then we can introduce these side by side.

The complexity is in the fields (such as actionTypeId becoming connectorType, or actionId becoming connectorId) and how we then support both the old and the new Api endpoints.

My suggestion would be that we refactor the old endpoints to become a thin "conversion" where we rename the old names to the new names, which then get passed into the underlying implementation.
The difficulty is then on the way back where the result might contain the new name instead of the old one - which we would then need to convert back.
To achieve this we'll have to duplicate all type like ActionResult and create a LegacyActionResult which is returned by the legacy Api endpoint. On these legacy types we'll rename the fields to ensure they align with the original types.
As the APIs don't expose the names alert/action (but rather return the attributes) we don't need to worry about this name change at api level.

When the conversion is down to a concrete TS type field, this is quite straight forward - the difficulty is going to be with Saved Objects and Apis that interact with them using queries such as _find.
We might be able to naively match KQL queries using a simple find & replace so that queries like this get updated on the fly:
alert.attributes.executionStatus.status:error... => rule.attributes.executionStatus.status:error....
We'll have to investigate the variety of KQL queries that could be passed in to ensure we can identify all usage of the legacy names, but as there appears to be a solid pattern of {entity name}.attributes..., I think we can make this work.

Much like the Apis we don't actually return the results with the name of the entity (alert/action) so we only need to manipulate the fields, not the SO entity itself.

Saved Object migration notes

At the moment we have three Saved Object types: alert, action and action_task_params.
I think we should rename them now, as it will become very confusing having an alert SO type that is used for rule and action which is used for connector.
So I suggest renaming them to rule, connector and action.

We don't currently have a method for achieving this but there's a chance Core can provide this via the migrations framework.
I've opened an issue to track this: #91143

...waiting to see what Core can offer before we delve deeper into this

Action / Alerts Clients migration

APIs on the clients themselves don't seem to refer to the entity in any way, so the only problems we'll have are at the level of the fields (such as actionTypeId becoming connectorType as already mentioned).
We'll need to support the same conversions for the usage of the find apis as mentioned above, but as the http Apis are using the clients, this doesn't mean duplication, only that the implementation will need to be at client level.

@pmuellr
Copy link
Member

pmuellr commented Feb 16, 2021

I assume we are changing plugin names as well?

  • actions -> connectors
  • alerts -> rules
  • stack_alerts -> stack_rules
  • triggers_actions_ui -> triggers_connectors_ui

Not sure I like rules as a top-level name, stack_rules is almost even more ambiguous! :-) alerting_rules? (alerting rules, dude!)

Or can we not easily change the plugin names for some reason (feature controls, RBAC, etc)?

I think there's also a convention that plugin xyz has http endpoints api/xyz/..., which we've "violated" here (eg, /api/alerting/rule)

@mikecote
Copy link
Contributor Author

GET /api/actions => /api/connectors

@YulNaumenko was mentioning it's a recommendation to keep the plugin id within the URL. This would be /api/actions/connectors?

GET /api/actions/action/{id} => /api/actions/connectors/{id}
GET /api/alerts/alert/{id}/_instance_summary => /api/alerting/rules/{id}/_alert_summary
PUT /api/alerts/alert/{id} => /api/alerting/rules/{id}

A couple of endpoints that should use singular rule or connector?

POST /api/actions/action/{id}/_execute => /api/actions/action/connector/{id}/_execute - this is a first step towards differentiating between actions and connectors. An "Action" is an invocation of a "Connector" with provided parameters, then it makes sense that the execute api live under action rather than connector, but it requires the id of a connector to invoke. I'm not sure about the exact syntax... perhaps /api/actions/connector/{id}/action/_execute makes more sense. 🤔

This one is interesting, I'd recommend thinking about how it would look like when we support multiple actions per connector and work backwards from there.

/api/actions/connector/{id}/action/_execute can make sense, if we add support for multiple actions, it would turn into something like /api/actions/connector/{connectorId}/action/{actionId}/_execute which can seem like a very long route.

We could also make the action part of the URL parameter. /api/actions/connector/{connectorId}/{actionId}/_execute. This would automatically be future proof and we could use something like default as the actionId value for now.

DELETE /api/alerts/alert/{id} => E /api/alerts/rule/{id}

Should this be /api/alerting/rule/{id}? Using alerting plugin id.

actionTypeId: string; => connectorType: string;

Should these changes be to connectorTypeId? I'm usually a fan of appending Id to indicate it's not the full object but an id reference.


I also notice inconsistencies between some API endpoints. Some are prefixed with _ (ex: /_execute) and some end with a verb (ex: /_find). I wonder if we should take the opportunity to review these as well while we're changing them.

@gmmorris
Copy link
Contributor

I assume we are changing plugin names as well?

  • actions -> connectors

I'm not sure we need to change that one specifically... 🤔
Actions are still there, still a big part of that plugin and not going anywhere :)
It's also worth noting that as we can't migrate RBAC, we'll be keeping the Actions & Connectors feature as is, so actions is still a prominent name in the system.

I'd opt for keeping it as is.

@gmmorris
Copy link
Contributor

This one is interesting, I'd recommend thinking about how it would look like when we support multiple actions per connector and work backwards from there.

Fair point... yeah, we could add that now... but I'm not sure what id we'd use there now, as we don't have an actionId at the moment.
Should it use the same id as the connector until we add that support?
Or do we not include it now and address this in the future by having a default actionId on a connector? 🤔

Should this be /api/alerting/rule/{id}? Using alerting plugin id.

Yup, copy-pasta.
Fixed.

Should these changes be to connectorTypeId? I'm usually a fan of appending Id to indicate it's not the full object but an id reference.

It's not an Id though, is it? It's the connector type (like .index).
To me the Ids are the generated UUIDs... no?

I also notice inconsistencies between some API endpoints. Some are prefixed with _ (ex: /_execute) and some end with a verb (ex: /_find). I wonder if we should take the opportunity to review these as well while we're changing them.

I'm not following... aren't those both verbs preceded by _? 😆

@mikecote
Copy link
Contributor Author

Should it use the same id as the connector until we add that support?
Or do we not include it now and address this in the future by having a default actionId on a connector? 🤔

Good question :-) I'd have to think about it or see what others think. We can always defer distinguishing action from connector to when connectors support multiple actions. For now we could have something like /api/actions/connector/{connectorId}/_execute and later add a new API with {actionId} in it in the future.

It's not an Id though, is it? It's the connector type (like .index).
To me the Ids are the generated UUIDs... no?

Connectors use generated UUIDs, but connector types don't, though they still use unique identifiers (see: https://github.com/elastic/kibana/blob/master/x-pack/plugins/actions/server/types.ts#L108).

I'm not following... aren't those both verbs preceded by _? 😆

Bad examples, hopefully the others I put below explain the inconsistencies better:

  • GET /api/alerting/rules/{id}/_alert_summary (prefix _)
  • GET /api/alerting/list_rule_types (no prefix _)
  • GET /api/alerting/rules/_find (verb at the end)
  • GET /api/connectors (no verb at the end)

@gmmorris
Copy link
Contributor

Connectors use generated UUIDs, s, but connector types don't, though they still use unique identifiers

Ah I see what you mean... it's a funny one 😆
I jest felt it was confusing that that same ID is used in all instances of that connector... so I don't think of it as an identifier so much, as a classification... 🤷
But no biggie... I definitely don't feel strongly about this...

Bad examples, hopefully the others I put below explain the inconsistencies better:

Gotcha, yeah, we should fix these here... I'll give it a pass

@YulNaumenko
Copy link
Contributor

@YulNaumenko was mentioning it's a recommendation to keep the plugin id within the URL. This would be /api/actions/connectors?

Yes, according to the Kibana guidelines

@YulNaumenko YulNaumenko removed this from Meta Issues in Kibana Alerting Mar 10, 2021
@gmmorris gmmorris added the loe:needs-research This issue requires some research before it can be worked on or estimated label Jul 6, 2021
@mikecote mikecote added this to 7.15 in Kibana Alerting Jul 12, 2021
@mikecote mikecote moved this from 7.15 to To-Do (Ordered by priority) in Kibana Alerting Jul 26, 2021
@mikecote
Copy link
Contributor Author

mikecote commented Aug 5, 2021

I'm going to move this back to triage as I don't think we have the capacity to further investigate this in 7.x.

@mikecote mikecote removed this from To-Do (Ordered by priority) in Kibana Alerting Aug 5, 2021
@mikecote mikecote added this to Backlog in Kibana Alerting Aug 11, 2021
@gmmorris gmmorris added the estimate:needs-research Estimated as too large and requires research to break down into workable issues label Aug 18, 2021
@gmmorris gmmorris removed the loe:needs-research This issue requires some research before it can be worked on or estimated label Sep 2, 2021
@XavierM XavierM removed this from Backlog in Kibana Alerting Jan 6, 2022
@kobelb kobelb added the needs-team Issues missing a team label label Jan 31, 2022
@botelastic botelastic bot removed the needs-team Issues missing a team label label Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
estimate:needs-research Estimated as too large and requires research to break down into workable issues Feature:Alerting Meta Project:UnifiedAlertingArchitectureAndExperience Alerting team project for developing a unified alerting experience. Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
No open projects
Development

No branches or pull requests

6 participants