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

[infra] Shorten IDs for ML jobs #168234

Merged
merged 34 commits into from
Nov 19, 2023
Merged

[infra] Shorten IDs for ML jobs #168234

merged 34 commits into from
Nov 19, 2023

Conversation

miltonhultgren
Copy link
Contributor

@miltonhultgren miltonhultgren commented Oct 6, 2023 •
edited
Loading

Closes #47477

Summary

ML job IDs have a limit of 64 characters. For the log ML jobs we add the string kibana-logs-ui plus the space and log view IDs as a prefix to the job names (log-entry-rate and log-entry-categories-count) which can quickly eat up the 64 character limit (even our own Stack Monitoring log view hits the limit). This prevents users from being able to create ML jobs and it's hard to rename a space or log view, and the limit is not hinted at during space creation (because they are unrelated in some sense).

In order to achieve a more stable length to the ID, this PR introduces a new format for the prefix which creates a UUID v5 which uses the space and log view ID as seed information (it then removes the dashes to still be within the size limit for the categorization job).

Since there is no technical difference between the new and old format, this PR makes an effort to continue to support the old format and allow migration of old jobs as needed. The old jobs work and may contain important data so the user should not feel forced to migrate.

The main addition is a new small API that checks if any ML jobs are available and which format they use for the ID so that the app can request data accordingly and the APIs have been modified to take the ID format into account (except during creation which should always use the new format).

The solution applied is not ideal. It simply passes the ID format along with the space and log view ID to each point where the ID is re-created (which is multiple). The ideal solution would be to store the job data in the store and pass that around instead but that seemed like a considerably larger effort. This PR does introduce some functional tests around the ML job creation process, so such a future refactor should be a bit safer than previously.

How to test

  • Start from main
  • Start Elasticsearch
  • Start Kibana
  • Load the Sample web logs (Kibana home -> Try sample data -> Other sample data sets)
  • Visit the Anomalies page in the Logs UI
  • Set up any of the two ML jobs or both, wait for some results to show up
  • Checkout the PR branch
  • Visit the anomalies page and verify that it still works (requests go to resolve the ID format, should return 'legacy' which should then load the data for the legacy job)
  • Recreate the ML job and verify that the new job works and results still show up (new requests should go out with the new format being used, which may be a mixed mode if you have two jobs and only migrate one of them)

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@miltonhultgren miltonhultgren mentioned this pull request Oct 13, 2023
Open
@miltonhultgren miltonhultgren changed the title [infra] Shorten IDs for ML jobs (#47477) [infra] Shorten IDs for ML jobs Oct 13, 2023
@miltonhultgren miltonhultgren added release_note:fix Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) backport:skip This commit does not require backporting and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 13, 2023
@miltonhultgren miltonhultgren marked this pull request as ready for review October 13, 2023 09:45
@miltonhultgren miltonhultgren requested a review from a team as a code owner October 13, 2023 09:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@gbamparop gbamparop added the Team:obs-ux-logs Observability Logs User Experience Team label Nov 9, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

@miltonhultgren
Copy link
Contributor Author

@crespocarlos Thanks for the code review! I think I've addressed your comments.

I made the change for the null checking for the idFormats but I'm not 100% sure it's needed since the check happening inside the page_providers block all the other code from executing. Though, if that where to change it's good to have those null checks in place so we get a good error message out instead!

crespocarlos
crespocarlos approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks for applying the suggestions. Both job_id and datafeed_id have the new format.

image

It works according to the steps in the description.

@miltonhultgren miltonhultgren enabled auto-merge (squash) November 14, 2023 09:29
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1335 1338 +3

Async chunks

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

id before after diff
infra 1.9MB 1.9MB +2.1KB

Page load bundle

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

id before after diff
infra 102.0KB 103.0KB +960.0B

History

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

tonyghiani
tonyghiani reviewed Nov 19, 2023
View reviewed changes
x-pack/plugins/infra/common/log_analysis/job_parameters.ts

export const bucketSpan = 900000;

export const categoriesMessageField = 'message';

export const partitionField = 'event.dataset';

