[actions] add rule saved object reference to action execution event log doc #101526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pmuellr
added
v8.0.0
release_note:skip
|
Per comment in the (current) code, the namespace and rule type id are not currently added to the event log saved object reference, since we don't have those handy - https://github.com/elastic/kibana/pull/101526/files#diff-9a1c0e513210895d1de20260558acd5fadacf4b8331879fba6f37a3e96375fabR162-R163 We could get these, presumably some code before this point has looked up the alert rule, so we could make a copy and arrange to pass it in here, but that may get complicated. I'm wondering if we really need these though, if it does get complicated to populate the fields:
|
|
Interesting to note that the alerting jest tests all passed after making the change to add these additional saved object reference array elements. Something should have failed! Hoping we see a function test failure! |
|
Huh, function tests pass as well. We'll have to do something about that! |
|
I'm kind of stuck on this, in if (isSavedObjectExecutionSource(source)) {
event.kibana?.saved_objects?.push({
rel: SAVED_OBJECT_REL_PRIMARY,
type: source.source.type,
id: source.source.id,
// type_id - we don't have this field, but would be nice to have
// namespace - we don't have this field, but would be nice to have
});The problem is that by the time we get here, we don't have the We could instead pass the alert info in new action task params fields, but then we lose out on the aspects of having them in the saved object references so they stay someone sync'd up. Maybe that's ok? They could be "out of date", but I wouldn't think they'd be WAAAY out of date - and we're only using them as output data. Which leaves us with fetching the alert AGAIN. But we're already fetching the alert once, here: It seems so painful to add another alert fetch. I'm going to do a little more playing around, see if I can capture the fetch from the One interesting option I thought of was to create a "caching" layer over our There's no way we can add a caching layer for 7.14, so even if we thought that was a good idea, we'd need an alternative for 7.14 ... |
pmuellr
force-pushed
the
actions/add-rule-to-execute-event
branch
from
bb51355 to
c79de5e
Compare
pmuellr
commented
Jun 15, 2021
| @@ -214,10 +214,10 @@ describe('Task Runner', () => { | |||
| expect(call.rule.consumer).toBe('bar'); | |||
| expect(call.rule.enabled).toBe(true); | |||
| expect(call.rule.schedule).toMatchInlineSnapshot(` | |||
| Object { | |||
There was a problem hiding this comment.
for this file, you'll want to hide whitespace changes, will reduce the amount of code to review :-)
|
from #101526 (comment)
This is the approach I'm going down now: I've added a field to the action task params called Kinda sad I couldn't figure out a good way to get the alert data from the fetch that we're already doing on the alert, but this does give us a little more flexibility in the long-run if there do end up being multiple saved objects referencing an action execution (think, visualization in a dashboard, or alert in a case), if we need it. |
pmuellr
force-pushed
the
actions/add-rule-to-execute-event
branch
2 times, most recently
from
6371d3f to
5d4b88b
Compare
pmuellr
force-pushed
the
actions/add-rule-to-execute-event
branch
from
5d4b88b to
3abc8ff
Compare
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
ymao1
approved these changes
Jun 21, 2021
| @@ -154,6 +157,16 @@ export class ActionExecutor { | |||
| }, | |||
| }; | |||
|
|
|||
| for (const relatedSavedObject of relatedSavedObjects || []) { | |||
| event.kibana?.saved_objects?.push({ | |||
| rel: SAVED_OBJECT_REL_PRIMARY, | |||
There was a problem hiding this comment.
I'm actually unclear what primary should mean when it's used here, but there is already a primary saved object that links to the action SO. Should these related SOs also be marked as primary?
There was a problem hiding this comment.
I agree, this is confusing for me too. There doesn't seem to be another option either
There was a problem hiding this comment.
Right, it is confusing. We added this rel: "primary" field as a kind of primitive way of filtering out some of the searches across the event log by saved object. Specifically, we didn't want someone with access to a connector, but not a rule, to see the "execute-action" event, which is logged when an action is queued to be executed by a rule. Since that would "leak" the rule to someone who doesn't have access to it.
So this rel field comes into play when searching the event log by saved object. We specifically require in the search that the saved object you're searching for should have a rel: primary field, otherwise we won't return that event log document.
This probably isn't even needed any more, since I believe that is the only case we DO NOT set the rel property - so I'll open an issue.
In this specific case, of the "action execute" event, it does seem like you'd want to find the event if you have either access to the connector or the rule, so it's set to "primary" on both.
And confusingly, if opposite of rel: primary is the absence of the rel field - not an actual value. So there is only one valid string value of rel: primary.
There was a problem hiding this comment.
I was going to note that the README doc could stand to be updated, to help clarify this, but I see there is already some discussion in there, at the end of this section: https://github.com/elastic/kibana/tree/master/x-pack/plugins/event_log#event-documents
However, I notice that it's now out-of-date! So I'll fix that up. And take requests for additional cleanup / clarification of that section regarding the confusion aspect.
There was a problem hiding this comment.
Opened issue #102811 to track if we can get rid of rel:primary
chrisronline
approved these changes
Jun 21, 2021
| @@ -154,6 +157,16 @@ export class ActionExecutor { | |||
| }, | |||
| }; | |||
|
|
|||
| for (const relatedSavedObject of relatedSavedObjects || []) { | |||
| event.kibana?.saved_objects?.push({ | |||
| rel: SAVED_OBJECT_REL_PRIMARY, | |||
There was a problem hiding this comment.
I agree, this is confusing for me too. There doesn't seem to be another option either
pmuellr
added a commit
to pmuellr/kibana
that referenced
this pull request
Jun 22, 2021
…og doc (elastic#101526) resolves elastic#99225 Prior to this PR, when an alerting connection action was executed, the event log document generated did not contain a reference to the originating rule. This makes it difficult to diagnose problems with connector errors, since the error is often in the parameters specified in the actions in the alert. In this PR, a reference to the alerting rule is added to the saved_objects field in the event document for these events.
pmuellr
added a commit
that referenced
this pull request
Jun 22, 2021
…og doc (#101526) (#102994) resolves #99225 Prior to this PR, when an alerting connection action was executed, the event log document generated did not contain a reference to the originating rule. This makes it difficult to diagnose problems with connector errors, since the error is often in the parameters specified in the actions in the alert. In this PR, a reference to the alerting rule is added to the saved_objects field in the event document for these events.
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / task-queue-process-18 / X-Pack Endpoint API Integration Tests.x-pack/test/security_solution_endpoint_api_int/apis/metadata·ts.Endpoint plugin test metadata api POST /api/endpoint/metadata when index is not empty metadata api should return one entry for each host with default pagingStandard OutStack TraceKibana Pipeline / general / task-queue-process-18 / X-Pack Endpoint API Integration Tests.x-pack/test/security_solution_endpoint_api_int/apis/metadata·ts.Endpoint plugin test metadata api POST /api/endpoint/metadata when index is not empty metadata api should return one entry for each host with default pagingStandard OutStack TraceKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/visualize/_vega_chart·ts.visualize app visualize ciGroup12 vega chart in visualize app vega chart initial render should have view and control containersStandard OutStack Traceand 1 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #99225
Summary
Currently, when an alerting connector action is executed, the event log document generated does not contain a reference to the originating rule. This makes it difficult to diagnose problems with connector errors, since the error is often in the parameters specified in the actions in the alert.
In this PR, we're adding a reference to the rule that caused the connector action to be executed, in the event log doc that it generates.
Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsAny UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listThis renders correctly on smaller devices using a responsive layout. (You can test this in your browser)This was checked for cross-browser compatibilityFor maintainers
This was checked for breaking API changes and was labeled appropriately