-
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
[Cases] Implement alert details metrics #120982
[Cases] Implement alert details metrics #120982
Conversation
total: 2, | ||
values: [{ name: 'host1', id: '1', count: 1 }], | ||
}); | ||
expect(metrics.alerts?.users).toBeUndefined(); |
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.
Now that we're implementing the metrics since we only passed alertHosts
we shouldn't receive anything regarding users.
@@ -10,4 +10,5 @@ import { CaseMetricsResponse } from '../../../common/api'; | |||
export interface MetricsHandler { | |||
getFeatures(): Set<string>; | |||
compute(): Promise<CaseMetricsResponse>; | |||
setupFeature?(feature: string): void; |
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.
Allows the handler to optionally implement a method to do some pre-compute setup for each feature.
const getName = (bucket: FieldAggregateBucket) => { | ||
const unsafeHostName = get(bucket.top_fields.hits.hits[0].fields, 'host.name'); | ||
|
||
if (Array.isArray(unsafeHostName) && unsafeHostName.length > 0) { |
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.
docvalues returns things as an array
docs: Alert[]; | ||
} | ||
|
||
function isEmptyAlert(alert: AlertInfo): boolean { |
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.
Just moved this to a private static function within the class.
@@ -66,65 +60,5 @@ export default ({ getService }: FtrProviderContext): void => { | |||
expect(errorResponse.message).to.contain('invalid features'); | |||
}); | |||
}); | |||
|
|||
describe('alerts', () => { |
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.
Moved to its own file
@@ -20,44 +24,8 @@ describe('getMetrics', () => { | |||
closed_at: '2021-11-23T19:59:44Z', | |||
}; | |||
|
|||
const client = createCasesClientMock(); |
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.
Just moved these into functions below
return handlersByFeature; | ||
checkAndThrowIfInvalidFeatures(params, handlerFeatures); | ||
|
||
return handlersToExecute; |
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.
We now only return the handles that need to be executed, instead of a map of all of them. That way we can avoid looping through the features and accidentally calling the alerts handler twice (once for users and once for hosts).
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
Pinging @elastic/security-threat-hunting-cases (Feature:Cases) |
|
||
private static buildTotalUniqueAggregations(fields: AggregationFields[]) { | ||
return fields.reduce<Record<string, estypes.AggregationsAggregationContainer>>((acc, field) => { | ||
switch (field) { |
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 the AlertingService
should be agnostic of the fields. We can build the query dynamically by iterating the fields
argument. You may need more information from the AggregationFields
.
const aggs = res.body.aggregations as FieldAggregate; | ||
|
||
return { | ||
hosts: AlertService.getMostFrequentHosts(aggs), |
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.
Same as before. I think the AlertingService
should be agnostic of the fields. The response should the same for all fields.
}; | ||
} | ||
|
||
private static buildFieldAggregations(fields: AggregationFields[], size: number) { |
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.
Same here :)
@elasticmachine merge upstream |
} | ||
} | ||
|
||
const hostName = 'host.name'; |
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.
Hey! I understand the logic of reading from top to bottom but I think variables & interfaces should be on the top of the file. Otherwise is difficult to understand what is going on.
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.
Sounds good, I'll move them to the top 👍
@elasticmachine merge upstream |
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! 🚀
return false; | ||
} | ||
|
||
for (const item of array1) { |
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.
Let's optimize it like this:
const arrayAsSet = new Set(array1)
return array2.every((item) => arrayAsSet.has(item))
This will convert the complexity of the code from O(n^2)
to O(n)
. What do you think?
if (Array.isArray(unsafeHostName) && unsafeHostName.length > 0) { | ||
return unsafeHostName[0]; | ||
} | ||
return unsafeHostName; |
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.
Can this be undefined
?
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.
Yep it can be, lodash.get
could return undefined
. I believe the Array.isArray
will guard the .length
check and then the metrics response allows name
to be undefined
https://github.com/elastic/kibana/pull/120982/files/f5932dd75fb55282dfe5d41f2492f250055dc279#diff-ba2d4ac2973bd7be2ba8b3ae85473e6870ce973584970ff68fda4adc64816e21R24
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'll add it in the return type though so it's more clear 👍
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.
Yeah I think is good to define the return type so TS can catch it when we use the function. lodash.get
return type is any
and we miss the possibility of undefined
. Thanks!
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 🚀 !
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
Pinging @elastic/response-ops (Team:ResponseOps) |
This PR implements the alert details metrics for the backend. Specifically how unique users and hosts are represented in the alerts attached to a case. It also returns the top 10 most frequent users and hosts.
High level changes
setupFeatures
was added to allow handler to setup their features prior to them being computed