-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ML] Update cloning for jobs to use exclude_generated #88898
Conversation
Pinging @elastic/ml-ui (:ml) |
jobs(jobIds: string[]) { | ||
const body = JSON.stringify({ jobIds }); | ||
jobs(jobIds: string[], excludeGenerated?: boolean) { | ||
const body = JSON.stringify({ jobIds, excludeGenerated }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as excludeGenerated
could be undefined, it should be optionally added to the body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed here as it's no longer needed
e9ad8f1
const analyticsIdString = analyticsId !== undefined ? `/${analyticsId}` : ''; | ||
return http<GetDataFrameAnalyticsResponse>({ | ||
path: `${basePath()}/data_frame/analytics${analyticsIdString}`, | ||
method: 'GET', | ||
query: { excludeGenerated }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excludeGenerated
shouldn't be added to the query if it is undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed here as it's no longer needed
e9ad8f1
@@ -42,6 +42,7 @@ export const forceStartDatafeedSchema = schema.object({ | |||
export const jobIdsSchema = schema.object({ | |||
/** Optional list of job IDs. */ | |||
jobIds: schema.maybe(schema.arrayOf(schema.maybe(schema.string()))), | |||
excludeGenerated: schema.maybe(schema.boolean()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should also be added to the datafeeds schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this here as it's no longer needed
e9ad8f1
const job = await loadFullJob(jobId); | ||
if (job.custom_settings && job.custom_settings.created_by) { | ||
const [cloneableJob, originalJob] = await Promise.all([ | ||
loadFullJob(jobId, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadFullJob
shouldn't be used here anymore as it adds job and datafeed stats to the job object.
The job and datafeed should be loaded directly from es. Ideally they would also not be combined and we would have a separate datafeed
object in tempJobCloningObjects
. but if that has unexpected side effects we can continue to combine them into a CombinedJob
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated added a new method called loadJobForExport
which will only get the CombinedJob
with datafeed and the job config with exclude_generated: true
. I also tried fetching them separately but encountered some side effects with the wizard behaving unpredictably, but if that's still the preferred method I can revert the change to do that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this to getJobForCloning
which will return them separately with exclude_generated
to always be true.
loadFullJob(jobId, true), | ||
loadFullJob(jobId, false), | ||
]); | ||
if (cloneableJob.custom_settings && originalJob?.custom_settings?.created_by) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if custom_settings
does not exist in cloneableJob
this entire block will be skipped.
To be safe we should only check for originalJob?.custom_settings?.created_by
and then create cloneableJob.custom_settings
if it doesn't exist.
const tempJob = cloneDeep(job); | ||
|
||
// remove all of the items which should not be copied | ||
// such as counts, state and times | ||
delete tempJob.state; | ||
delete tempJob.job_version; | ||
delete tempJob.calendars; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none of these deletes should be needed, this should be solved by not using loadFullJob
when loading the job for cloning.
The same goes for the datafeed
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this cloneJob
utility completely now 9f446c2#diff-bfe918c1ce7b408b4f1552aa1cf94e37bea099c0b11f239abd55e7567bfc5eccR76 as the new getJobForCloning
service no longer includes these.
This reverts commit ebb0174
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and functionality all looks good. Just left some comments on the code.
x-pack/plugins/ml/public/application/jobs/jobs_list/components/utils.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/services/ml_api_service/jobs.ts
Outdated
Show resolved
Hide resolved
* @apiGroup JobService | ||
* | ||
* @api {post} /api/ml/jobs/job_for_cloning Get job for cloning | ||
* @apiName GetJobForCloning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetJobForCloning
needs to be added to ml/server/routes/apidoc.json
too, so that API docs are generated for this new endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here a1fb963
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested latest edits and LGTM. Just need to add the new route to apidoc.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested DFA functionality and code LGTM ⚡
if (Array.isArray(datafeedsResults)) { | ||
if (datafeedsResults.length === 1) { | ||
const datafeed = datafeedsResults[0]; | ||
if (datafeed.job_id === jobId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the job id doesn't match, this should load all of the datafeeds to find one that does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here 6ea5508
// if the job was doesn't use the standard datafeedId format | ||
// get all the datafeeds and match it with the jobId | ||
const { | ||
body: { datafeeds: allDatafeedsResults }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, i think the variable name allDatafeedsResults
is unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here 6ea5508
datafeed = datafeedResults?.datafeeds[0]; | ||
} | ||
|
||
// create jobs objects containing job stats, datafeeds, datafeed stats and calendars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this comment has been copied from somewhere else.
the check below to find the job could be the same as the datafeed check above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here 6ea5508
if (jobResults && jobResults.jobs) { | ||
const job = jobResults.jobs.find((j) => j.job_id === jobId); | ||
if (job && datafeed) { | ||
return { job, datafeed }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a chance that the datafeed for a job can be missing.
I think this should always return an object, but with job
and datafeed
possibly being undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here 6ea5508
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Alerting API Integration Tests.x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/rbac_legacy·ts.alerting api integration security and spaces enabled Alerts legacy alerts alerts superuser at space1 should schedule actions on legacy alertsStandard Out
Stack Trace
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
…167319) When exporting an anomaly detection job, it would be useful if the original `created_by` property was not removed from the job config. Related to #167021 (comment) Related PR #88898
Summary
This PR addresses #79281. It adds a
exclude_generated
flag when fetching data when cloning an Anomaly Detection job or a Data Frame Analytics job.