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.

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

[APM] Fixes duplicate ML job creation for existing environments (#85023) #93098

Merged

Conversation

ogupte
Copy link
Contributor

@ogupte ogupte commented Mar 1, 2021

Closes #85023.

Checks for existing ML jobs for the currently selected environments in the API before creating a duplicate jobs. This was only happening on the client-side previously.

Example:

Pre-condition: ML Jobs already exist for APM environments: "test" & "prod"
---
POST /api/apm/settings/anomaly-detection/jobs
{ "environments": [ "test", "prod", "staging" ] }
---
server    log   [14:49:58.851] [warning][apm][plugins] Skipping creation of existing ML jobs for environments: [test,prod]}
server    log   [14:49:58.853] [info][apm][plugins] Creating ML anomaly detection jobs for environments: [staging].

@ogupte ogupte requested a review from a team as a code owner March 1, 2021 19:59
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Mar 1, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

// skip creation of duplicate ML jobs
const jobs = await getAnomalyDetectionJobs(setup, logger);
const existingMlJobEnvs = jobs.map(({ environment }) => environment);
const requestedExistingMlJobEnvs = environments.filter((env) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish partition was a native array method. You can get it from lodash but this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't know about _.partition. It's neat but I had to go to the lodash docs to understand it.
The current code I can read with no manual. So while slightly longer it comes with a lower overhead (for me at least)

Copy link
Member

Choose a reason for hiding this comment

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

Btw. I agree partition would be better if it was native

@@ -38,14 +39,32 @@ export async function createAnomalyDetectionJobs(
throw Boom.forbidden(ML_ERRORS.ML_NOT_AVAILABLE_IN_SPACE);
}

// skip creation of duplicate ML jobs
Copy link
Member

@sorenlouv sorenlouv Mar 1, 2021

Choose a reason for hiding this comment

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

nit: What about extracting this out?

async function getUniqueMlJobEnvs(
  setup: Setup,
  environments: string[],
  logger: Logger
) {
  // skip creation of duplicate ML jobs
  const jobs = await getAnomalyDetectionJobs(setup, logger);
  const existingMlJobEnvs = jobs.map(({ environment }) => environment);
  const requestedExistingMlJobEnvs = environments.filter((env) =>
    existingMlJobEnvs.includes(env)
  );

  if (requestedExistingMlJobEnvs.length) {
    logger.warn(
      `Skipping creation of existing ML jobs for environments: [${requestedExistingMlJobEnvs}]}`
    );
  }

  return environments.filter((env) => !existingMlJobEnvs.includes(env));
}

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Good job! Let's hope this fixes it for good 🤞

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
triggersActionsUi 1.6MB 1.5MB -23.9KB

Page load bundle

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

id before after diff
triggersActionsUi 104.0KB 104.1KB +82.0B
Unknown metric groups

async chunk count

id before after diff
triggersActionsUi 41 42 +1

History

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

@ogupte ogupte merged commit 3f5473e into elastic:master Mar 2, 2021
ogupte added a commit to ogupte/kibana that referenced this pull request Mar 2, 2021
…tic#85023) (elastic#93098)

* [APM] Fixes duplicate ML job creation for existing environments (elastic#85023)

* Removes commented out test code.

* Adds API integration tests

* clean up code for readability
ogupte added a commit to ogupte/kibana that referenced this pull request Mar 2, 2021
…tic#85023) (elastic#93098)

* [APM] Fixes duplicate ML job creation for existing environments (elastic#85023)

* Removes commented out test code.

* Adds API integration tests

* clean up code for readability
ogupte added a commit that referenced this pull request Mar 2, 2021
…) (#93098) (#93291)

* [APM] Fixes duplicate ML job creation for existing environments (#85023)

* Removes commented out test code.

* Adds API integration tests

* clean up code for readability
ogupte added a commit that referenced this pull request Mar 2, 2021
…) (#93098) (#93290)

* [APM] Fixes duplicate ML job creation for existing environments (#85023)

* Removes commented out test code.

* Adds API integration tests

* clean up code for readability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:APM All issues that need APM UI Team support v7.12.0 v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Investigate: ML job is sometimes created twice (duplicate jobs)
5 participants