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

[Event Log] adds query support to the Event Log #62015

Merged
merged 26 commits into from
Apr 6, 2020

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Mar 31, 2020

Summary

Adds a public API for querying the Event Log by a referenced Saved Object at /api/{saved-object-type}/{saved-object-id}/_find

Closes #55633

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@gmmorris gmmorris added release_note:enhancement Feature:Alerting v8.0.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.8 labels Mar 31, 2020
@elasticmachine
Copy link
Contributor

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

* master: (64 commits)
  Adding PagerDuty icon to connectors cards (elastic#60805)
  Fix drag and drop flakiness (elastic#61993)
  Grok debugger migration (elastic#60658)
  Endpoint: Fix resolver SVG position issue (elastic#61886)
  [SIEM] version 7.7 rule import (elastic#61903)
  Added styles to make combobox list items wider for alerting flyout (elastic#61894)
  [UA] Tight worker loop can cause high CPU usage (elastic#60950)
  [ML] DF Analytics results table: use index pattern field format if one exists (elastic#61709)
  [ML] Catching unknown index pattern errors (elastic#61935)
  [Discover] Deangularize and euificate sidebar  (elastic#47559)
  Endpoint: Add ts-node dev dependency (elastic#61884)
  Add an onBlur handler for the kuery bar. Only resubmit when input changes. (elastic#61901)
  [ML] Handle Empty Partition Field Values in Single Metric Viewer (elastic#61649)
  Auto interval on date histogram is getting displayed as timestamp per… (elastic#59171)
  [Maps] Explicitly pass fetch function to ems-client (elastic#61846)
  [SIEM][CASE] Fix aria-labels and translations (elastic#61670)
  [ML] Settings: Increase number of items that can be paged in calendars and filters lists (elastic#61842)
  [EPM] update epm filepath route (elastic#61910)
  APM] Set ignore_above to 1024 for telemetry saved object (elastic#61732)
  [Logs UI] Log stream row rendering (elastic#60773)
  ...
* master: (44 commits)
  [Alerting] add alerting privileges for uptime and metrics (elastic#61113)
  Update percy agent to latest version (elastic#62089)
  [APM] Update central configuration text (elastic#61556)
  [Fleet] Ouput api key do not need metricbeat* access (elastic#60319)
  Document new `xpack.security.authc.*` settings and related 8.0.0 breaking changes. (elastic#61443)
  Migrate test plugins ⇒ NP (kbn_tp_sample_panel_action) (elastic#60749)
  [Alerting] Add "Start trial" button for connectors (elastic#61774)
  [ML] Transforms: Fix handling of default and advanced search on step summary view. (elastic#61799)
  [Task Manager] Change info message "ran out Available Workers" to debug (elastic#62083)
  [Maps] Highlight selected layer in TOC (elastic#61510)
  ensure pageIndex is set correclty in analytics list (elastic#62041)
  [ML] Functional API tests - fix mml request bodies (elastic#62116)
  Fix validation for index threshold when selecting an index (elastic#61615)
  [SIEM][Detection Engine] Adds release notes link and updates one UI section
  [backport] Bump to 5.1.2 (elastic#62117)
  [APM] .apm-agent-configuration is not created if Kibana is started while ES is not ready (elastic#61610)
  [Fleet] Enrollment list page (elastic#61346)
  [ML] Fix maximum default enabled columns for data grid. (elastic#62005)
  [Home][Tutorial] Add Oracle data UI (elastic#61595)
  [APM] Ensure telemetry data matches SO/telemetry mapping (elastic#61957)
  ...
* master:
  [HomeApp] Set breadcrumbs when coming back from add data dir (elastic#62186)
  [Lens] fix error for minInterval>computedInterval for XYChart (elastic#61931)
  ci: remove AppArch label from ProBot path-labeler (elastic#62211)
  [Uptime] Optimize get latest monitor API (elastic#61820)
  [Maps] Separate layer wizards for Clusters and heatmap (elastic#60870)
  Remove polling delay (elastic#62099)
  accessibility tests for dashboard panel ( OSS)  (elastic#62055)
  rename README.md to readme, avoiding issues with case change
  [SIEM] [Detection Engine] Fixes all rules sorting (elastic#62039)
  [SIEM] CASES Bugs BC2 (elastic#62170)
  Revert "Endpoint: Add ts-node dev dependency (elastic#61884)" (elastic#62197)
  Closes elastic#60173 by turning off client caching for the main service map API call (elastic#62111)
  [SIEM] Restores the _External alert count_ widget's subtitle (elastic#62094)
  [Maps] Update ems client dependency to 7.8.0 (elastic#62181)
  [Metrics Alerts] Fix action variables, default message, and EU… (elastic#62061)
  Update CODEOWNERS with ES-UI apps, including grok debugger. (elastic#62045)
  Update ILM node attributes blacklist. (elastic#62093)
Comment on lines 44 to 55
export interface IEventLogClientService {
getClient(request: KibanaRequest): IEventLogClient;
}

export interface IEventLogClient {
findEventsBySavedObject(
type: string,
id: string,
options?: Partial<FindOptionsType>
): Promise<IEvent[]>;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach (rather than extending the existing EventLogService) was in order to align this with the AlertClient which is also exposed on the Start api and extends the context of a request.

Comment on lines +7 to +11
export default function() {
describe('Event Log service API', () => {
it('should allow logging an event', async () => {});
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here as reference for @YulNaumenko it'll be removed once she picks this up for her PR.

Comment on lines 211 to 212
{ page: 10, per_page: 10, start: undefined, end: undefined }
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seems to be a problem with config schema that forces maybe fields to be explicitly marked as undefined.
I'm looking at replacing our use of schema with fp-ts which is what other solutions are migrating to (infact, we now use it in alerting for our SerDe) which should solve this problem.

Copy link
Member

Choose a reason for hiding this comment

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

Ya, it's a pain, which is why I added the DeepWritable/DeepPartial bits for the event interface itself, here:

type DeepWriteable<T> = { -readonly [P in keyof T]: DeepWriteable<T[P]> };
type DeepPartial<T> = {
[P in keyof T]?: T[P] extends Array<infer U> ? Array<DeepPartial<U>> : DeepPartial<T[P]>;
};
export const ECS_VERSION = '1.3.1';
// types and config-schema describing the es structures
export type IValidatedEvent = TypeOf<typeof EventSchema>;
export type IEvent = DeepPartial<DeepWriteable<IValidatedEvent>>;
export const EventSchema = schema.maybe(

There's still something not quite right about that, IIRC, like the top-level is also optional, so when used, even when declaring a var of that type, undefined is a possible value so kind of messes with the typing in a different way. Typing!

I'm a little worried about fp-ts in that the validation messages aren't going to be as "nice" as config-schema's, but for cases where we don't really need to generate super-nice validation messages (like on IEvent itself), doesn't matter, so hopefully fp-ts will be a nice replacement. For http P/Q/B and kibana config, I'm a little more worried.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, should have mentioned, I'm sure we have other tests - eg, for actions - that pass explicit undefined values for the same reason - I think that's fine - we're not expecting customers to be using these types explicitly (at least not yet). We'll fix 'em all when we get a better schema story ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sigh, I went back and forth and ending up reverting the fp-ts stuff... it requires a lot of boiler plate too for things like default values.
It's great for SerDe, but not great for other things.

I'll try and find a typescript solution in the future... for now using a method like yours :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sigh, I went back and forth and ending up reverting the fp-ts stuff... it requires a lot of boiler plate too for things like default values.
It's great for SerDe, but not great for other things.

I'll try and find a typescript solution in the future... for now using a method like yours :/

@mikecote mikecote self-requested a review April 2, 2020 13:45
@mikecote mikecote added v7.8.0 and removed v7.8 labels Apr 2, 2020
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

It's great to see an API started for this! 👍I have a few comments and nits but it's heading towards the right direction!

x-pack/plugins/event_log/server/routes/find.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/event_log/server/routes/find.test.ts Outdated Show resolved Hide resolved
})
);

export const findOptionsSchema = schema.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should support a few more things like sorting and searching at the API level to support the searchbox and table in the UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to leave that for when those features were being built, keep the PR smaller 🤷‍♂

But sure, I've now added sorting.
I'm not sure how we'd want to support searching with a searchbox so I'll leave that for the PR that actually specifies that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, thanks for adding the sorting 👍

esContext: this.esContext,
savedObjectsService: core.savedObjects,
});
return this.eventLogClientService;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should expose explicit functions of the eventLogClientService like getEventLogClient instead to allow future variables to also be part of the start contract.

Copy link
Contributor Author

@gmmorris gmmorris Apr 3, 2020

Choose a reason for hiding this comment

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

Yeah, I thought of that, but chose to align with existing Setup api which exposes a service.
I'll rename eventLogClientService to eventLogStartService which is what I actually had at first but renamed because "Start" is not a very expressive name 🤷‍♂

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense now

Copy link
Member

Choose a reason for hiding this comment

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

We may want to change the shape of the setup/start interfaces to make them more aligned - I guess changing the setup to return a eventLogSetupService or something. We'll see ...

id: string,
options?: Partial<FindOptionsType>
): Promise<IEvent[]> {
await this.savedObjectsClient.get(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.

Might be worth a comment here to explain why it's important to do this even though we're not doing anything with the response. It would be nice to have api integration tests for this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, though there's an explicit unit test that validates this and I consider that more reliable documentation than a comment, but happy to add one 🤷‍♂

* master:
  Switch to embeddable factory interface with optional override (elastic#61165)
  fix text error in diagrams (elastic#62101)
  [Index management] Prepare support Index template V2 format (elastic#61588)
  Updates dashboard images (elastic#62011)
  [Maps] remove MapBounds type (elastic#62332)
  [Uptime] Convert anomaly Jobs name to lowercase to comply with… (elastic#62293)
  Make d3 place nicely with object values (elastic#62004)
  EMT-287: update schema with elastic agent id (elastic#62252)
  [Maps] fix replaceLayerList to handle case where map is not intialized (elastic#62202)
  Remove support for deprecated xpack.telemetry configurations (elastic#51142)
  [Uptime] Remove static constant for index name completely (elastic#62256)
  [APM] E2E: install dependencies for vanilla workspaces (elastic#62178)
  [backport] Bump to 5.1.3 (elastic#62286)
  Show server name in Remote Cluster detail panel (elastic#62250)
  Rename some alert types (elastic#61693)
  changing duration type to ms, s, m (elastic#62265)
  [ML] Clear Kibana index pattern cache on creation or form reset. (elastic#62184)
  Move `src/legacy/server/index_patterns` to data plugin (server) (Remove step) (elastic#61618)
  [NP] Remove IndexedArray usage in Schemas (elastic#61410)
* master:
  [Drilldowns] Dashboard state fixes for drilldowns (elastic#61457)
  allow null for filterQuery (elastic#62310)
  [ML] call job validation endpoint with complete payload (elastic#62331)
  removing configuration from agents (elastic#62290)
  Allow Enterprise license for service map (elastic#62371)
  docs: updates to apm agent config (elastic#61893)
  [Ingest] Fix package info request returning 500 (elastic#61712)
  move crypto to server utils (elastic#62344)
  Start indexing documents by default (elastic#62159)
  [Endpoint] Update host field accordion (elastic#61878)
  Add more definitions about Ingest Management (elastic#62049)
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

LGTM

@gmmorris
Copy link
Contributor Author

gmmorris commented Apr 3, 2020

@elasticmachine merge upstream

@gmmorris
Copy link
Contributor Author

gmmorris commented Apr 5, 2020

@elasticmachine merge upstream

elasticmachine and others added 3 commits April 5, 2020 18:40
…into event-log/query-support

* 'event-log/query-support' of github.com:gmmorris/kibana: (41 commits)
  [jenkins] refer to sizes in most pipeline code (elastic#62082)
  skip flaky suite (elastic#60470)
  [Discover] Fix flaky FT in field visualize (elastic#62418)
  [ML] Data Frame Analytics: Fix feature importance (elastic#61761)
  [Reporting] Use a shim for server config (elastic#62086)
  [Reporting] Fix reporting for non-default spaces (elastic#62226)
  Fix bug that coerced empty scaled float value to 0 (elastic#62251)
  [SIEM] [Detection Engine] Remove has manage api keys requireme… (elastic#62446)
  [Maps] Safely handle empty string and invalid strings from EuiColorPicker (elastic#62507)
  Reporting/bug more blacklisted headers (elastic#62389)
  [SIEM] Prevent undefined behavior in our ML popover (elastic#62498)
  [SIEM] [Detection Engine] remove all unknowns from all rules t… (elastic#62327)
  base changes for active/current node styling (elastic#62007)
  [kbn/ui-shared-deps] expand and split (elastic#62364)
  [ML] DF Analytics - ensure destination index pattern created (elastic#62450)
  Mark rule run as failure if there was an error (elastic#62383)
  Add docs for metric explorer alerts (elastic#62314)
  skip flaky suite (elastic#62281)
  [SIEM][Detection Engine] Fixes export of single rule and the icons
  fixes flakiness (elastic#62406)
  ...
@gmmorris gmmorris requested a review from a team as a code owner April 6, 2020 07:56
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Changes in x-pack/plugins/encrypted_saved_objects/README.md LGTM with a minor nit. Thanks!

x-pack/plugins/encrypted_saved_objects/README.md Outdated Show resolved Hide resolved
x-pack/plugins/encrypted_saved_objects/README.md Outdated Show resolved Hide resolved
gmmorris and others added 4 commits April 6, 2020 09:39
Co-Authored-By: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Co-Authored-By: Aleh Zasypkin <aleh.zasypkin@gmail.com>
…into event-log/query-support

* 'event-log/query-support' of github.com:gmmorris/kibana:
  Update x-pack/plugins/encrypted_saved_objects/README.md
  Update x-pack/plugins/encrypted_saved_objects/README.md
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

x-pack/plugins/event_log/server/event_log_client.test.ts Outdated Show resolved Hide resolved
x-pack/plugins/event_log/server/event_log_client.ts Outdated Show resolved Hide resolved
esContext: this.esContext,
savedObjectsService: core.savedObjects,
});
return this.eventLogClientService;
Copy link
Member

Choose a reason for hiding this comment

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

We may want to change the shape of the setup/start interfaces to make them more aligned - I guess changing the setup to return a eventLogSetupService or something. We'll see ...

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@gmmorris gmmorris merged commit e7a4ca2 into elastic:master Apr 6, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 6, 2020
* added Start api on Event Log plugin

* added empty skeleton for Event Log FTs

* added functional test to public find events api

* added test for pagination

* fixed unit tests

* added support for date ranges

* removed unused code

* replaces valdiation typing

* Revert "replaces valdiation typing"

This reverts commit 711c098.

* replaces match with term

* added sorting

* fixed saved objects nested query

* updated plugin FTs path

* Update x-pack/plugins/encrypted_saved_objects/README.md

Co-Authored-By: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* Update x-pack/plugins/encrypted_saved_objects/README.md

Co-Authored-By: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* remofed validation from tests

* fixed typos

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
gmmorris added a commit that referenced this pull request Apr 6, 2020
Adds a public API for querying the Event Log by a referenced Saved Object at /api/{saved-object-type}/{saved-object-id}/_find
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[alerting event log] add query support
6 participants