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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Add quick job installation to anomalies table #142861

Merged
merged 10 commits into from
Oct 20, 2022
Merged

[Security Solution] Add quick job installation to anomalies table #142861

merged 10 commits into from
Oct 20, 2022

Conversation

machadoum
Copy link
Member

@machadoum machadoum commented Oct 6, 2022 •
edited
Loading

figma
issue

Summary

Add the 'Run job' button to the anomalies table. The button runs the job and installs it when the module is uninstalled.

Implementation details:

  • It replaces useInstalledSecurityJobs by useSecurityJobs to also load uninstalled jobs.
  • It updates the error handling code to throw some errors that were previously swallowed.
  • Refactor data feed enablement logic to be reusable by making it a hook.
  • Refactor useSecurityJobs(refetchData: boolean) refetch param from toggle to a callback function.

Oct-17-2022 10-49-39

Checklist

@machadoum machadoum changed the title Wip [Security Solution] Add quick job installation to anomalies table Oct 6, 2022
@machadoum machadoum self-assigned this Oct 17, 2022
@machadoum machadoum added release_note:enhancement enhancement New value added to drive a business result Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore labels Oct 17, 2022
@machadoum machadoum marked this pull request as ready for review October 17, 2022 09:38
@machadoum machadoum requested review from a team as code owners October 17, 2022 09:38
@machadoum machadoum requested a review from banderror October 17, 2022 09:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

jamster10
jamster10 reviewed Oct 18, 2022
View reviewed changes
x-pack/plugins/security_solution/public/common/components/ml/api/errors.ts
@@ -19,3 +20,6 @@ export interface MlStartJobError {
// Otherwise for now, has will work ok even though it casts 'unknown' to 'any'
export const isMlStartJobError = (value: unknown): value is MlStartJobError =>
has('error.msg', value) && has('error.response', value) && has('error.statusCode', value);

export const isUnknownError = (value: unknown): value is ErrorResponseBase =>
has('error.error.reason', value);
Copy link
Contributor

Choose a reason for hiding this comment

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

error.error 😬 😅 . I know we cant probably change that object structure but dang :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it is terrible. But that is what the API returns. 😞

jamster10
jamster10 approved these changes Oct 18, 2022
View reviewed changes
Copy link
Contributor

@jamster10 jamster10 left a comment

Choose a reason for hiding this comment

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

desk tested and reviewed

@machadoum
Copy link
Member Author

@jamster10 I tested that bug you found when the cluster is empty and hopefully it doesn't happen on 8.5.
So I fixed the bug here and we don't have to backport it.

Before the Fix:
Screenshot 2022-10-19 at 10 45 51
After the fix:
Screenshot 2022-10-19 at 10 45 02

How it looks on 8.5
Screenshot 2022-10-19 at 10 18 59

@machadoum
Copy link
Member Author

I updated the loading state to be shared across jobs (same behaviour as ml pop) to fix the issue where the page gets into an inconsistent state when the user starts multiple jobs at once.

Oct-19-2022 15-14-09

banderror
banderror approved these changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Checked out and tested the ML job settings popover on the Rule Management page. Seems to work fine!

I have to admit that I'm not super familiar with the changed code, but I noticed that you've done some refactoring (e.g. useEnableDataFeed) and clean-up, and added new tests. Thank you for that @machadoum!

Left some suggestions.

x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_security_jobs.ts
const [jobs, setJobs] = useState<SecurityJob[]>([]);
const [loading, setLoading] = useState(true);
const mlCapabilities = useMlCapabilities();
const [securitySolutionDefaultIndex] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY);
const http = useHttp();
const { addError } = useAppToasts();

const refetch = useRef<inputsModel.Refetch>(noop);
Copy link
Contributor

Choose a reason for hiding this comment

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

For example, refetching is natively supported by React Query and is a matter of simple config for a given hook.

@machadoum machadoum enabled auto-merge (squash) October 20, 2022 14:25
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #39 / alerting api integration spaces only Alerting builtin alertTypes index_threshold rule runs correctly: avg all
  • [job] [logs] FTR Configs #14 / dashboard drilldowns Dashboard to dashboard drilldown Create & use drilldowns test dashboard to dashboard drilldown with controls use dashboard to dashboard drilldown via onClick action

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3168 3170 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 9.3MB 9.3MB +1.8KB

History

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

cc @machadoum

@machadoum machadoum merged commit 2de7ddc into elastic:main Oct 20, 2022
@kibanamachine kibanamachine added v8.6.0 backport:skip This commit does not require backporting labels Oct 20, 2022
@joepeeples joepeeples mentioned this pull request Nov 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angorayc angorayc angorayc left review comments

@banderror banderror banderror approved these changes

@jamster10 jamster10 jamster10 approved these changes

Assignees

@machadoum machadoum

Labels
backport:skip This commit does not require backporting enhancement New value added to drive a business result release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@machadoum @elasticmachine @kibana-ci @angorayc @banderror @jamster10 @kibanamachine