Skip to content

[ML] Switching to config files for serverless ML features#166477

Merged
jgowdyelastic merged 18 commits intoelastic:mainfrom
jgowdyelastic:switching-to-config-file-for-serverless-ml-features
Sep 21, 2023
Merged

[ML] Switching to config files for serverless ML features#166477
jgowdyelastic merged 18 commits intoelastic:mainfrom
jgowdyelastic:switching-to-config-file-for-serverless-ml-features

Conversation

@jgowdyelastic
Copy link
Copy Markdown
Member

@jgowdyelastic jgowdyelastic commented Sep 14, 2023

Switches to using the serverless config file to enabled/disable ML features rather than a function shared from the setup contract.
Storing these flags in a config file means they are already available when setup runs and so can be used when registering integrations into other plugins.

Removes the dependency on ML from security_solution_serverless, serverless_observability and serverless_search

@jgowdyelastic jgowdyelastic marked this pull request as ready for review September 19, 2023 06:59
@jgowdyelastic jgowdyelastic requested review from a team as code owners September 19, 2023 06:59
@jgowdyelastic jgowdyelastic requested a review from a team September 19, 2023 06:59
@jgowdyelastic jgowdyelastic requested review from a team as code owners September 19, 2023 06:59
@jgowdyelastic jgowdyelastic requested review from a team September 19, 2023 06:59
@jgowdyelastic jgowdyelastic requested review from a team as code owners September 19, 2023 06:59
@jgowdyelastic jgowdyelastic requested a review from a team September 19, 2023 06:59
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner September 19, 2023 06:59
@jgowdyelastic jgowdyelastic requested review from darnautov and qn895 and removed request for darnautov September 19, 2023 08:16
@jgowdyelastic jgowdyelastic requested a review from qn895 September 19, 2023 16:16
registerAnomalyDetectionAlertType(alertParams);
registerJobsMonitoringRuleType(alertParams);
export function registerMlAlerts(alertParams: RegisterAlertParams, enabledFeatures: MlFeatures) {
if (enabledFeatures.ad === true) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

question/suggestion: did you consider performing a check before registerMlAlerts and registerCasesPersistableState are called?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm passing in the enabled features into registerMlAlerts and have the check happen in there.
I did it this way to allow for future non AD alerts to still be registered by this function. As in, the registerMlAlerts function will decide which alerts to register.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To move the check out into the plugin.ts file, these register functions would need to be renamed to have AD or similar in the name e.g. registerMlADAlerts
I thought it would be neater to have one function to register all ML alerts, and have it decide which ones to register based on feature enablement.


initMlServerLog({ log: this.log });

if (plugins.alerting) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (plugins.alerting) {
if (plugins.alerting && this.enabledFeatures.ad) {

mlSharedServices: sharedServicesProviders,
mlServicesProviders: internalServicesProviders,
},
this.enabledFeatures
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.enabledFeatures

@@ -323,8 +308,12 @@ export class MlServerPlugin
}

if (mlLicense.isMlEnabled() && mlLicense.isFullLicense()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Copy Markdown
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

Security changes LGTM!

@jgowdyelastic jgowdyelastic requested review from qn895 and removed request for qn895 September 20, 2023 15:47
Copy link
Copy Markdown
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM 👍

xpack.securitySolution.enableExperimental:
- discoverInTimeline

xpack.ml.ad.enabled: true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably not related to the changes here, but should the Memory usage page be accessible in the Security project? There is a link to it from the ML home page, but I get the 'Access denied' page.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The memory usage page is currently disabled in serverless.
This may be a mistake. I've pushed a fix to another WIP PR to change this. As that PR has some similar client side changes.

@jgowdyelastic jgowdyelastic marked this pull request as draft September 21, 2023 07:20
@jgowdyelastic jgowdyelastic marked this pull request as ready for review September 21, 2023 07:20
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

re-approving on behalf of core?

@sebelga sebelga self-requested a review September 21, 2023 09:29
Copy link
Copy Markdown
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested each project, and stateful, and LGTM. Just left a comment about renaming a file.

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Sep 21, 2023

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: f3fdd7b
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-166477-f3fdd7bc3aeb

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #31 / serverless common UI Index Management Index Templates "before all" hook for "renders the index templates tab"

Metrics [docs]

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5614 +5614
total size - 6.0MB +6.0MB

History

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

cc @jgowdyelastic

@jgowdyelastic jgowdyelastic merged commit e0ae59f into elastic:main Sep 21, 2023
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Sep 21, 2023
@watson watson added the Project:Serverless Work as part of the Serverless project for its initial release label Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting ci:build-serverless-image :ml Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes v8.11.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.