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

[EventLog] Added event log API to get events for multiple saved objects. #87596

Merged
merged 12 commits into from
Jan 13, 2021

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Jan 6, 2021

Added support for getting events by the object type and multiple ids: exposed by EventLogClient and find_by_ids REST API
Resolve #70856

@YulNaumenko YulNaumenko self-assigned this Jan 6, 2021
@YulNaumenko YulNaumenko linked an issue Jan 6, 2021 that may be closed by this pull request
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it's headed in the right direction. We'll have more work to do to get to the point where we can use this to get all active instances - this isn't all of it. So I think the title should be changed to "Added event log API to get events for multiple saved objects".

? Promise.all(
objects.map(async (objectItem) => await (await client).get({ id: objectItem.id }))
)
: new Promise(() => []);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: new Promise(() => []);
: Promise.resolve([])

I don't think the new Promise(() => []) is doing what we want - it seems to return a promise that never resolves, but I think we want a promise that resolves to an empty array instead.

Same pattern is in the alerts plugin ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better, current PR can close the opened issue #70856 and the alerting related work will be a separate PR

@YulNaumenko YulNaumenko linked an issue Jan 11, 2021 that may be closed by this pull request
@YulNaumenko YulNaumenko changed the title [Alerts] Added alerting API to get all active instances [EventLog] Added event log bulk facility Jan 11, 2021
@YulNaumenko YulNaumenko changed the title [EventLog] Added event log bulk facility [EventLog] Added event log API to get events for multiple saved objects. Jan 11, 2021
@YulNaumenko YulNaumenko marked this pull request as ready for review January 11, 2021 17:14
@YulNaumenko YulNaumenko requested a review from a team as a code owner January 11, 2021 17:14
@YulNaumenko YulNaumenko linked an issue Jan 11, 2021 that may be closed by this pull request
@YulNaumenko YulNaumenko added Feature:EventLog Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Jan 11, 2021
@elasticmachine
Copy link
Contributor

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

@YulNaumenko YulNaumenko added v7.12.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 11, 2021
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

const objects = ids.reduce(
(prev: Array<{ type: string; id: string }>, id) => [...prev, { type, id }],
[]
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be clearer to use a map instead of a reduce
const objects = ids.map((id: string) => ({ type, id }));

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@YulNaumenko YulNaumenko merged commit fb67443 into elastic:master Jan 13, 2021
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jan 13, 2021
…ts. (elastic#87596)

* Added alerting API to get all active instances

* modofied event log findEventsBySavedObject to support bulk ids, renamed to findEventsBySavedObjectIds

* fixed faling typechecks

* fixed crash on zpd/api/event_log/alert/84c00970-5130-11eb-9fa7/_find for non existing id

* fixed faling typechecks

* fixed faling typechecks

* fixed due to comments

* fixed due to comments

* fixed failing test

* fixed due to comments
YulNaumenko added a commit that referenced this pull request Jan 13, 2021
…ts. (#87596) (#88116)

* Added alerting API to get all active instances

* modofied event log findEventsBySavedObject to support bulk ids, renamed to findEventsBySavedObjectIds

* fixed faling typechecks

* fixed crash on zpd/api/event_log/alert/84c00970-5130-11eb-9fa7/_find for non existing id

* fixed faling typechecks

* fixed faling typechecks

* fixed due to comments

* fixed due to comments

* fixed failing test

* fixed due to comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:EventLog needs_docs release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[eventLog] provide bulk query facility API to get all active instances from Observability consumers
6 participants