export const getJobIdPrefix = (spaceId: string, sourceId: string) =>
`kibana-logs-ui-${spaceId}-${sourceId}-`;
const ID_NAMESPACE = 'f91b78c0-fdd3-425d-a4ba-4c028fe57e0f';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Hardcoded IDs in the codebase are a bit of a code smell in the long run, can we just comment on what it represents and why can we keep it hardcoded? Future ourselves will thank us for this 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The auto merge went through but to address your concern, I feel the variable name and it's usage is clear enough?

We need to generate a unique ID for each job so we use uuid.v5 which accepts a UUID to use as the namespace for the hashing and a name unique in that namespace.

tonyghiani
tonyghiani approved these changes Nov 19, 2023
View reviewed changes
Copy link
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

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

LGTM, everything requested change is already addressed and I didn't find any issue, the reproduction works as explained in the description, just left a (non-blocking) nit, thanks for this work!

@miltonhultgren miltonhultgren merged commit 4963e6b into elastic:main Nov 19, 2023
26 checks passed
@miltonhultgren miltonhultgren mentioned this pull request Nov 24, 2023
Open
@Kerry350 Kerry350 mentioned this pull request Apr 10, 2024
Merged
3 tasks
@tonyghiani tonyghiani mentioned this pull request Oct 7, 2024
Merged
tonyghiani added a commit that referenced this pull request Oct 10, 2024
@tonyghiani
[Logs ML] Check permissions before granting access to Logs ML pages (#…
e0e4ec1 
…195278)

## 📓 Summary

Closes #191206 

This work fixes issues while accessing the Logs Anomalies and Logs
Categories pages due to a lack of user privileges.

The privileges were correctly handled until
#168234 was merged, which
introduced a call to retrieve ml formats information higher in the React
hierarchy before the privileges could be asserted for the logged user.
This was resulting in the call failing and letting the user stack in
loading states or erroneous error pages.

These changes lift the license + ML read privileges checks higher in the
hierarchy so we can display the right prompts before calling the ml
formats API, which will resolve correctly if the user has the right
privileges.

### User without valid license

<img width="3008" alt="Screenshot 2024-10-07 at 17 01 17"
src="https://github.com/user-attachments/assets/bf6478ce-b007-4f15-9538-c7959c497e8a">

### User with a valid license (or Trial), but no ML privileges

<img width="3003" alt="Screenshot 2024-10-07 at 17 03 48"
src="https://github.com/user-attachments/assets/c5a82159-b4e8-4f22-9531-23d5e5a9377f">

### User with a valid license (or Trial) and only Read ML privileges

<img width="3003" alt="Screenshot 2024-10-07 at 17 04 21"
src="https://github.com/user-attachments/assets/990f4695-e07e-46a2-9214-d0de3628caf7">

### User with a valid license (or Trial) and All ML privileges, which
are the requirements to work with ML Logs features

