-
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
[ML] Space management UI #83320
[ML] Space management UI #83320
Conversation
@elasticmachine merge upstream |
Pinging @elastic/ml-ui (:ml) |
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.
Spaces changes LGTM! I added a follow-up issue for my team to add TSdocs for the SpacesManager that is now exported: #83451
I see that you borrowed a lot of code from the new "Share to space" flyout, so I spent some extra time to play around with this. Hope you don't mind some additional optional feedback on that 🙂
- I noticed that the Machine Learning Jobs management page is scoped to the current space (that is, you can only see jobs that exist in the current space). However, the way this is written, you can remove a job from the current space, which causes it to disappear (even though it still exists), and is a bit of a jarring experience for end-users. In the Saved Objects Management page, we opted to prevent users from deselecting the current space to avoid this situation.
- If you delete a job and then create it again, it still exists in the same space(s) as it did before deletion. I'm not sure if that behavior is intended or not, I just wanted to point that out.
- If a job is shared to a space that the current user doesn't have access to, that information is completely omitted. It looks like this was an intentional design decision, but I wanted to raise it as a comment. My only small concern is that this behavior is inconsistent with that of the Saved Objects Management page and it could be confusing for end users. The reason we originally decided to surface that information to end users is that it helps them make a more informed decision on whether to edit or delete a saved object.
- If you have a job shared to individual spaces, then you share it to "All spaces", then you view the spaces again, you can still see the individual spaces that were previously selected. This is because, when sharing to "All spaces", the client never makes an API call to remove the job from the individual spaces. I could potentially see this becoming a problem if, for example, some spaces are deleted -- the job would still have that space ID in its spaces list forever. It shouldn't cause any real behavioral problems or errors, but I just wanted to point it out. I'd recommend making this small change to avoid space ID cruft =)
If you decide to make any changes based on my feedback, I'm happy to lend a hand and/or have a chat. Just let me know!
<EuiCallOut | ||
color="warning" | ||
iconType="help" | ||
title={i18n.translate('xpack.ml.management.spacesSelectorFlyout.cannotEditCallout.title', { | ||
defaultMessage: 'Insufficient permissions to edit spaces for {jobId}', | ||
values: { jobId }, | ||
})} | ||
> | ||
<FormattedMessage | ||
id="xpack.ml.management.spacesSelectorFlyout.cannotEditCallout.text" | ||
defaultMessage="This job cannot be moved as it is in the * space and you do not have permission to see all spaces. Please log in as a user who had permission to see all spaces to perform this action." | ||
/> | ||
</EuiCallOut> |
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 a couple of thoughts on this callout:
- When implementing the "Share to space" flyout in the Saved Object Management page, I added a tooltip that is displayed when the user runs into this situation. It simply states, "You need additional privileges to change this option." I was working with @gchaps and she pointed out that we should try to put a positive spin on these messages (e.g., avoid "you cannot" and "you don't have permission" types of statements).
- The text as-is is a bit inaccurate -- simply having the Read privilege in all spaces is not sufficient, you need the All privilege.
I'd suggest changing the wording to something like this:
To change this job's spaces, you need privileges to modify jobs in all spaces. Contact your system administrator for more information.
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 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 added a suggestion in #83320 (comment), but I think it's still unclear which privileges are required. If we can be more explicit, e.g. "...you need xxx privileges in yyy spaces..." that would be even better.
))} | ||
</EuiFlexGroup> | ||
); | ||
export const JobSpacesList: FC<Props> = ({ spaceIds, jobId, jobType, refresh }) => { |
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.
listProps={{ | ||
bordered: true, | ||
rowHeight: 40, | ||
className: 'spcCopyToSpace__spacesList', |
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 relies on the SCSS code that is in the Spaces plugin, which is a brittle dependency.
I would recommend renaming this class and making a separate SCSS file for it in this plugin.
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.
thanks for spotting this, it was left in by accident.
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.
fixed in b4300cd
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.
After submitting my review, I thought of another thing I hadn't tested --
I disabled the Spaces plugin and started Kibana, now I can no longer access the Machine Learning Jobs page when logged in as a superuser:
The API calls to /internal/spaces/_active_space
and /api/spaces/space
are still made and both result in a 404 error.
I think this needs some rethinking in terms of its dependency on the Spaces plugin -- some additional code to handle when Spaces is disabled.
@@ -34,7 +34,8 @@ | |||
"kibanaReact", | |||
"dashboard", | |||
"savedObjects", | |||
"home" | |||
"home", | |||
"spaces" |
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 is marked as a required bundle, but the ML plugin lists the Spaces plugin as an optional dependency.
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 is fixed with 21a176c
It now checks whether the spaces
plugin exists.
@jportner Thanks for the review. These are all good points. I'll address each one.
The original plan was to list all jobs in all of the spaces that the user has access to, I was hoping this would be something I could "switch on" in a later PR and so the behaviour of jobs disappearing when moved out of the current space would go away.
It looks like this is an issue caused by a last minute change in a previous PR. Originally a saved object would be completely deleted before a new one with the same ID is created.
Could you point me in the direction of where I can see an example of this?
I considered this a feature :)
Thanks! I'll take you up on that. |
Ah, OK, good to know! It's definitely possible. In fact, I just participated in a discussion with @elastic/kibana-platform with regards to making the Saved Objects Management page show objects in all spaces. Based on that discussion, it sounds like we are leaning towards making that change, but I'm not sure when that will happen. If you want to change the ML Jobs page to behave like this, you should be able to do that by searching across all spaces. I just checked and apparently our public documentation doesn't mention this (whoops!) but you can do this by simply adding the However, keep in mind that other SavedObjectsClient APIs are scoped to the current namespace -- so if you try to, for instance, delete an object that doesn't exist in the current space, you'll get a Not Found error. We'll probably need to put some more thought into this.
Sounds good. You can keep your overwrite behavior, and override the newly-recreated job's namespaces by using the
So I set up some sample data locally --
Example when viewing the object as a superuser. I'm happy to walk you through this offline if you want to dive in a bit more.
I don't have a problem with it if that's intentional! |
tooltip: isGlobalControlChecked | ||
? i18n.translate( | ||
'xpack.ml.management.spacesSelectorFlyout.shareToAllSpaces.cannotUncheckTooltip', | ||
{ defaultMessage: 'You need additional privileges to change this option.' } |
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.
Agreed with keeping this simple. To link to the docs, you can use:
You need additional privileges to change this option. Learn more.
const { successCount, errorCount } = getResponseCounts(resp); | ||
if (errorCount > 0) { | ||
const title = i18n.translate('xpack.ml.management.repairSavedObjectsFlyout.repair.error', { | ||
defaultMessage: 'Some jobs could not be repaired', |
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 this is a complete sentence, I'd add an ending period. Also, can it be:
Some jobs cannot be repaired.
@jportner do you know if this is possible? |
x-pack/plugins/ml/public/application/components/job_spaces_repair/job_spaces_repair_flyout.tsx
Show resolved
Hide resolved
await loadRepairList(true); | ||
|
||
if (resp === null) { | ||
return; |
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.
When would the response be null? If it is null - do we need to show the user some sort of message for clarity on what's happening before returning?
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'll be null
if the first call to loadRepairList
fails for an unknown reason. In this situation an error toast is triggered from inside loadRepairList
x-pack/plugins/ml/public/application/components/job_spaces_repair/repair_results.tsx
Outdated
Show resolved
Hide resolved
After you retrieve all of the spaces, you can iterate through each space's
|
perfect answer! thank you |
Great addition! 🎉 Code LGTM - giving it a test 👌 |
@elasticmachine merge upstream |
<h2> | ||
<FormattedMessage | ||
id="xpack.ml.management.repairSavedObjectsFlyout.headerLabel" | ||
defaultMessage="Repair saved objects" |
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.
Did you consider any alternative terms to 'Repair' for this functionality, such as 'Sync'? The word 'repair' could imply something is broken, which is not the case; it's just a case of needing to sync up the saved objects and jobs.
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 button could be renamed.
I think this would be more of a product decision, I'm happy to change the text if needed.
I think the repair
endpoint could remain the same as it is not user facing. But if we decided that it needs to change then we should also change all relevant functions and files to remove the word "repair"
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.
My vote is to rename this to 'Sync' or 'Synchronize', and if we change the label on the button we should also change the endpoint to match in case it becomes more widely used by Kibana API users. This can all be done in a follow-up.
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
LGTM ⚡
* master: skip "Dashboards linked by a drilldown are both copied to a space" (elastic#83824) [alerts] adds action group and date to mustache template variables for actions (elastic#83195) skip flaky suite (elastic#79389) [DOCS] Reallocates limitations to point-of-use (elastic#79582) [Enterprise Search] Engine overview layout stub (elastic#83756) Disable exporting/importing of templates. Optimize pitch images a bit (elastic#83098) [DOCS] Consolidates plugins (elastic#83712) [ML] Space management UI (elastic#83320) test just part of the message to avoid updates (elastic#83703) [Data Table] Remove extra column in split mode (elastic#83193) Improve snapshot error messages (elastic#83785) skip flaky suite (elastic#83773) skip flaky suite (elastic#83771) skip flaky suite (elastic#65278) skip flaky suite (elastic#83793) [Task Manager] Ensures retries are inferred from the schedule of recurring tasks (elastic#83682) [index patterns] improve index pattern cache (elastic#83368) [Fleet] Rename ingestManager plugin ID fleet (elastic#83200) fixed pagination in connectors list (elastic#83638)
* [ML] Space management UI * fixing types * small react refactor * adding repair toasts * text and style changes * handling spaces being disabled * correcting initalizing endpoint response * text updates * text updates * fixing spaces manager use when spaces is disabled * more text updates * switching to delete saved object first rather than overwrite * filtering non ml spaces * renaming file * fixing types * updating list style Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [ML] Space management UI * fixing types * small react refactor * adding repair toasts * text and style changes * handling spaces being disabled * correcting initalizing endpoint response * text updates * text updates * fixing spaces manager use when spaces is disabled * more text updates * switching to delete saved object first rather than overwrite * filtering non ml spaces * renaming file * fixing types * updating list style Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Adds UI controls to ML's management app plugin to allow the user to move jobs between spaces and to repair job saved objects.
Assigning spaces to jobs
The user can now click on the spaces for a job and assign/unassign the job from every space they has access to or to the
*
space.If a user does not have access to all space, they will not be able to assign to the
*
space. Additionally, if the job is already in the*
space, that user will not be able to unassign it from the*
space.Only jobs in the current space are listed. Moving a job out of this space will cause it to disappear from the list.
Repair job saved objects
The UI wraps the repair endpoint. This will:
Checklist
Delete any items that are not applicable to this PR.