Denormalize actionTypeId into alert actions for easier filtering #51628
Conversation
mikecote
added
Feature:Alerting
v8.0.0
release_note:skip
|
Pinging @elastic/kibana-stack-services (Team:Stack Services) |
💚 Build Succeeded |
…rmalize-actionTypeId
…rmalize-actionTypeId
💚 Build Succeeded |
|
@elasticmachine merge upstream |
| actions: Array<{ | ||
| group: string; | ||
| id: string; | ||
| actionTypeId: string; |
There was a problem hiding this comment.
As @pmuellr mentioned, APIs (create / update) shouldn't accept actionTypeId and we should add it ourselves while also validating the action.
💔 Build Failed |
…rmalize-actionTypeId
| .expect(200); | ||
| objectRemover.add(space.id, createdAlert.id, 'alert'); | ||
|
|
||
| const response = await supertestWithoutAuth | ||
| .get( | ||
| `${getUrlPrefix( | ||
| space.id | ||
| )}/api/alert/_find?filter=alert.attributes.alertTypeId:test.noop` | ||
| )}/api/alert/_find?filter=alert.attributes.actions:{ actionTypeId: test.noop }` |
There was a problem hiding this comment.
Do we have previous examples of JSON being used on query string like this?
This seems non idiomatic, so just wondering what the reason is. 🤷♂️
There was a problem hiding this comment.
No examples I can think of, I added this here as we need a test to make sure we can filter on nested ES objects and this will become more common for the alerts list.
There was a problem hiding this comment.
This isn't JSON though, is it? It's KQL AFAIK, which happens to look like JSON for nested queries ...
There was a problem hiding this comment.
You're right, it's KQL and not JSON, looks very similar for nested queries.
…te/kibana into alerting/denormalize-actionTypeId
|
@elasticmachine merge upstream |
💚 Build Succeeded |
|
@elasticmachine merge upstream |
There was a problem hiding this comment.
LGTM
I took a quick check and didn't see any tests for an alert with multiple actions, that would go through the new denormalizeActions() call. Suggest we add one, maybe in a subsequent PR (create an issue if so), ... or maybe there is one and I missed it. :-)
| ): Promise<{ actions: RawAlert['actions']; references: SavedObjectReference[] }> { | ||
| // Fetch action objects in bulk | ||
| const bulkGetOpts = [ | ||
| ...new Set(alertActions.map(alertAction => ({ id: alertAction.id, type: 'action' }))), |
There was a problem hiding this comment.
TIL you could use an iterable with ... in an array - makes total sense, but I guess I've always thought it had to be an array (in this context).
| .expect(200); | ||
| objectRemover.add(space.id, createdAlert.id, 'alert'); | ||
|
|
||
| const response = await supertestWithoutAuth | ||
| .get( | ||
| `${getUrlPrefix( | ||
| space.id | ||
| )}/api/alert/_find?filter=alert.attributes.alertTypeId:test.noop` | ||
| )}/api/alert/_find?filter=alert.attributes.actions:{ actionTypeId: test.noop }` |
There was a problem hiding this comment.
This isn't JSON though, is it? It's KQL AFAIK, which happens to look like JSON for nested queries ...
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…stic#51628) * Denormalize actionTypeId for easier filtering of alerts * Add tests * No longer pass actionTypeId for each alert action in APIs * Add tests to ensure denormalizeActions works on multiple actions * Fix ESLint errors
) (#52588) * Denormalize actionTypeId for easier filtering of alerts * Add tests * No longer pass actionTypeId for each alert action in APIs * Add tests to ensure denormalizeActions works on multiple actions * Fix ESLint errors
…stic#51628) * Denormalize actionTypeId for easier filtering of alerts * Add tests * No longer pass actionTypeId for each alert action in APIs * Add tests to ensure denormalizeActions works on multiple actions * Fix ESLint errors
Alerts UI will need a filter on action types (ex: show me alerts that send emails). In order to do this, it's better to denormalize the
actionTypeIdfield within alert actions than it would be to load all the actions that use a certain action type and then filter by those ids.In this PR, I'm copying the
alertTypeIdvalue to eachalert.actions[x]object.Done:
Before this merges, we need to make sure saved objects KQL will work with this (#51637). So far encountering an error likeThis key 'actionTypeId' need to be wrapped by a saved object type like alert:.