-
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
[Event Log] Added a new event log API, which allows to fetch events summary aggregations for saved objects by Ids. #91731
Conversation
…ummary aggregations for saved objects by Ids
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
@@ -156,6 +156,29 @@ export default function ({ getService }: FtrProviderContext) { | |||
expect(eventsUntil.length).to.be(expectedEvents.length + 1); | |||
assertEventsFromApiMatchCreatedEvents(eventsUntil, [firstEvent, ...expectedEvents]); | |||
}); | |||
|
|||
it('should aggregate events summary by Saved Object ids', async () => { |
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 feel that some tests are missing here:
- What happens when a user requests SOs they don't have permission to see?
- What happens when a user requests a mix of SOs they are allowed to see and SOs they're not allowed to see?
- What happens when a user requests SOs that don't exist?
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.
Testing this locally, I'm not sure the behaviour I'm seeing aligns with what I'd expect.
If a user specifies a SO that doesn't exist or they have no permission to view - this just gets silently ignored.
I think we should actually return errors in these cases... 404s... or 302s.... accordingly.
What do you think?
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 404 should be the right error message.
Closing this PR in favor of the upcoming changes for the concept "Alert as a data". Observability team currently doesn't rely any more on this API. |
Time to open this back up! RAC is looking to do aggs over the event log. Hopefully it's still in good shape! |
Nice! I will take a look. If it's not in the shape I will fix it. @pmuellr do you the opened issue for this? Or the requirements is still the same? |
Responded off-line, realized I should have here as well:
I've taken a quick look, the conflicts don't look bad, will hopefully have a commit with merge to master later today.
I'm going to have the RAC folks see if the API shape fits their requirements - as long as it does, I'm not sure we need a new issue opened at this point. Seems like #91265 is the best issue to focus on for this PR, although I wouldn't say it resolves that issue, more that this PR is required to resolve that issue. I don't mind if we create an issue specifically for this though ... |
I've merged master on this, but it was as of last week while working out the conflicts. So will need another merge, which I'll do from here, after this comment. There's a typescript error in the query, so that will need to be resolved, but there's some other bits to do like handling the date ranges, allow additional filter clauses, etc. |
@elasticmachine merge upstream |
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / jest / Jest Tests.x-pack/plugins/event_log/server/es.queryEventsSummaryBySavedObjectIds should call cluster with proper aggregations and with default namespaceStandard Out
Stack Trace
Kibana Pipeline / general / X-Pack Case API Integration Tests.x-pack/test/case_api_integration/security_and_spaces/tests/common/cases/import_export·ts.cases security and spaces enabled: basic Common import and export cases imports a case with a connectorStandard Out
Stack Trace
Kibana Pipeline / general / X-Pack Case API Integration Tests.x-pack/test/case_api_integration/security_and_spaces/tests/common/cases/import_export·ts.cases security and spaces enabled: basic Common import and export cases imports a case with a connectorStandard Out
Stack Trace
and 1 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @YulNaumenko |
As of commit 2efb2b8 , I've got master merged again after this PR was closed for a few months, and also done a subsequent merge master. Still a lot of work to go:
|
Closing as we still don't have a hard requirement for this (I don't think) and the PR is quite old at this point. We'll have to dust it off again next time we make a go at it ... |
We've started doing some aggs on the event log, through another mechanism, for telemetry purposes. These usages don't need the RBAC support, but presumably we'd have some "core" aggs support which would then be used with an RBAC filter (or SO id filter). Examples:
Both of these use cases also use scripts, so if we want those use cases to also be fulfilled with first class aggs support on the event log, we'd also need to add script support. Perhaps that would just be internal? |
I've asked @spong to file an issue with clear requirements given the needs raised in #124198.
Indeed, the first pass should likely be internal, exposed for the needs raised in @spong 's issue. We can then explore opening it up once we're confident the API makes sense for broader use. |
@banderror has created this issue for the Detection Rules Area (#125645) outlining the requirements for adding aggregations to the event-log client. |
Current PR exposes a new event log API, which allows to fetch events summary aggregations for saved objects by Ids.
Example:
POST /api/event_log/{type}/saved_object_summary?start=2021-02-04T06:39:27.442Z&end=2021-02-04T06:39:27.442Z
Body example