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.

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

Merged
merged 22 commits into from
Nov 19, 2020
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions x-pack/plugins/ml/common/types/saved_objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,23 @@
export type JobType = 'anomaly-detector' | 'data-frame-analytics';
export const ML_SAVED_OBJECT_TYPE = 'ml-job';

type Result = Record<string, { success: boolean; error?: any }>;
export interface SavedObjectResult {
[jobId: string]: { success: boolean; error?: any };
}

export interface RepairSavedObjectResponse {
savedObjectsCreated: Result;
savedObjectsDeleted: Result;
datafeedsAdded: Result;
datafeedsRemoved: Result;
savedObjectsCreated: SavedObjectResult;
savedObjectsDeleted: SavedObjectResult;
datafeedsAdded: SavedObjectResult;
datafeedsRemoved: SavedObjectResult;
}

export type JobsSpacesResponse = {
[jobType in JobType]: { [jobId: string]: string[] };
};

export interface InitializeSavedObjectResponse {
jobs: Array<{ id: string; type: string }>;
success: boolean;
error?: any;
}
3 changes: 2 additions & 1 deletion x-pack/plugins/ml/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@
"kibanaReact",
"dashboard",
"savedObjects",
"home"
"home",
"spaces"
Copy link
Contributor

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.

Copy link
Member Author

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.

],
"extraPublicDirs": [
"common"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
* you may not use this file except in compliance with the Elastic License.
*/

export { JobSpacesList } from './job_spaces_list';
export { JobSpacesList, ALL_SPACES_ID } from './job_spaces_list';
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,64 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { FC } from 'react';
import React, { FC, useState, useEffect } from 'react';

import { EuiFlexGroup, EuiFlexItem, EuiBadge } from '@elastic/eui';
import { EuiFlexGroup, EuiFlexItem, EuiButtonEmpty } from '@elastic/eui';
import { JobSpacesFlyout } from '../job_spaces_selector';
import { JobType } from '../../../../common/types/saved_objects';
import { useSpacesContext } from '../../contexts/spaces';
import { Space, SpaceAvatar } from '../../../../../spaces/public';

export const ALL_SPACES_ID = '*';

interface Props {
spaces: string[];
spaceIds: string[];
jobId: string;
jobType: JobType;
refresh(): void;
}

function filterUnknownSpaces(ids: string[]) {
return ids.filter((id) => id !== '?');
}

export const JobSpacesList: FC<Props> = ({ spaces }) => (
<EuiFlexGroup wrap responsive={false} gutterSize="xs">
{spaces.map((space) => (
<EuiFlexItem grow={false} key={space}>
<EuiBadge color={'hollow'}>{space}</EuiBadge>
</EuiFlexItem>
))}
</EuiFlexGroup>
);
export const JobSpacesList: FC<Props> = ({ spaceIds, jobId, jobType, refresh }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you share a job to a lot of other spaces, it doesn't render too well:

(Screenshot)

image

It may be worth implementing a show more / show less link like we use in the Saved Object Management page:

(Screenshot)

image

const { allSpaces } = useSpacesContext();

const [showFlyout, setShowFlyout] = useState(false);
const [spaces, setSpaces] = useState<Space[]>([]);

useEffect(() => {
const tempSpaces = spaceIds.includes(ALL_SPACES_ID)
? [{ id: ALL_SPACES_ID, name: ALL_SPACES_ID, disabledFeatures: [], color: '#DDD' }]
: allSpaces.filter((s) => spaceIds.includes(s.id));
setSpaces(tempSpaces);
}, [spaceIds, allSpaces]);

function onClose() {
setShowFlyout(false);
refresh();
}

return (
<>
<EuiButtonEmpty onClick={() => setShowFlyout(true)} style={{ height: 'auto' }}>
<EuiFlexGroup wrap responsive={false} gutterSize="xs">
{spaces.map((space) => (
<EuiFlexItem grow={false} key={space.id}>
<SpaceAvatar space={space} size={'s'} />
Copy link
Contributor

Choose a reason for hiding this comment

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

SpaceAvatar by default renders as aria-hidden=true so this EuiButtonEmpty has no inner content. Looks like if you pass in announceSpaceName={true} it will appropriately wire up the aria-label for each space.

Someone more familiar with this component should probably evaluate if this default (i.e., SpaceAvatar components being completely hidden to screen readers) is a smart one across the board.

Copy link
Member

Choose a reason for hiding this comment

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

In most cases, the space avatar is rendered alongside a text description of the space, so we opted to make it "hidden" by default to avoid announcing it twice: #27748

Copy link
Contributor

Choose a reason for hiding this comment

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

But if people don't know that SpaceAvatar is a purely visual component, this is the way more destructive default (i.e., it's better to double announce something by accident than not announce anything at all)

Copy link
Member

Choose a reason for hiding this comment

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

That's a fair point. Up until now, the security team have been the only consumers of this component, and its original design didn't expect broad usage at the time. It probably makes sense to revisit this decision now that it's being used in more places by more teams, who don't know of its history

</EuiFlexItem>
))}
</EuiFlexGroup>
</EuiButtonEmpty>
{showFlyout && (
<JobSpacesFlyout
jobId={jobId}
spaceIds={filterUnknownSpaces(spaceIds)}
jobType={jobType}
onClose={onClose}
/>
)}
</>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export { JobSpacesRepairFlyout } from './job_spaces_repair_flyout';
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React, { FC, useState, useEffect } from 'react';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import {
EuiFlyout,
EuiFlyoutHeader,
EuiFlyoutFooter,
EuiFlexGroup,
EuiFlexItem,
EuiButton,
EuiButtonEmpty,
EuiTitle,
EuiFlyoutBody,
EuiText,
EuiCallOut,
EuiSpacer,
} from '@elastic/eui';

import { ml } from '../../services/ml_api_service';
import {
RepairSavedObjectResponse,
SavedObjectResult,
} from '../../../../common/types/saved_objects';
import { RepairList } from './repair_results';
import { useToastNotificationService } from '../../services/toast_notification_service';

interface Props {
onClose: () => void;
}
export const JobSpacesRepairFlyout: FC<Props> = ({ onClose }) => {
const { displayErrorToast, displaySuccessToast } = useToastNotificationService();
const [loading, setLoading] = useState(false);
const [repairable, setRepairable] = useState(false);
const [repairResp, setRepairResp] = useState<RepairSavedObjectResponse | null>(null);

async function loadRepairList(simulate: boolean = true) {
setLoading(true);
try {
const resp = await ml.savedObjects.repairSavedObjects(simulate);
setRepairResp(resp);

const count = Object.values(resp).reduce((acc, cur) => acc + Object.keys(cur).length, 0);
setRepairable(count > 0);
setLoading(false);
return resp;
} catch (error) {
// this shouldn't be hit as errors are returned per-repair task
// as part of the response
alvarezmelissa87 marked this conversation as resolved.
Show resolved Hide resolved
displayErrorToast(error);
}
return null;
}

useEffect(() => {
loadRepairList();
}, []);

async function repair() {
if (repairable) {
// perform the repair
const resp = await loadRepairList(false);
// check simulate the repair again to check that all
// items have been repaired.
await loadRepairList(true);

if (resp === null) {
return;
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 Nov 17, 2020

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?

Copy link
Member Author

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

}
const { successCount, errorCount } = getResponseCounts(resp);
if (errorCount > 0) {
const title = i18n.translate('xpack.ml.management.repairSavedObjectsFlyout.repair.error', {
defaultMessage: 'Some jobs cannot be repaired.',
});
displayErrorToast(resp as any, title);
return;
}

displaySuccessToast(
i18n.translate('xpack.ml.management.repairSavedObjectsFlyout.repair.success', {
defaultMessage: '{successCount} {successCount, plural, one {job} other {jobs}} repaired',
values: { successCount },
})
);
}
}

return (
<>
<EuiFlyout maxWidth={600} onClose={onClose}>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h2>
<FormattedMessage
id="xpack.ml.management.repairSavedObjectsFlyout.headerLabel"
defaultMessage="Repair saved objects"
Copy link
Contributor

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.

Copy link
Member Author

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"

Copy link
Contributor

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.

/>
</h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
<EuiCallOut color="primary">
<EuiText size="s">
<FormattedMessage
id="xpack.ml.management.repairSavedObjectsFlyout.description"
defaultMessage="Repair the saved objects if they are out of sync with the machine learning jobs in Elasticsearch."
/>
</EuiText>
</EuiCallOut>
<EuiSpacer />
<RepairList repairItems={repairResp} />
</EuiFlyoutBody>
<EuiFlyoutFooter>
<EuiFlexGroup justifyContent="spaceBetween">
<EuiFlexItem grow={false}>
<EuiButtonEmpty iconType="cross" onClick={onClose} flush="left">
<FormattedMessage
id="xpack.ml.management.repairSavedObjectsFlyout.closeButton"
defaultMessage="Close"
/>
</EuiButtonEmpty>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButton
onClick={repair}
fill
isDisabled={repairable === false || loading === true}
>
<FormattedMessage
id="xpack.ml.management.repairSavedObjectsFlyout.repairButton"
defaultMessage="Repair"
/>
</EuiButton>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlyoutFooter>
</EuiFlyout>
</>
);
};

function getResponseCounts(resp: RepairSavedObjectResponse) {
let successCount = 0;
let errorCount = 0;
Object.values(resp).forEach((result: SavedObjectResult) => {
Object.values(result).forEach(({ success, error }) => {
if (success === true) {
successCount++;
} else if (error !== undefined) {
errorCount++;
}
});
});
return { successCount, errorCount };
}
Loading