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.

Sign up for GitHub

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

Jump to bottom

[APM] Encode spaces when creating ML job #63683

Merged
merged 1 commit into from
Apr 16, 2020
Merged

[APM] Encode spaces when creating ML job #63683

merged 1 commit into from
Apr 16, 2020

Conversation

dgieselaar
Copy link
Member

@dgieselaar dgieselaar commented Apr 16, 2020 •
edited
Loading

@dgieselaar dgieselaar added bug Fixes for quality problems that affect the customer experience release_note:fix v7.8.0 v7.7.1 labels Apr 16, 2020
@dgieselaar dgieselaar requested a review from a team as a code owner April 16, 2020 10:56
@dgieselaar dgieselaar mentioned this pull request Apr 16, 2020
Closed
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

cauemarcondes
cauemarcondes approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@dgieselaar dgieselaar merged commit 5413cee into elastic:master Apr 16, 2020
@dgieselaar dgieselaar deleted the encode-ml-job-id branch April 16, 2020 20:50
@dgieselaar dgieselaar mentioned this pull request Apr 16, 2020
Merged
dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Apr 16, 2020
@dgieselaar
[APM] Encode spaces when creating ML job (elastic#63683)
e36eb04 
Closes elastic#62370. Per https://github.com/elastic/elasticsearch/blob/95a7eed9aa35f47b228e402508709b5bd6703cf4/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/utils/MlStrings.java#L20-L26, spaces are not supported in job and group ids.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 17, 2020
@gmmorris
Merge branch 'master' into alerting/es-query
34e0612 
* master: (40 commits)
  [APM]Upgrade apm-rum agent to latest version to fix full page reload (elastic#63723)
  add deprecation warning for legacy 3rd party plugins (elastic#62401)
  Migrate timelion vis (elastic#62819)
  Replacebad scope link with actual values (elastic#63444)
  Index pattern management UI -> TypeScript and New Platform Ready (create_index_pattern_wizard) (elastic#63111)
  [SIEM] Threat hunting enhancements: Filter for/out value, Show top field, Copy to Clipboard, Draggable chart legends (elastic#61207)
  [Maps] fix term join agg key collision (elastic#63324)
  [Ingest] Fix agent config key sorting (elastic#63488)
  [Monitoring] Fixed server response errors (elastic#63181)
  update elastic charts to 18.3.0 (elastic#63732)
  Start services (elastic#63720)
  [APM] Encode spaces when creating ML job (elastic#63683)
  Uptime 7.7 docs (elastic#62228)
  [DOCS] Updates remote cluster and ccr docs (elastic#63517)
  [Maps] Add 3rd party vector tile support (elastic#62084)
  [Endpoint][EPM] Retrieve Index Pattern from Ingest Manager (elastic#63016)
  [Endpoint] Host Details Policy Response Panel (elastic#63518)
  [Uptime] Certificate expiration threshold settings (elastic#63682)
  Refactor saved object types to use `namespaceType` (elastic#63217)
  [SIEM][CASE] Create comments sequentially (elastic#63692)
  ...
dgieselaar added a commit that referenced this pull request Apr 17, 2020
@dgieselaar
[7.x] [APM] Encode spaces when creating ML job (#63683) (#63769)
24cae43 
Closes #62370. Per https://github.com/elastic/elasticsearch/blob/95a7eed9aa35f47b228e402508709b5bd6703cf4/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/utils/MlStrings.java#L20-L26, spaces are not supported in job and group ids.
sorenlouv
sorenlouv reviewed Apr 27, 2020
View reviewed changes
x-pack/plugins/apm/common/ml_job_constants.ts
@@ -16,3 +16,7 @@ export function getMlJobId(serviceName: string, transactionType?: string) {
export function getMlIndex(serviceName: string, transactionType?: string) {
return `.ml-anomalies-${getMlJobId(serviceName, transactionType)}`;
}

export function encodeForMlApi(value: string) {
return value.replace(/\s+/g, '_').toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

What about using encodeURIComponent? With this change service a and service_a are now the same (although it's very much an edge case).

Btw. A comment would probably be useful later (eg. "ML doesn't allow spaces in group properties")

Copy link
Member

Choose a reason for hiding this comment

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

Btw. are there other characters that we should be aware of that ML doesn't support?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't consider that! But unfortunately % is not supported either. Not sure how we could solve this issue. See https://github.com/elastic/elasticsearch/blob/95a7eed9aa35f47b228e402508709b5bd6703cf4/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/utils/MlStrings.java#L20-L26 for the regex.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay. Not sure how to fix then. Let's leave it for now.

Copy link
Member

@jgowdyelastic jgowdyelastic Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is the function we use to validate an ML job/group ID in the UI.

kibana/x-pack/plugins/ml/common/util/job_utils.js

Line 257 in 0b8cd6e

export function isJobIdValid(jobId) {

Copy link
Member

@sorenlouv sorenlouv Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for chiming in @jgowdyelastic.

We use a slightly more liberal regex: ^[a-zA-Z0-9 _-]+$ (alphanumeric characters, spaces, underscores, and dashes).
https://www.elastic.co/guide/en/apm/get-started/current/agents.html#choose-service-name

// it must also start and end with an alphanumeric character'

Sounds like we might run into problems with service names that start or end with non-alphanumeric character eg. service-a- or _internal_service.

Either we should find a fix on our side, or perhaps you can allow this?
What's the background for this limitation?

Copy link
Member

@jgowdyelastic jgowdyelastic Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There are a few reasons why we don't allow non-alphanumeric chars at the start and end of the ID. They are mainly to do with the ID being used as part of our endpoint URIs.
We follow elasticsearch's naming convention and do now allow - or _ a the start. . is use for "hidden" indices etc.
Some url parsing systems don't like . chars at the end, e.g. slack will not auto add it to a link.
It's reasons like this why we've gone with this fairly strict rule.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. I don't anticipate that this will be common in service names either but currently we do allow it meaning we'll have to handle it somehow.

The easy solution would be to strip non-alphanumeric chars at the beginning and end, but that would create the same problem as I mentioned before, where _service-A and service-A are now identical (very edge casey as well)

@cauemarcondes cauemarcondes self-assigned this May 19, 2020
@cauemarcondes
Copy link
Contributor

Tested:

  • Chrome ✅
  • Safari ✅
  • FF ✅

@cauemarcondes cauemarcondes added the apm:test-plan-done Pull request that was successfully tested during the test plan label May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgowdyelastic jgowdyelastic jgowdyelastic left review comments

@sorenlouv sorenlouv sorenlouv left review comments

@cauemarcondes cauemarcondes cauemarcondes approved these changes

Assignees

@cauemarcondes cauemarcondes

Labels
apm:test-plan-7.8.0 apm:test-plan-done Pull request that was successfully tested during the test plan bug Fixes for quality problems that affect the customer experience release_note:fix v7.7.1 v7.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Cannot create ML jobs for services with spaces in their name
5 participants
@dgieselaar @kibanamachine @cauemarcondes @sorenlouv @jgowdyelastic