-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Security Solution] Improve find rule and find rule status route performance #99678
[Security Solution] Improve find rule and find rule status route performance #99678
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
@elasticmachine merge upstream |
@@ -68,7 +68,7 @@ export const metricsAggsSchemas: Record<string, ObjectType> = { | |||
stored_fields: s.maybe(s.oneOf([s.string(), s.arrayOf(s.string())])), | |||
from: s.maybe(s.number()), | |||
size: s.maybe(s.number()), | |||
sort: s.maybe(s.oneOf([s.literal('asc'), s.literal('desc')])), | |||
sort: s.maybe(s.any()), |
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.
The schema here was too strict - string values in sort
are used as field names, so this only allowed sorting on fields named asc
or desc
. The JS Elasticsearch client types allow the full range of Sort
options for top hits aggs (https://github.com/elastic/elasticsearch-js/blob/master/api/types.d.ts#L11982)
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.
Sorry, I would really like to avoid any
in the aggregation schemas. I'm very fine changing the sort
schema to get closer to the real type (it do not need to be exhaustive here, only to match your needs), but the whole goal of the aggs validation is to not allow arbitrary values.
export type Sort = SortCombinations | Array<SortCombinations>
export type SortCombinations = Field | SortContainer | SortOrder
export type SortOrder = 'asc' | 'desc' | '_doc';
export interface Field {
name: string;
type: string;
esTypes?: string[];
aggregatable: boolean;
filterable: boolean;
searchable: boolean;
subType?: FieldSubType;
}
export interface SortContainerKeys {
_score?: ScoreSort
_doc?: ScoreSort
_geo_distance?: GeoDistanceSort
_script?: ScriptSort
}
export type SortContainer = SortContainerKeys |
{ [property: string]: FieldSort | SortOrder }
From your code, it seems you're using the SortContainer
format
I would introduce a SortSchema
, and also use it in other aggregations where we have a sort
options, e.g terms
.
// note: these schemas are not exhaustive. See the `Sort` type of `@elastic/elasticsearch` if you need to enhance it.
const sortOrderSchema = s.oneOf([s.literal('asc'), s.literal('desc'), s.literal('_doc')]);
const sortContainerSchema = s.recordOf(s.string(), sortOrderSchema);
const sortCombinationsSchema = s.oneOf([sortContainerSchema, sortOrderSchema])
const sortSchema = s.oneOf([sortCombinationsSchema, s.arrayOf(sortCombinationsSchema)])
You can create a src/core/server/saved_objects/service/lib/aggregations/aggs_types/common_schemas.ts
file for that.
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.
No problem, thanks for the feedback! I added the file you suggested with some of the reusable sort schemas and used them to enhance the terms.order
and top hits sort validation.
@elasticmachine merge upstream |
@@ -68,7 +68,7 @@ export const metricsAggsSchemas: Record<string, ObjectType> = { | |||
stored_fields: s.maybe(s.oneOf([s.string(), s.arrayOf(s.string())])), | |||
from: s.maybe(s.number()), | |||
size: s.maybe(s.number()), | |||
sort: s.maybe(s.oneOf([s.literal('asc'), s.literal('desc')])), | |||
sort: s.maybe(s.any()), |
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.
Sorry, I would really like to avoid any
in the aggregation schemas. I'm very fine changing the sort
schema to get closer to the real type (it do not need to be exhaustive here, only to match your needs), but the whole goal of the aggs validation is to not allow arbitrary values.
export type Sort = SortCombinations | Array<SortCombinations>
export type SortCombinations = Field | SortContainer | SortOrder
export type SortOrder = 'asc' | 'desc' | '_doc';
export interface Field {
name: string;
type: string;
esTypes?: string[];
aggregatable: boolean;
filterable: boolean;
searchable: boolean;
subType?: FieldSubType;
}
export interface SortContainerKeys {
_score?: ScoreSort
_doc?: ScoreSort
_geo_distance?: GeoDistanceSort
_script?: ScriptSort
}
export type SortContainer = SortContainerKeys |
{ [property: string]: FieldSort | SortOrder }
From your code, it seems you're using the SortContainer
format
I would introduce a SortSchema
, and also use it in other aggregations where we have a sort
options, e.g terms
.
// note: these schemas are not exhaustive. See the `Sort` type of `@elastic/elasticsearch` if you need to enhance it.
const sortOrderSchema = s.oneOf([s.literal('asc'), s.literal('desc'), s.literal('_doc')]);
const sortContainerSchema = s.recordOf(s.string(), sortOrderSchema);
const sortCombinationsSchema = s.oneOf([sortContainerSchema, sortOrderSchema])
const sortSchema = s.oneOf([sortCombinationsSchema, s.arrayOf(sortCombinationsSchema)])
You can create a src/core/server/saved_objects/service/lib/aggregations/aggs_types/common_schemas.ts
file for that.
…ibana into rules-status-aggs-no-bulk
@elasticmachine merge upstream |
@@ -10,7 +10,7 @@ import moment from 'moment'; | |||
import uuidv5 from 'uuid/v5'; | |||
import dateMath from '@elastic/datemath'; | |||
import type { estypes } from '@elastic/elasticsearch'; | |||
import { isEmpty, partition } from 'lodash'; | |||
import { chunk, isEmpty, partition } from 'lodash'; |
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.
It's ok if you want to use lodash, although still most people pull from lodash/fp
but I don't see a big issue unless I see someone using one the mutatious things from lodash.
const buckets = get(results, 'aggregations.alertIds.buckets'); | ||
return buckets.reduce((acc: Record<string, unknown>, bucket: unknown) => { | ||
const key = get(bucket, 'key'); | ||
const hits = get(bucket, 'most_recent_statuses.hits.hits'); |
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.
nit: I would try to cast here if possible maybe? const hits: someType
even if that type was something loose such as Record<string, unknown>
. I didn't play with it, but less any
is good where/when we can use unknown
or some other type.
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.
Looks straight forward enough for security solutions. Thanks for the improvements, good luck with the core pieces and other feedback on it.
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.
LGTM, thanks for the changes
@elasticmachine merge upstream |
merge conflict between base and head |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: |
…ormance (elastic#99678) * Fetch rule statuses using single aggregation instead of N separate requests * Optimize _find API and _find_statuses * Merge alerting framework errors into rule statuses * Add sortSchema for top hits agg, update terms.order schema Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…ormance (#99678) (#100912) * Fetch rule statuses using single aggregation instead of N separate requests * Optimize _find API and _find_statuses * Merge alerting framework errors into rule statuses * Add sortSchema for top hits agg, update terms.order schema Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
* master: (77 commits) [RAC][Security Solution] Register Security Detection Rules with Rule Registry (elastic#96015) [Enterprise Search] Log warning for Kibana/EntSearch version mismatches (elastic#100809) updating the saved objects test to include more saved object types (elastic#100828) [ML] Fix categorization job view examples link when datafeed uses multiple indices (elastic#100789) Fixing ES archive mapping failure (elastic#100835) Fix bug with Observability > APM header navigation (elastic#100845) [Security Solution][Endpoint] Add event filters summary card to the fleet endpoint tab (elastic#100668) [Actions] Taking space id into account when creating email footer link (elastic#100734) Ensure comments on parameters in arrow functions are captured in the docs and ci metrics. (elastic#100823) [Security Solution] Improve find rule and find rule status route performance (elastic#99678) [DOCS] Adds video to introduction (elastic#100906) [Fleet] Improve combo box for fleet settings (elastic#100603) [Security Solution][Endpoint] Endpoint generator and data loader support for Host Isolation (elastic#100813) [DOCS] Adds Lens video (elastic#100898) [TSVB] [Table tab] Fix "Math" aggregation (elastic#100765) chore(NA): moving @kbn/io-ts-utils into bazel (elastic#100810) [Alerting] Adding feature flag for enabling/disabling rule import and export (elastic#100718) [TSVB] Fix Upgrading from 7.12.1 to 7.13.0 breaks TSVB (elastic#100864) [Lens] Adds dynamic table cell coloring (elastic#95217) [Security Solution][Endpoint] Do not display searchbar in security-trusted apps if there are no items (elastic#100853) ...
Summary
With the implementation of aggregations in saved objects, we can now fetch statuses for multiple rules in one request. This PR implements a
findBulk
method on the RuleStatusClient, adds a bulk "get rule actions" method, and parellelizes the requests in the find rule status route. We can further improve performance in the rule status route by replacing the Nget
calls ingetFailingRules
with a singlegetBulk
call if bulk AlertsClient methods are added (#99216). With an experimentalbulkGet
implementation, this cut the rule status API request time to ~300ms (an additional ~60% reduction over the performance in this PR).Find route
Find status route
Checklist
Delete any items that are not applicable to this PR.
For maintainers