-
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
[RCA] Start investigation from alert details page #190307
[RCA] Start investigation from alert details page #190307
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
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 some comments about the cache keys.
x-pack/plugins/observability_solution/investigate_app/public/hooks/query_key_factory.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/investigate_app/server/services/create_investigation.ts
Outdated
Show resolved
Hide resolved
/ci |
/ci |
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 did a quick review before the meeting, will continue after. Looking good so far
...solution/investigate_app/server/routes/get_global_investigate_app_server_route_repository.ts
Outdated
Show resolved
Hide resolved
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.
Changes to kibana.jsonc
LGTM
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 left some comments, thanks for the initial refactoring of the search API.
A few things that we can tackle in another PR: duplication of hooks
x-pack/plugins/observability_solution/investigate/common/schema/create.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/investigate/common/schema/find.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/investigate/common/schema/investigation.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/investigate_app/public/hooks/use_get_alert_details.tsx
Outdated
Show resolved
Hide resolved
...lugins/observability_solution/investigate_app/public/hooks/use_get_investigation_details.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/investigate_app/server/models/investigation.ts
Outdated
Show resolved
Hide resolved
const alertId = investigationDetails?.origin.id ?? ''; | ||
|
||
const { | ||
data: alertDetails, | ||
isLoading: isFetchAlertLoading, | ||
isError: isFetchAlertError, | ||
} = useFetchAlert({ id: alertId }); |
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.
More of a philosophical question, but do you think the get investigation API should query RAC and include the alert details directly in its response?
So instead of doing two request sequentially, the client would just do one and get everything?
That's maybe a bit of a premature optimization. But something to think about.
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.
that's a good question, something I was thinking about it too while implementing. We would need to have rac
in context to use alerts client. I will look into it.
...ability_solution/observability/public/pages/alert_details/hooks/use_create_investigation.tsx
Show resolved
Hide resolved
return ( | ||
<> | ||
<EuiFlexGroup direction="row" gutterSize="s" justifyContent="flexEnd"> | ||
{investigate && |
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 investigate
null when the feature flag is off?
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 undefined
} = useKibana().services; | ||
|
||
const { rule, refetch } = useFetchRule({ | ||
ruleId: alert?.fields[ALERT_RULE_UUID] || '', | ||
}); | ||
|
||
const { data: investigations } = useFetchInvestigationsByAlert({ |
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 should not even try to fetch the investigations in case the investigate feature flag is not enabled
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.
Good catch. Addressed in 5a24c0b
…a/find.ts Co-authored-by: Kevin Delemme <kdelemme@gmail.com>
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @benakansara |
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, I think we'll have to move things around to avoid too much duplication at some point. But for now, it's fine.
title: 'Something went wrong while fetching Investigations', | ||
}); | ||
}, | ||
enabled: Boolean(investigatePlugin), |
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.
👍🏻 nice
Resolves #190320 and #190396
ongoing
alert
"Start investigation" is hidden for other alert types and when investigate plugin is disabled.
Testing
kibana.dev.yml
Screen.Recording.2024-08-12.at.12.24.02.mov