<img width="3000" alt="Screenshot 2024-10-07 at 17 04 52"
src="https://github.com/user-attachments/assets/c9b4d832-d3c8-4337-9e17-8a220e7be084">

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 10, 2024
@tonyghiani
[Logs ML] Check permissions before granting access to Logs ML pages (e…
5a190f1 
…lastic#195278)

## 📓 Summary

Closes elastic#191206

This work fixes issues while accessing the Logs Anomalies and Logs
Categories pages due to a lack of user privileges.

The privileges were correctly handled until
elastic#168234 was merged, which
introduced a call to retrieve ml formats information higher in the React
hierarchy before the privileges could be asserted for the logged user.
This was resulting in the call failing and letting the user stack in
loading states or erroneous error pages.

These changes lift the license + ML read privileges checks higher in the
hierarchy so we can display the right prompts before calling the ml
formats API, which will resolve correctly if the user has the right
privileges.

### User without valid license

<img width="3008" alt="Screenshot 2024-10-07 at 17 01 17"
src="https://github.com/user-attachments/assets/bf6478ce-b007-4f15-9538-c7959c497e8a">

### User with a valid license (or Trial), but no ML privileges

<img width="3003" alt="Screenshot 2024-10-07 at 17 03 48"
src="https://github.com/user-attachments/assets/c5a82159-b4e8-4f22-9531-23d5e5a9377f">

### User with a valid license (or Trial) and only Read ML privileges

<img width="3003" alt="Screenshot 2024-10-07 at 17 04 21"
src="https://github.com/user-attachments/assets/990f4695-e07e-46a2-9214-d0de3628caf7">

### User with a valid license (or Trial) and All ML privileges, which
are the requirements to work with ML Logs features

<img width="3000" alt="Screenshot 2024-10-07 at 17 04 52"
src="https://github.com/user-attachments/assets/c9b4d832-d3c8-4337-9e17-8a220e7be084">

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
(cherry picked from commit e0e4ec1)
kibanamachine added a commit that referenced this pull request Oct 10, 2024
@kibanamachine @tonyghiani
[8.x] [Logs ML] Check permissions before granting access to Logs ML p…
c5bc4dd 
…ages (#195278) (#195759)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Logs ML] Check permissions before granting access to Logs ML pages
(#195278)](#195278)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Marco Antonio
Ghiani","email":"marcoantonio.ghiani01@gmail.com"},"sourceCommit":{"committedDate":"2024-10-10T12:33:18Z","message":"[Logs
ML] Check permissions before granting access to Logs ML pages
(#195278)\n\n## 📓 Summary\r\n\r\nCloses #191206 \r\n\r\nThis work fixes
issues while accessing the Logs Anomalies and Logs\r\nCategories pages
due to a lack of user privileges.\r\n\r\nThe privileges were correctly
handled until\r\nhttps://github.com//pull/168234 was
merged, which\r\nintroduced a call to retrieve ml formats information
higher in the React\r\nhierarchy before the privileges could be asserted
for the logged user.\r\nThis was resulting in the call failing and
letting the user stack in\r\nloading states or erroneous error
pages.\r\n\r\nThese changes lift the license + ML read privileges checks
higher in the\r\nhierarchy so we can display the right prompts before
calling the ml\r\nformats API, which will resolve correctly if the user
has the right\r\nprivileges.\r\n\r\n### User without valid
license\r\n\r\n<img width=\"3008\" alt=\"Screenshot 2024-10-07 at 17 01
17\"\r\nsrc=\"https://github.com/user-attachments/assets/bf6478ce-b007-4f15-9538-c7959c497e8a\">\r\n\r\n###
User with a valid license (or Trial), but no ML privileges\r\n\r\n<img
width=\"3003\" alt=\"Screenshot 2024-10-07 at 17 03
48\"\r\nsrc=\"https://github.com/user-attachments/assets/c5a82159-b4e8-4f22-9531-23d5e5a9377f\">\r\n\r\n###
User with a valid license (or Trial) and only Read ML
privileges\r\n\r\n<img width=\"3003\" alt=\"Screenshot 2024-10-07 at 17
04
21\"\r\nsrc=\"https://github.com/user-attachments/assets/990f4695-e07e-46a2-9214-d0de3628caf7\">\r\n\r\n###
User with a valid license (or Trial) and All ML privileges, which\r\nare
the requirements to work with ML Logs features\r\n\r\n<img
width=\"3000\" alt=\"Screenshot 2024-10-07 at 17 04
52\"\r\nsrc=\"https://github.com/user-attachments/assets/c9b4d832-d3c8-4337-9e17-8a220e7be084\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Marco Antonio Ghiani
<marcoantonio.ghiani@elastic.co>","sha":"e0e4ec10e3c329f933bed0a01dbeaecdf79cfa99","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Logs
UI","release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-logs"],"title":"[Logs
ML] Check permissions before granting access to Logs ML
pages","number":195278,"url":"https://github.com/elastic/kibana/pull/195278","mergeCommit":{"message":"[Logs
ML] Check permissions before granting access to Logs ML pages
(#195278)\n\n## 📓 Summary\r\n\r\nCloses #191206 \r\n\r\nThis work fixes
issues while accessing the Logs Anomalies and Logs\r\nCategories pages
due to a lack of user privileges.\r\n\r\nThe privileges were correctly
handled until\r\nhttps://github.com//pull/168234 was
merged, which\r\nintroduced a call to retrieve ml formats information
higher in the React\r\nhierarchy before the privileges could be asserted
for the logged user.\r\nThis was resulting in the call failing and
letting the user stack in\r\nloading states or erroneous error
pages.\r\n\r\nThese changes lift the license + ML read privileges checks
higher in the\r\nhierarchy so we can display the right prompts before
calling the ml\r\nformats API, which will resolve correctly if the user
has the right\r\nprivileges.\r\n\r\n### User without valid
license\r\n\r\n<img width=\"3008\" alt=\"Screenshot 2024-10-07 at 17 01
17\"\r\nsrc=\"https://github.com/user-attachments/assets/bf6478ce-b007-4f15-9538-c7959c497e8a\">\r\n\r\n###
User with a valid license (or Trial), but no ML privileges\r\n\r\n<img
width=\"3003\" alt=\"Screenshot 2024-10-07 at 17 03
48\"\r\nsrc=\"https://github.com/user-attachments/assets/c5a82159-b4e8-4f22-9531-23d5e5a9377f\">\r\n\r\n###
User with a valid license (or Trial) and only Read ML
privileges\r\n\r\n<img width=\"3003\" alt=\"Screenshot 2024-10-07 at 17
04
21\"\r\nsrc=\"https://github.com/user-attachments/assets/990f4695-e07e-46a2-9214-d0de3628caf7\">\r\n\r\n###
User with a valid license (or Trial) and All ML privileges, which\r\nare
the requirements to work with ML Logs features\r\n\r\n<img
width=\"3000\" alt=\"Screenshot 2024-10-07 at 17 04
52\"\r\nsrc=\"https://github.com/user-attachments/assets/c9b4d832-d3c8-4337-9e17-8a220e7be084\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Marco Antonio Ghiani
<marcoantonio.ghiani@elastic.co>","sha":"e0e4ec10e3c329f933bed0a01dbeaecdf79cfa99"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195278","number":195278,"mergeCommit":{"message":"[Logs
ML] Check permissions before granting access to Logs ML pages
(#195278)\n\n## 📓 Summary\r\n\r\nCloses #191206 \r\n\r\nThis work fixes
issues while accessing the Logs Anomalies and Logs\r\nCategories pages
due to a lack of user privileges.\r\n\r\nThe privileges were correctly
handled until\r\nhttps://github.com//pull/168234 was
merged, which\r\nintroduced a call to retrieve ml formats information
higher in the React\r\nhierarchy before the privileges could be asserted
for the logged user.\r\nThis was resulting in the call failing and
letting the user stack in\r\nloading states or erroneous error
pages.\r\n\r\nThese changes lift the license + ML read privileges checks
higher in the\r\nhierarchy so we can display the right prompts before
calling the ml\r\nformats API, which will resolve correctly if the user
has the right\r\nprivileges.\r\n\r\n### User without valid
license\r\n\r\n<img width=\"3008\" alt=\"Screenshot 2024-10-07 at 17 01
17\"\r\nsrc=\"https://github.com/user-attachments/assets/bf6478ce-b007-4f15-9538-c7959c497e8a\">\r\n\r\n###
User with a valid license (or Trial), but no ML privileges\r\n\r\n<img
width=\"3003\" alt=\"Screenshot 2024-10-07 at 17 03
48\"\r\nsrc=\"https://github.com/user-attachments/assets/c5a82159-b4e8-4f22-9531-23d5e5a9377f\">\r\n\r\n###
User with a valid license (or Trial) and only Read ML
privileges\r\n\r\n<img width=\"3003\" alt=\"Screenshot 2024-10-07 at 17
04
21\"\r\nsrc=\"https://github.com/user-attachments/assets/990f4695-e07e-46a2-9214-d0de3628caf7\">\r\n\r\n###
User with a valid license (or Trial) and All ML privileges, which\r\nare
the requirements to work with ML Logs features\r\n\r\n<img
width=\"3000\" alt=\"Screenshot 2024-10-07 at 17 04
52\"\r\nsrc=\"https://github.com/user-attachments/assets/c9b4d832-d3c8-4337-9e17-8a220e7be084\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Marco Antonio Ghiani
<marcoantonio.ghiani@elastic.co>","sha":"e0e4ec10e3c329f933bed0a01dbeaecdf79cfa99"}}]}]
BACKPORT-->

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani01@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonyghiani tonyghiani tonyghiani approved these changes

@crespocarlos crespocarlos crespocarlos approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Logs UI Logs UI feature release_note:fix Team:obs-ux-logs Observability Logs User Experience Team v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Logs UI] Shorten the logs ML job ID prefixes
10 participants
@miltonhultgren @apmmachine @elasticmachine @jgowdyelastic @weltenwort @kibana-ci @crespocarlos @tonyghiani @gbamparop @kibanamachine