[SIEM] NP Plugin dependency cleanup #64842

rylnd · 2020-04-29T21:34:42Z

Summary

Addresses #59250.

Removes dependencies from which we only import static code (esUiShared, kibanaUtils)
- We can do this independent of the plugin being enabled/declared; the files will always be imported and included in siem's bundle
Makes usageCollection optional
- If the user disables this, we simply swap their tracking function with a no-op
Removes unused plugin contracts

The following is an audit of our remaining required dependencies:

alerting
- Critical to Detection rules
actions
- transitive dependency through alerting.
- while we don't currently use the plugin contract, we have code referencing its request handler context (although that too is unused), which means we should (at least) declare it as optional
- I'm leaving this as required for now, and I've got an issue for followup: ([SIEM] Remove unused references to actions #64855)
data
- critical to global search/filtering
embeddable
- critical to maps embeddable
features
- critical to privileges registration
home
- could be made optional, however:
- cannot currently be disabled, and is a transient dependency of management besides
inspector
- critical to EmbeddablePanel
licensing
- critical to license-based authorization
maps
- critical to maps embeddable
triggers_actions_ui
- critical to Rule Actions
- critical to case/connector functionality (needs verification: @stephmilovic @cnasikas)
uiActions
- critical to EmbeddablePanel

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

We are only importing static code from these plugins, and not consuming their plugin contracts. For this reason, we can safely remove them from kibana.json as that imported code will always be included in our own bundle.

If it's not enabled, we simply use a noop for our tracker call.

These are only needed when we're actually using them in our codebase. For request handler contexts, we only need our kibana.json declaration.

elasticmachine · 2020-04-29T21:34:44Z

Pinging @elastic/siem (Team:SIEM)

FrankHassanabad · 2020-04-30T16:44:20Z

x-pack/plugins/siem/public/lib/telemetry/index.ts

  } catch (error) {
-    // ignore failed setup here, as we'll just have an inert tracker
+    _track = noop;


Can this part of the exception be hit anymore? It doesn't seem like it with the way you changed it above. Just a question I have.

I had a similar thought! Probably not, unless reportUiStats wasn't a function? I debated between the safety of the try/catch vs the cleanliness of not having it.

FrankHassanabad · 2020-04-30T16:54:58Z

x-pack/plugins/siem/server/plugin.ts

-  alerting: AlertingStart;
-}
+// eslint-disable-next-line @typescript-eslint/no-empty-interface
+export interface StartPlugins {}

 // eslint-disable-next-line @typescript-eslint/no-empty-interface
 export interface PluginSetup {}


You can optionally remove these two empty interfaces if you want and just do this below:

export class Plugin implements IPlugin<void, void, SetupPlugins, StartPlugins> {

And then remove the return {} you have in the start and setup to not return anything unless I'm missing something and we are returning empty objects for a purpose. But it looks like the contracts allow void for return types and we don't have anything useful to return at the moment.

@FrankHassanabad we return {} instead of undefined to allow consumers to distinguish between a disabled plugin and an enabled plugin with no contract. We first encountered this with our newsfeed integration, which resulted in #56236.

FrankHassanabad

LGTM and thanks for the interesting and detailed read in the conversation notes as that helps a lot to understand what is going on server side. 👍

With the addition of the null coalescing, the only chance for an error is in usageCollection. However, if the usageCollection contract changes, we should get a type error long before we see a runtime error.

* passes missing generic arguments to public plugin interface * passes missing generic arguments to both plugins' CoreSetup types

Instead, import them from core as needed.

kibanamachine · 2020-04-30T19:22:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 98e2cc3

History

💚 Build #44259 succeeded b0da9f9

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

* Remove static src dependencies from kibana.json We are only importing static code from these plugins, and not consuming their plugin contracts. For this reason, we can safely remove them from kibana.json as that imported code will always be included in our own bundle. * Make usageCollection an optional dependency If it's not enabled, we simply use a noop for our tracker call. * Remove unused plugin contracts These are only needed when we're actually using them in our codebase. For request handler contexts, we only need our kibana.json declaration. * Remove unnecessary try/catch With the addition of the null coalescing, the only chance for an error is in usageCollection. However, if the usageCollection contract changes, we should get a type error long before we see a runtime error. * Improve typings of our Plugin classes * passes missing generic arguments to public plugin interface * passes missing generic arguments to both plugins' CoreSetup types * Don't re-export core types Instead, import them from core as needed.

kibanamachine · 2020-05-04T19:48:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

* Remove static src dependencies from kibana.json We are only importing static code from these plugins, and not consuming their plugin contracts. For this reason, we can safely remove them from kibana.json as that imported code will always be included in our own bundle. * Make usageCollection an optional dependency If it's not enabled, we simply use a noop for our tracker call. * Remove unused plugin contracts These are only needed when we're actually using them in our codebase. For request handler contexts, we only need our kibana.json declaration. * Remove unnecessary try/catch With the addition of the null coalescing, the only chance for an error is in usageCollection. However, if the usageCollection contract changes, we should get a type error long before we see a runtime error. * Improve typings of our Plugin classes * passes missing generic arguments to public plugin interface * passes missing generic arguments to both plugins' CoreSetup types * Don't re-export core types Instead, import them from core as needed. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

elasticmachine · 2021-09-23T14:35:25Z

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

rylnd added 3 commits April 29, 2020 13:51

Remove static src dependencies from kibana.json

687d301

We are only importing static code from these plugins, and not consuming their plugin contracts. For this reason, we can safely remove them from kibana.json as that imported code will always be included in our own bundle.

Make usageCollection an optional dependency

809382a

If it's not enabled, we simply use a noop for our tracker call.

Remove unused plugin contracts

b0da9f9

These are only needed when we're actually using them in our codebase. For request handler contexts, we only need our kibana.json declaration.

rylnd added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 29, 2020

rylnd requested review from dhurley14, FrankHassanabad, patrykkopycinski, stephmilovic and cnasikas April 29, 2020 21:34

rylnd self-assigned this Apr 29, 2020

rylnd mentioned this pull request Apr 29, 2020

[SIEM] Remove unused references to actions #64855

Closed

rylnd marked this pull request as ready for review April 30, 2020 00:15

rylnd requested review from a team as code owners April 30, 2020 00:15

FrankHassanabad reviewed Apr 30, 2020

View reviewed changes

FrankHassanabad approved these changes Apr 30, 2020

View reviewed changes

rylnd added 4 commits April 30, 2020 11:58

Merge branch 'master' into siem_dependency_trimming

a71022f

Remove unnecessary try/catch

11f0662

With the addition of the null coalescing, the only chance for an error is in usageCollection. However, if the usageCollection contract changes, we should get a type error long before we see a runtime error.

Improve typings of our Plugin classes

808b1b4

* passes missing generic arguments to public plugin interface * passes missing generic arguments to both plugins' CoreSetup types

Don't re-export core types

98e2cc3

Instead, import them from core as needed.

rylnd merged commit b8e0730 into elastic:master Apr 30, 2020

rylnd deleted the siem_dependency_trimming branch April 30, 2020 19:48

rylnd mentioned this pull request Apr 30, 2020

[7.x] [SIEM] NP Plugin dependency cleanup (#64842) #64955

Merged

kibanamachine added backport missing Added to PRs automatically when the are determined to be missing a backport. and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels May 4, 2020

rylnd mentioned this pull request Jul 29, 2020

[SIEM] Audit our plugin dependencies and their availability #59250

Closed

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM] NP Plugin dependency cleanup #64842

[SIEM] NP Plugin dependency cleanup #64842

rylnd commented Apr 29, 2020 •

edited

Loading

elasticmachine commented Apr 29, 2020

FrankHassanabad Apr 30, 2020

rylnd Apr 30, 2020

FrankHassanabad Apr 30, 2020

rylnd Apr 30, 2020

FrankHassanabad left a comment

kibanamachine commented Apr 30, 2020

kibanamachine commented May 4, 2020

elasticmachine commented Sep 23, 2021

[SIEM] NP Plugin dependency cleanup #64842

[SIEM] NP Plugin dependency cleanup #64842

Conversation

rylnd commented Apr 29, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Apr 29, 2020

FrankHassanabad Apr 30, 2020

Choose a reason for hiding this comment

rylnd Apr 30, 2020

Choose a reason for hiding this comment

FrankHassanabad Apr 30, 2020

Choose a reason for hiding this comment

rylnd Apr 30, 2020

Choose a reason for hiding this comment

FrankHassanabad left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 30, 2020

💚 Build Succeeded

History

kibanamachine commented May 4, 2020

elasticmachine commented Sep 23, 2021

rylnd commented Apr 29, 2020 •

edited

Loading