[AI4DSOC] Move AI4DSOC components from `kbn-elastic-assistant` to `security_solution` by stephmilovic · Pull Request #219288 · elastic/kibana

stephmilovic · 2025-04-25T17:03:40Z

Summary

I made a bad call and put the AI4DSOC alert flyout components in kbn-elastic-assistant. This PR corrects that mistake and moves the files to security_solution.

Waiting to merge until Tuesday to avoid being included in the demo build

elasticmachine · 2025-04-25T17:03:43Z

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

kqualters-elastic · 2025-04-25T17:28:29Z

The root of the problem is that the AssistantContext is completely coupled to the security solution plugin, even though it existing in a package makes that seem like it's not the case. Should either be it's own plugin with a set of dependencies, or used only within security solution. This tight coupling has led to numerous issues I've heard of anecdotally, and one that got me, preventing some changes I was trying to make to security solution routes to help performance in #212808 . Hiding dependencies in a context (as also cases and some other plugins also do) is not a pattern we should use long term, leads to tons of issues like this, duplicated code, performance problems, etc.

stephmilovic · 2025-04-25T17:57:22Z

The root of the problem is that the AssistantContext is completely coupled to the security solution plugin

@kqualters-elastic yeah I get that but does something in this PR specifically bother you? Not sure this is the place for this discussion...?

stephmilovic · 2025-04-25T20:33:57Z

@elasticmachine merge upstream

elasticmachine · 2025-04-28T17:48:11Z

💚 Build Succeeded

Buildkite Build
Commit: 90eb144

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`automaticImport`	800	783	-17
`securitySolution`	7342	7337	-5
total			-22

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/elastic-assistant`	152	166	+14

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.1MB	9.1MB	-2.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/elastic-assistant`	11	13	+2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	91.0KB	91.0KB	+4.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/elastic-assistant`	188	198	+10

ESLint disabled line counts

id	before	after	diff
`@kbn/elastic-assistant`	14	13	-1
`securitySolution`	586	587	+1
total			-0

Total ESLint disabled count

id	before	after	diff
`@kbn/elastic-assistant`	15	14	-1
`securitySolution`	673	674	+1
total			-0

History

💔 Build #296366 failed 302191a
💔 Build #296346 failed bb3f7c0

andrew-goldstein · 2025-04-28T19:16:11Z

...plugins/security_solution/public/attack_discovery/pages/use_find_attack_discoveries/index.ts

 import { useKibanaFeatureFlags } from '../use_kibana_feature_flags';

 interface Props {
+  alertIds?: string[];


This PR is not tagged as being backported to 8.19, but consider backporting this change to simplify future backports and testing, (avoiding diverging implementations).

andrew-goldstein · 2025-04-28T19:16:32Z

...plugins/security_solution/public/attack_discovery/pages/use_find_attack_discoveries/index.ts

 }

-const DEFAULT_PAGE = 1; // CAUTION: sever-side API uses a 1-based page index convention (for consistency with similar existing APIs)
+const DEFAULT_PAGE = 1; // CAUTION: server-side API uses a 1-based page index convention (for consistency with similar existing APIs)


This PR is not tagged as being backported to 8.19, but consider backporting this change to simplify future backports and testing, (avoiding diverging implementations).

andrew-goldstein · 2025-04-28T19:16:38Z

...plugins/security_solution/public/attack_discovery/pages/use_find_attack_discoveries/index.ts

 const DEFAULT_PER_PAGE = 10;

 export const useFindAttackDiscoveries = ({
+  alertIds,


This PR is not tagged as being backported to 8.19, but consider backporting this change to simplify future backports and testing, (avoiding diverging implementations).

Unfortunately, backporting this would lead to a cascade of backports including the following and likely more. I think we may be past that point... @PhilippeOberti what are your thoughts here?

#218018
#219004
#214889
#215246
#216744
#217421
#217696
#217744

I don't think we should backport this... Like @stephmilovic said above, we'd have to backport a bunch of PRs (24 to be exact) and I don't think it's a good idea...

If we truly believe we should, I'll get started on backporting the PRs. Thankfully I kept most of them very targeted and with low impact on the rest of the code, but some of them will be more involved...

The thing to keep in mind is many other PRs from other teams (outside of the 24 I linked above which are just for the alert summary work) have not been backported as well. So we'd have to sync with the rest of the group to make sure that we backport everything? Or you'd have a bunch of code in 8.19 that is untestable and it would make no sense... while also adding risk to break something in the process...

Synced with Andrew over zoom. He just wants me to bring the changes to this specific file over to 8.19, since it exists there, in order to bring it up to speed with main. This is going to be a manual PR, not using the backport tool, so limited to the changes to this file. He wasn't suggesting we backport the whole PR, just the file changes. Sorry for the confusion @PhilippeOberti

Haaaaaa that makes sense, all good then!! 😆

andrew-goldstein · 2025-04-28T19:16:45Z

...plugins/security_solution/public/attack_discovery/pages/use_find_attack_discoveries/index.ts

        method: 'GET',
        version: API_VERSIONS.internal.v1,
        query: {
+          alert_ids: alertIds,


This PR is not tagged as being backported to 8.19, but consider backporting this change to simplify future backports and testing, (avoiding diverging implementations).

andrew-goldstein · 2025-04-28T19:16:52Z

...plugins/security_solution/public/attack_discovery/pages/use_find_attack_discoveries/index.ts

        signal: abortController.current.signal,
      }),
    [
+      alertIds,


This PR is not tagged as being backported to 8.19, but consider backporting this change to simplify future backports and testing, (avoiding diverging implementations).

andrew-goldstein · 2025-04-28T19:17:07Z

...plugins/security_solution/public/attack_discovery/pages/use_find_attack_discoveries/index.ts

    [
      'GET',
      ATTACK_DISCOVERY_FIND,
+      alertIds,


This PR is not tagged as being backported to 8.19, but consider backporting this change to simplify future backports and testing, (avoiding diverging implementations).

andrew-goldstein

Thanks @stephmilovic!
✅ Desk tested locally
LGTM

…curity_solution` (elastic#219288)

stephmilovic added 3 commits April 25, 2025 10:34

move

9938f72

more moving

55eb47d

revert

bb3f7c0

stephmilovic requested review from a team as code owners April 25, 2025 17:03

fix translations

302191a

elasticmachine and others added 2 commits April 25, 2025 22:34

Merge branch 'main' into move_ai4dsoc_to_ss

fded4f6

Merge branch 'main' into move_ai4dsoc_to_ss

90eb144

andrew-goldstein reviewed Apr 28, 2025

View reviewed changes

andrew-goldstein approved these changes Apr 28, 2025

View reviewed changes

stephmilovic merged commit 89fc5f9 into elastic:main Apr 29, 2025
9 checks passed

stephmilovic mentioned this pull request Apr 29, 2025

[8.19] Move AI4DSOC components from kbn-elastic-assistant to security_solution #219559

Merged

akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request Apr 30, 2025

[AI4DSOC] Move AI4DSOC components from kbn-elastic-assistant to `se…

4e14f0c

…curity_solution` (elastic#219288)

akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request May 29, 2025

[AI4DSOC] Move AI4DSOC components from kbn-elastic-assistant to `se…

80905c1

…curity_solution` (elastic#219288)

PhilippeOberti mentioned this pull request Jun 5, 2025

[8.19] [AI4DSOC] Alert Flyout (#218018) #222689

Merged

stephmilovic added a commit to PhilippeOberti/kibana that referenced this pull request Jun 5, 2025

[AI4DSOC] Move AI4DSOC components from kbn-elastic-assistant to `se…

34b66f3

…curity_solution` (elastic#219288)

Conversation

stephmilovic commented Apr 25, 2025

Summary

Uh oh!

elasticmachine commented Apr 25, 2025

Uh oh!

kqualters-elastic commented Apr 25, 2025

Uh oh!

stephmilovic commented Apr 25, 2025

Uh oh!

stephmilovic commented Apr 25, 2025

Uh oh!

elasticmachine commented Apr 28, 2025

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-goldstein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants