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] Transforms: Adds _schedule_now action to transform list. #153545

Merged
merged 13 commits into from
Mar 26, 2023

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Mar 23, 2023

Summary

Fixes #150350.
Fixes #153314.

  • Adds _schedule_now action to transform list.

image

  • Fixes bulk actions to be correctly disabled when not available.

image

Checklist

@walterra walterra self-assigned this Mar 23, 2023
@walterra walterra requested review from qn895 and peteharverson March 23, 2023 14:46
@walterra walterra marked this pull request as ready for review March 23, 2023 14:46
@walterra walterra requested a review from a team as a code owner March 23, 2023 14:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@walterra
Copy link
Contributor Author

I copied the pattern with the confirmation modal from the start transform action, but I wonder if it's necessary here since the transform is running anyway so a warning about cluster load seems unnecessary and the _schedule_now action also doesn't have additional options like the delete model to toggle whether to delete destination indices. Let me know what you think, I tend to remove the modal.

@qn895
Copy link
Member

qn895 commented Mar 23, 2023

Tested latest changes with different Kibana users with and without transform_admin privilege and the actions are disabled/enabled as expected 👍 For wording, I agree that it will be good to discuss whether we should use Schedule now or a more user-friendly term.

On minor suggestion is to change how we are currently handling error message toasts. The toast content can be minimized/simplified, and the full error can be more explanatory. For example (and I know this permission issue has already been fixed), clicking See the full error will yield this modal, which is not meaningful for the general user. In addition, having a shorter timeout might be good as well.

Screen Shot 2023-03-23 at 13 14 30

Screen Shot 2023-03-23 at 11 34 17

@walterra
Copy link
Contributor Author

I agree the error handling and toasts would be good to be improved. However, since this affects all actions on the page and not just _schedule_now, I created a new issue for that to be picked up in a follow up: #153620

/**
* @apiGroup Transforms
*
* @api {post} /api/transform/schedule_now_transforms Schedules transforms now
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can you add an API integration test for this new route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added API integration tests in 3563f14.

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and overall looks good. Just think that some of the text needs tweaking.

@szabosteve any thoughts on the text here?

@@ -286,6 +326,9 @@ export const TransformList: FC<TransformListProps> = ({
{bulkDeleteAction.isModalVisible && <DeleteActionModal {...bulkDeleteAction} />}
{bulkResetAction.isModalVisible && <ResetActionModal {...bulkResetAction} />}
{bulkStopAction.isModalVisible && <StopActionModal {...bulkStopAction} />}
{bulkScheduleNowAction.isModalVisible && (
<ScheduleNowActionModal {...bulkScheduleNowAction} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you that this confirm modal isn't necessary here. It makes sense to show the warning when starting a transform, but since it is already started for this action, it seems unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the modal in f4898da.

if (result.success === true) {
toastNotifications.addSuccess(
i18n.translate('xpack.transform.transformList.scheduleNowTransformSuccessMessage', {
defaultMessage: 'Request to schedule transform {transformId} now acknowledged.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the word now is needed here. Request to schedule transform access_data_by_client acknowledged.

title: i18n.translate(
'xpack.transform.transformList.scheduleNowTransformErrorMessage',
{
defaultMessage: 'An error occurred scheduling the transform {transformId} now.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, I don't think you need the word now in here. It's ambiguous as it is anyway - did the error occur 'now' or did an error occur calling schedule_now?

message = i18n.translate(
'xpack.transform.capability.noPermission.scheduleNowTransformTooltip',
{
defaultMessage: 'You do not have permission to schedule transforms now.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need the word now here. You do not have permission to schedule transforms.

title: i18n.translate(
'xpack.transform.stepCreateForm.scheduleNowTransformResponseSchemaErrorMessage',
{
defaultMessage: 'An error occurred calling the schedule now transforms request.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe An error occurred calling the request to schedule the transform. ?

'xpack.transform.transformList.cannotScheduleNowCompleteBatchTransformBulkActionToolTip',
{
defaultMessage:
'One or more transforms are completed batch transforms and cannot be scheduled now.',
Copy link
Contributor

Choose a reason for hiding this comment

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

One or more transforms are completed batch transforms which cannot be scheduled. perhaps?

completedBatchTransformMessage = i18n.translate(
'xpack.transform.transformList.cannotScheduleNowCompleteBatchTransformToolTip',
{
defaultMessage: '{transformId} is a completed batch transform and cannot be scheduled now.',
Copy link
Contributor

Choose a reason for hiding this comment

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

{transformId} is a completed batch transform and cannot be scheduled. ?

} from '../../../../lib/authorization';
import { TransformListRow, isCompletedBatchTransform } from '../../../../common';

export const scheduleNowActionNameText = i18n.translate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a tooltip to this button to provide clarification as to what this action does as @qn895 suggested?

Something like Instantly run the transform to process data without waiting for the configured interval between checks for changes in the source indices. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a tooltip in a92b1ae.

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

I would lean to explain what happens here instead of using the endpoint name. It's very hard to use the endpoint name unambiguously in the UI text (in fact, any text). It's a bit unwieldy. I left some suggestions. Please let me know what you think.

EDIT: After a discussion with Walter, I updated the suggestions to avoid using the verb run because of the ambiguity with start.

if (result.success === true) {
toastNotifications.addSuccess(
i18n.translate('xpack.transform.transformList.scheduleNowTransformSuccessMessage', {
defaultMessage: 'Request to schedule transform {transformId} now acknowledged.',
Copy link
Contributor

@szabosteve szabosteve Mar 24, 2023

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'Request to schedule transform {transformId} now acknowledged.',
defaultMessage: 'Request to schedule transform {transformId} to process data instantly acknowledged.',

title: i18n.translate(
'xpack.transform.transformList.scheduleNowTransformErrorMessage',
{
defaultMessage: 'An error occurred scheduling the transform {transformId} now.',
Copy link
Contributor

@szabosteve szabosteve Mar 24, 2023

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'An error occurred scheduling the transform {transformId} now.',
defaultMessage: 'An error occurred scheduling transform {transformId} to process data instantly.',

title: i18n.translate(
'xpack.transform.stepCreateForm.scheduleNowTransformResponseSchemaErrorMessage',
{
defaultMessage: 'An error occurred calling the schedule now transforms request.',
Copy link
Contributor

@szabosteve szabosteve Mar 24, 2023

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'An error occurred calling the schedule now transforms request.',
defaultMessage: 'An error occurred calling the request to schedule the transform to process data instantly.',

message = i18n.translate(
'xpack.transform.capability.noPermission.scheduleNowTransformTooltip',
{
defaultMessage: 'You do not have permission to schedule transforms now.',
Copy link
Contributor

@szabosteve szabosteve Mar 24, 2023

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'You do not have permission to schedule transforms now.',
defaultMessage: 'You do not have permission to schedule transforms to process data instantly.',

message = i18n.translate(
'xpack.transform.capability.noPermission.scheduleNowTransformTooltip',
{
defaultMessage: 'You do not have permission to schedule transforms now.',
Copy link
Contributor

@szabosteve szabosteve Mar 24, 2023

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'You do not have permission to schedule transforms now.',
defaultMessage: 'You do not have permission to schedule transforms to process data instantly.',

'xpack.transform.transformList.cannotScheduleNowCompleteBatchTransformBulkActionToolTip',
{
defaultMessage:
'One or more transforms are completed batch transforms and cannot be scheduled now.',
Copy link
Contributor

@szabosteve szabosteve Mar 24, 2023

Choose a reason for hiding this comment

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

Suggested change
'One or more transforms are completed batch transforms and cannot be scheduled now.',
'One or more transforms are completed batch transforms and cannot be scheduled to process data instantly.',

completedBatchTransformMessage = i18n.translate(
'xpack.transform.transformList.cannotScheduleNowCompleteBatchTransformToolTip',
{
defaultMessage: '{transformId} is a completed batch transform and cannot be scheduled now.',
Copy link
Contributor

@szabosteve szabosteve Mar 24, 2023

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: '{transformId} is a completed batch transform and cannot be scheduled now.',
defaultMessage: '{transformId} is a completed batch transform and cannot be scheduled to process data instantly.',

@walterra
Copy link
Contributor Author

@peteharverson @qn895 @szabosteve Addressed all comments, this is ready for another look! Thanks for your feedback esp. about the wording!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
transform 313 317 +4

Async chunks

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

id before after diff
transform 372.9KB 376.6KB +3.7KB
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

cc @walterra

@qn895
Copy link
Member

qn895 commented Mar 24, 2023

Latest changes LGTM 🎉

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

UI text LGTM!

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Latest text changes LGTM

@walterra walterra merged commit 0cc560f into elastic:main Mar 26, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 26, 2023
@walterra walterra deleted the ml-150350-transform-schedule-now branch March 26, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Transforms ML transforms :ml release_note:enhancement v8.8.0
Projects
None yet
7 participants