-
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
[Response Ops][Cases] Fetch alerts within observability #123883
Conversation
setAlert(parsedAlert); | ||
setLoading(false); |
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 we want to set loading to false when we're done, not only when we successfully get data?
} | ||
}; | ||
|
||
if (!isEmpty(alertId) && loading === false && alert === null) { |
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 was running into rerender issues here as well when I added the abort signal, but removing these dependencies seemed to fix it. I don't think we want to rely on loading here because it'll change during the fetch.
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.
you still want to check that is not loading or you can end up loading the same data twice.
@@ -6,3 +6,5 @@ | |||
*/ | |||
|
|||
export type { CaseActionConnector } from '../../common/ui/types'; | |||
|
|||
export type FetchAlertDataFunction = (alertIds: string[]) => [boolean, Record<string, unknown>]; |
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.
Should this be in common/ui
instead?
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.
Yeap it makes more sense to me. Also, I think it is better if we renamed it to UseFetchAlertData
so we can understand that the type is about a React hook.
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
@elasticmachine merge upstream |
const [loading, setLoading] = useState(false); | ||
const [alerts, setAlerts] = useState<Record<string, unknown> | undefined>(undefined); | ||
|
||
const fetch = useCallback( |
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.
alternatively you can write this fetch function out of the hook, accept parameters and returns a promise, then where you use it you await the promise and call setAlerts
there.
@@ -52,7 +53,7 @@ export interface UserActionBuilderArgs { | |||
selectedOutlineCommentId: string; | |||
loadingCommentIds: string[]; | |||
loadingAlertData: boolean; | |||
alertData: Record<string, Ecs>; | |||
alertData: Record<string, unknown>; |
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.
is unknown
really the best type here? is there a closer real type we can use for this? like a Record or a string?
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'm open to other ideas, but I'm not quite sure what to do since security solution's implementation of this hook used to return an Ecs
object with the fields broken done into nested objects like:
{
kibana: {
alert: "some value",
}
}
Where as this function will return it as:
{
"kibana.alert": "some value"
}
So the values could be object | string | string[] | number | number[]
and potentially nested
By putting unknown
I think we force the recipient to figure out what type the data is before they use it which might be safe.
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 roll with this one and create an issue to work on a proper data 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.
Will do, I'll create an issue
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.
Issue: #124048
import { BASE_RAC_ALERTS_API_PATH } from '../../../../rule_registry/common/constants'; | ||
|
||
export const useFetchAlertData = (alertIds: string[]): [boolean, Record<string, unknown>] => { | ||
const { http } = useKibana().services; |
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.
tip: you can also use useHttp()
it returns the http object directly.
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.
Are you referring to this one? https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/public/common/lib/kibana/hooks.ts#L32
Doesn't look like I can access that while inside of observability, should I move the useHttp
hook into cases/common
?
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 do not think you should move it to cases/common so other plugins can use it. useHttp
is a cases helper for internal use and useKibana
is more appropriate in other plugins (unless they have their own helpers).
…ana into cases-obs-rule-link
@elasticmachine merge upstream |
@elasticmachine merge upstream |
}); | ||
|
||
useEffect(() => { | ||
fetch(); |
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.
this is a way to do it but it adds extra complications that are not necessary.
You can start the fetch immediately inside useDataFetcher
and invoke it here, have it return a promise and await for that promise here.
@elasticmachine merge upstream |
@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.
Tested locally and LGTM
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 30ed7bb)
💔 Some backports could not be created
How to fixRe-run the backport manually:
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 30ed7bb) # Conflicts: # x-pack/plugins/cases/README.md # x-pack/plugins/cases/public/components/app/types.ts # x-pack/plugins/cases/public/components/case_view/types.ts # x-pack/plugins/cases/public/components/user_actions/comment/alert.tsx # x-pack/plugins/cases/public/components/user_actions/index.test.tsx # x-pack/plugins/cases/public/components/user_actions/index.tsx # x-pack/plugins/cases/public/components/user_actions/types.ts # x-pack/plugins/observability/public/pages/cases/cases.tsx # x-pack/plugins/security_solution/public/cases/pages/use_fetch_alert_data.ts
… (#125370) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com>
Fixes: #123755
This PR adds functionality to cases within observability to retrieve the alerts data so the rules links can be built in the event that the case comments does not have the information.
If the alert can't be found it will still mark the rule as
Unknown
.Example working in observability
Note
The alerts client returns the alert data in the form of collapsed fields
"a.b.c"
instead of it being nested. I removed the requirement for theuseFetchAlertData
to return Ecs because that will be misleading. Security solution has functionality that transforms the collapsed fields into nested fields by recursively iterating over the results. I'm open to doing that as well but we'd need to include those helper functions and I don't think it buys us much since we can just use lodash'sget
to access it as a collapsed field.Example ES response
Testing
To get an alert to show up in observability I typically navigate to the Metrics section in observability. Then at the top where it says Alerts -> Create -> Metrics and then use a Document Count is above 0. Then all you need to do is add some data and the rule should be active and show up under the Alerts tab.
Once you have that, grab the alert ID out of the comment and make a postman request with this body:
POST api/cases/<case id>/comments
The case should show two alerts that both have the rule name link created. Following the link should take you to the appropriate rule.
Release Notes
This fixes an issue where the rule's link in Cases within Observability were displayed as
Unknown rule
even when we could retrieve the correct rule name.Before:
After: