-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Adds 'Anomaly detection' settings page to create ML jobs per environment #70560
[APM] Adds 'Anomaly detection' settings page to create ML jobs per environment #70560
Conversation
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
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.
Just a quick review for the copy mainly as I had updated it yesterday.
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/add_environments.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/add_environments.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/add_environments.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/jobs_list.tsx
Outdated
Show resolved
Hide resolved
@bmorelli25 Can you give the general copy a quick look and give your suggestion for improvements. This was written very quickly by yours truly, but I feel like it needs professional help 😊 |
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
2ce5235
to
0c17927
Compare
to list and create the apm anomaly detection jobs per environment. Some test data is hardcoded while the the required changes in the ML plugin are in flight.
in groups array. Also adds random token to the job ID to prevent collisions for job ids where diffferent environment names convert to the same string
…ML module - Implements job list in settings by reading from `custom_settings.job_tags['service.environment']` - Add ML module method `createModuleItem` for job configuration - Don't allow user to type in duplicate environments
…ction/add_environments.tsx Co-authored-by: Casper Hübertz <[email protected]>
…ction/index.tsx Co-authored-by: Casper Hübertz <[email protected]>
- makes the 'all' environment name ALL_OPTION_VALUE agent configuration-specific - replace field literals with constants
417d01d
to
cb9a8ad
Compare
Pinging @elastic/apm-ui (Team:apm) |
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
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 really good. Just a bunch of nits
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Show resolved
Hide resolved
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/lib/anomaly_detection/get_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/jobs_list.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
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.
A few text suggestions
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/jobs_list.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/add_environments.tsx
Outdated
Show resolved
Hide resolved
… 'Not defined' option is disabled if it already exists.
x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/add_environments.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Outdated
Show resolved
Hide resolved
documents without service.environment set
@elasticmachine merge upstream |
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
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.
ML changes LGTM
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* master: Rename legacy ES mock accessors (elastic#70432) [APM] Adds 'Anomaly detection' settings page to create ML jobs per environment (elastic#70560) Forbid timezones not working in Elasticsearch (elastic#70780)
…rbac * alerting/consumer-based-rbac: Rename legacy ES mock accessors (elastic#70432) [APM] Adds 'Anomaly detection' settings page to create ML jobs per environment (elastic#70560) Forbid timezones not working in Elasticsearch (elastic#70780) [ML] Adding peak_model_bytes to model size stats type (elastic#70825)
…vironment (elastic#70560) * Adds 'Anomaly detection' settings page along with require API endpoints to list and create the apm anomaly detection jobs per environment. Some test data is hardcoded while the the required changes in the ML plugin are in flight. * Converts the environment name to a compatible ML id string and persist in groups array. Also adds random token to the job ID to prevent collisions for job ids where diffferent environment names convert to the same string * - Improve job creation with latest updates for the `apm_transaction` ML module - Implements job list in settings by reading from `custom_settings.job_tags['service.environment']` - Add ML module method `createModuleItem` for job configuration - Don't allow user to type in duplicate environments * Update x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/add_environments.tsx Co-authored-by: Casper Hübertz <[email protected]> * Update x-pack/plugins/apm/public/components/app/Settings/anomaly_detection/index.tsx Co-authored-by: Casper Hübertz <[email protected]> * UX feedback, adds i18n, and handles failed state for ML jobs fetch. * - Moves get_all_environments from agent_configuration dir to common dir - makes the 'all' environment name ALL_OPTION_VALUE agent configuration-specific - replace field literals with constants * PR feedback * Adds support to create jobs for environment which are not defined. * Fixes description copy, rearranges settings links, and makes sure the 'Not defined' option is disabled if it already exists. * Only show "Not defined" in environment selector if there are actually documents without service.environment set * get the indexPatternName for the ML job from the set of user-definned indices * updated job_tags type definition Co-authored-by: Casper Hübertz <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
}; | ||
|
||
export function convertToMLIdentifier(value: string) { | ||
return value.replace(/\s+/g, '_').toLowerCase(); |
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.
@ogupte we should improve this
// remove non-alphanumeric characters
retur value.toLowerCase().replace(/[^a-z0-9+]+/g, '_');
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 simplified this in another PR:
kibana/x-pack/plugins/apm/server/lib/anomaly_detection/create_anomaly_detection_jobs.ts
Line 86 in 55b1e91
prefix: `${ML_GROUP_NAME_APM}-${snakeCase(environment)}-${randomToken}-`, |
snakeCase
. This also has the effect of deburring and removing all invalid chars
return []; | ||
} | ||
try { | ||
const { jobs } = await ml.anomalyDetectors.jobs(ML_GROUP_NAME_APM); |
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.
It just occurs to me that we are retrieving jobs without specifying that we are filtering by groups
. Will this return for any field that matches ML_GROUP_NAME_APM
?
I'd expect something like:
const { jobs } = await ml.anomalyDetectors.jobs({group: ML_GROUP_NAME_APM });
Similarly, if we want to retrieve jobs by job id:
const { jobs } = await ml.anomalyDetectors.job({ id: 'some-id' });
Use "apm-*" to match the new job IDs added in elastic#70560.
* [APM] Update ML job ID in data telemetry tasks Use "apm-*" to match the new job IDs added in #70560. * additional fix * Remove unused import
* [APM] Update ML job ID in data telemetry tasks Use "apm-*" to match the new job IDs added in elastic#70560. * additional fix * Remove unused import
Closes #70422.
Adds 'Anomaly detection' settings page along with required API endpoints to list and create the APM anomaly detection jobs per environment.