Fix Watcher stuck firing state #138563

cjcenizal · 2022-08-10T20:04:03Z

Fixes:

Follow-up to #138026

Release note

Watches will no longer get stuck in a "Firing" state in Watcher.

Summary

I simplified the possible action and watch states, updated the UI accordingly, added more info to the UI in the form of tooltips to help users make sense of what they're seeing, and updated the tests. By removing the concept of a "firing" state from the UI, watches can now longer get "stuck" in that state and cause confusion among users. By adding more information about watch and action state, users will be able to form a clearer understanding of their behavior.

Data changes

ACTION_STATES and WATCH_STATES have been simplified.

WATCH_STATES:

~~FIRING~~ ~~OK~~ -> ACTIVE
~~DISABLED~~ -> INACTIVE
ERROR
CONFIG_ERROR

ACTION_STATES

~~FIRING~~
OK
ACKNOWLEDGED
THROTTLED
ERROR
CONFIG_ERROR
UNKNOWN

Regarding config error: I understand that we don't expect ES to permit invalid watch configurations, but I decided to retain CONFIG_ERROR for now since it seems outside of the scope of the specific problem this PR fixes. See #23887 for more background on this state.

Regarding isAckable: isAckable is now a direct translation of the action model's underlying ES value, instead of derived from its state. I believe this is a truer reflection of how ES treats ackability (docs).

UI changes

Watch list

I added columns for "Condition last met" and "Last checked", and added tooltips to most of the columns.

Execution history panel

I retained the "State" column because it could contain useful information, for example if it was in an error state. I added a "Condition met" column and added tooltips to most of the columns.

Action statuses panel

I added a "Last executed" column and added tooltips to most of the columns.

Simulate watch results flyout

I didn't make any changes to this flyout beyond surfacing the new action states, because it seems beyond the scope of the problem this PR addresses.

Steps to test

Create a watch locally with this request and explore the UI.

PUT _watcher/watch/test
{
  "trigger": {
    "schedule": {
      "interval": "1m"
    }
  },
  "actions" : {
    "log" : { 
      "logging" : {
        "text" : "executed at {{ctx.execution_time}}" 
      }
    }
  }
}

cjcenizal · 2022-08-11T18:34:04Z

x-pack/plugins/watcher/__fixtures__/get_watch_history.ts

I just renamed this file to align with the exported function name and reduced the function's params.

cjcenizal · 2022-08-11T18:34:48Z

x-pack/plugins/watcher/common/types/status_types.ts

I changed this name to remove all references to "firing".

cjcenizal · 2022-08-11T18:35:11Z

x-pack/plugins/watcher/public/application/models/watch_status/watch_status.js

I removed these properties because it didn't look like they were being used anywhere.

cjcenizal · 2022-08-11T18:35:37Z

...lic/application/sections/watch_edit_page/components/json_watch_edit/json_watch_edit_form.tsx

This fixed a React error that surfaced when executing the Jest tests.

cjcenizal · 2022-08-11T18:37:23Z

x-pack/plugins/watcher/server/models/action_status_model/action_status_model.ts

This change means that we no longer needed the tests about the relationship between isAckable and state, above, because there is no longer any relationship.

… with ACTIVE. Update ACTION_STATES by replacing FIRING with OK. - Refactor isAckable logic to directly translate value provided by ES. - Replace WatchStatus component with ActionStateBadge and WatchStateBadge components.

…'Last triggered' column to 'Last checked'. - Add tooltips to both. - Add tooltips to State and Comment headers.

…y. Remove unused properties from client WatchStatus model.

… Execution History Panel.

…cuted header to Action Statuses Panel and tooltips. Update tests.

elasticmachine · 2022-08-11T19:20:22Z

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

alisonelizabeth

Nice work @cjcenizal! So awesome to see this addressed 🎉

Everything worked as expected. I think I tested all the different states, except error (I wasn't sure the best way to trigger an error).

Two questions -

Do you think it would be helpful to surface whether there is an action that is "ackable" in the "Comments" column of the watch list table? It seems like it could be and is supplemental to the existing throttled/acknowledged comments.
Were you planning on updating the Watcher docs? Let me know if you need help here.

alisonelizabeth · 2022-08-15T19:17:19Z

x-pack/plugins/watcher/public/application/components/watch_state_badge.tsx

+
+interface Props {
+  state: string;
+  size?: 'xs' | 's' | 'm';


Can you use EuiTextProps['size']?

Nice! Will do.

alisonelizabeth · 2022-08-15T19:17:59Z

x-pack/plugins/watcher/public/application/components/watch_state_badge.tsx

+}
+
+const stateToIconMap = {
+  [WATCH_STATES.ACTIVE]: <EuiIcon type="check" color="success" />,


What do you think about changing the Active icon to a green dot? Maybe not a big deal, but we're also using the check icon for the Condition met column on the history page, so sometimes it's displayed for both columns.

Great idea, will do.

…ING..

gchaps

Tooltip text should end with a period.

gchaps · 2022-08-15T20:22:15Z

x-pack/plugins/watcher/public/application/sections/watch_list_page/watch_list_page.tsx

+            content={i18n.translate(
+              'xpack.watcher.sections.watchList.watchTable.lastFiredHeader.tooltipText',
+              {
+                defaultMessage: `The most recent time this watch's condition was met and its actions were executed`,


I know its repetitive, but IMO "last time" is easier to understand than "most recent time"

The last time the condition was met and action taken.

gchaps · 2022-08-15T20:38:54Z

x-pack/plugins/watcher/public/application/sections/watch_list_page/watch_list_page.tsx

+              'xpack.watcher.sections.watchList.watchTable.commentHeader.tooltipText',
+              {
+                defaultMessage:
+                  'Information about whether the watch is throttled, acknowledged, or has failed to execute',


Is it the watch that is throttled, or is it the action?

Whether the watch was acknowledged, throttled, or failed to execute.

or something like:

The state of the watch--throttled, acknowledged, or failed to execute.

Good question -- it's whether any of the watch's actions have been acknowledged, throttled, or failed to execute. So how about:

Whether any actions have been acknowledged, throttled, or failed to execute.

I suggest removing "any". On second look, can this be:

Whether actions are acknowledged, throttled, or failed.

gchaps · 2022-08-15T20:39:54Z

...s/watcher/public/application/sections/watch_status_page/components/action_statuses_panel.tsx

+          content={i18n.translate(
+            'xpack.watcher.sections.watchDetail.watchTable.stateHeader.tooltipText',
+            {
+              defaultMessage: 'OK, acked, throttled, or error state',


Suggested change

defaultMessage: 'OK, acked, throttled, or error state',

defaultMessage: 'OK, acknowledged, throttled, or error',

gchaps · 2022-08-15T20:41:38Z

...s/watcher/public/application/sections/watch_status_page/components/action_statuses_panel.tsx

+          content={i18n.translate(
+            'xpack.watcher.sections.watchHistory.watchActionStatusTable.lastExecuted.tooltipText',
+            {
+              defaultMessage: `When this action was last executed`,


Suggested change

defaultMessage: `When this action was last executed`,

defaultMessage: `The last time this action was executed`,

*executed could also be "taken"

gchaps · 2022-08-15T20:43:04Z

...watcher/public/application/sections/watch_status_page/components/execution_history_panel.tsx

+          content={i18n.translate(
+            'xpack.watcher.sections.watchHistory.watchTable.metConditionHeader.tooltipText',
+            {
+              defaultMessage: `Whether the watch's condition was met and its actions were executed`,


Whether the condition was met and action taken.

gchaps · 2022-08-15T20:51:13Z

x-pack/plugins/watcher/public/application/sections/watch_list_page/watch_list_page.tsx

+            content={i18n.translate(
+              'xpack.watcher.sections.watchList.watchTable.stateHeader.tooltipText',
+              {
+                defaultMessage: 'Active, inactive, or error state',


Since the title of the column in state, can you remove "state" from the tooltip?

Active, inactive, or error

gchaps · 2022-08-15T20:57:28Z

x-pack/plugins/watcher/public/application/sections/watch_list_page/watch_list_page.tsx

+            content={i18n.translate(
+              'xpack.watcher.sections.watchList.watchTable.lastTriggeredHeader.tooltipText',
+              {
+                defaultMessage: `The most recent time this watch's condition was checked`,


The last time the condition was checked.

gchaps · 2022-08-15T21:05:58Z

...watcher/public/application/sections/watch_status_page/components/execution_history_panel.tsx

+          content={i18n.translate(
+            'xpack.watcher.sections.watchHistory.watchTable.stateHeader.tooltipText',
+            {
+              defaultMessage: 'Active or error state',


Active or error

Should this also include "inactive"?

I don't believe so, because this part of the UI shows a history of attempted executed actions, and if it's inactive then it won't execute.

gchaps · 2022-08-15T21:08:18Z

...watcher/public/application/sections/watch_status_page/components/execution_history_panel.tsx

+            'xpack.watcher.sections.watchHistory.watchTable.commentHeader.tooltipText',
+            {
+              defaultMessage:
+                'Information about whether the watch was throttled, acknowledged, or failed to execute',


Suggested change

'Information about whether the watch was throttled, acknowledged, or failed to execute',

'Whether the action was throttled, acknowledged, or failed to execute',

gchaps · 2022-08-15T21:26:19Z

...watcher/public/application/sections/watch_status_page/components/execution_history_panel.tsx

+              content={i18n.translate(
+                'xpack.watcher.sections.watchHistory.watchActionStatusTable.state.tooltipText',
+                {
+                  defaultMessage: 'OK, acked, throttled, or error state',


Suggested change

defaultMessage: 'OK, acked, throttled, or error state',

defaultMessage: 'OK, acknowledged, throttled, or error',

…S.IS_ACKABLE state.

cjcenizal · 2022-08-15T22:00:15Z

@alisonelizabeth @gchaps Thank you for your feedback! I've implemented all of your suggestions, except the docs update. I'd like to do that in a separate PR after this one has been approved and merged. Could you please take another look at these changes?

gchaps · 2022-08-15T22:33:23Z

x-pack/plugins/watcher/public/application/sections/watch_list_page/watch_list_page.tsx

+              'xpack.watcher.sections.watchList.watchTable.commentHeader.tooltipText',
+              {
+                defaultMessage:
+                  'Whether any actions are acknowledgeable or have been acknowledged, throttled, or failed to execute.',


Can this be the same as the one below?

Whether actions are acknowledged, acknowledgeable, throttled, or failed.

gchaps

A few questions, but otherwise LGTM

gchaps · 2022-08-15T22:35:38Z

...watcher/public/application/sections/watch_status_page/components/execution_history_panel.tsx

+            'xpack.watcher.sections.watchHistory.watchTable.commentHeader.tooltipText',
+            {
+              defaultMessage:
+                'Whether the action was throttled, acknowledged, acknowledgeable, or failed to execute.',


Is this the right order, or should it go from positive to negative: acknowledged, acknowledgeable, throttled, or failed? Should actions be singular or plural as the comment above?

Whether the action was throttled, acknowledged, acknowledgeable, or failed.',

Can we come up with another word for acknowledgeable?

For context, some actions can be "Acknowledged". I made up the term "Acknowledgeable" to describe this. Another term known to our users is "Ackable".

I have no preference about the order, and I don't have a feeling that the order will affect the meaning extracted by the reader.

What do you mean by "Should actions be singular or plural as the comment above"? I can't find the referenced comment.

gchaps · 2022-08-15T22:45:13Z

...watcher/public/application/sections/watch_status_page/components/execution_history_panel.tsx

+              content={i18n.translate(
+                'xpack.watcher.sections.watchHistory.watchActionStatusTable.state.tooltipText',
+                {
+                  defaultMessage: 'OK, acknowledged, throttled, or error.',


How does this related to acknowledge, acknowledgeable, throttled, and failed. Should these two messages be using the same words?

cjcenizal · 2022-08-15T23:31:45Z

@gchaps @alisonelizabeth I reverted the addition of the "Acknowledgeable" state, since AFAIK this hasn't been requested by users and it's raising additional questions in my mind about its value to the user. We can always add it back in a separate PR.

alisonelizabeth

Latest LGTM

kibana-ci · 2022-08-16T01:13:57Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`watcher`	181	182	+1

Async chunks

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

id	before	after	diff
`watcher`	269.9KB	273.4KB	+3.6KB

Page load bundle

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

id	before	after	diff
`watcher`	13.5KB	13.3KB	-178.0B

History

💚 Build #65250 succeeded 426cb79
💚 Build #65150 succeeded e5dbcd6
💚 Build #64640 succeeded 72e7bc6
💔 Build #64429 failed 71e7b85416078501a2911672a65aeabe3b356097
💚 Build #64423 succeeded 6036d514fe797012e60fc7bc22774e2244b3633c
💔 Build #64380 failed 2cc8fb60d2f3985c2880c0f1d62e85409f8ace05

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

* Update WATCH_STATES by replacing DISABLED with INACTIVE and OK/FIRING with ACTIVE. Update ACTION_STATES by replacing FIRING with OK. - Refactor isAckable logic to directly translate value provided by ES. - Replace WatchStatus component with ActionStateBadge and WatchStateBadge components. * Change WatchListPage 'Last fired' column to 'Condition last met' and 'Last triggered' column to 'Last checked'. - Add tooltips to both. - Add tooltips to State and Comment headers. * Rename Watch Status Model's lastFired -> lastExecution for consistency. Remove unused properties from client WatchStatus model. * Add Condition Met column and tooltips to State and Comment headers on Execution History Panel. * Add tooltip to State header in Execution History flyout. Add Last executed header to Action Statuses Panel and tooltips. Update tests. * Use EUI types instead of hand-rolling them. * Remove unused ACTION_STATES.FIRING and references to WATCH_STATES.FIRING. * Use dots to denote states, similar to Index Management. * Refactor deriveComment to use early exits.

cjcenizal added release_note:fix Feature:Watcher Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// labels Aug 10, 2022

cjcenizal mentioned this pull request Aug 10, 2022

Convert Watch Status and Action Status model classes to stateless functions and TS #138026

Merged

cjcenizal force-pushed the bug/watcher-stuck-firing-state branch 2 times, most recently from 71e7b85 to 87d2208 Compare August 11, 2022 18:27

cjcenizal commented Aug 11, 2022

View reviewed changes

CJ Cenizal added 5 commits August 11, 2022 12:20

Change WatchListPage 'Last fired' column to 'Condition last met' and …

afe5cc6

…'Last triggered' column to 'Last checked'. - Add tooltips to both. - Add tooltips to State and Comment headers.

Rename Watch Status Model's lastFired -> lastExecution for consistenc…

ef2442d

…y. Remove unused properties from client WatchStatus model.

Add Condition Met column and tooltips to State and Comment headers on…

6dc2959

… Execution History Panel.

Add tooltip to State header in Execution History flyout. Add Last exe…

72e7bc6

…cuted header to Action Statuses Panel and tooltips. Update tests.

cjcenizal force-pushed the bug/watcher-stuck-firing-state branch from 87d2208 to 72e7bc6 Compare August 11, 2022 19:20

cjcenizal marked this pull request as ready for review August 11, 2022 19:20

cjcenizal requested a review from a team as a code owner August 11, 2022 19:20

cjcenizal requested a review from alisonelizabeth August 11, 2022 19:21

Merge branch 'main' into bug/watcher-stuck-firing-state

e5dbcd6

alisonelizabeth reviewed Aug 15, 2022

View reviewed changes

CJ Cenizal added 3 commits August 15, 2022 13:59

Use EUI types instead of hand-rolling them.

74dc349

Remove unused ACTION_STATES.FIRING and references to WATCH_STATES.FIR…

9fc49e5

…ING..

Use dots to denote states, similar to Index Management.

b888297

gchaps reviewed Aug 15, 2022

View reviewed changes

CJ Cenizal added 2 commits August 15, 2022 14:40

Refactor deriveComment to use early exits and add WATCH_STATE_COMMENT…

6c5af1a

…S.IS_ACKABLE state.

Update copy.

426cb79

gchaps reviewed Aug 15, 2022

View reviewed changes

gchaps approved these changes Aug 15, 2022

View reviewed changes

Revert addition of IS_ACKABLE comment state.

712ec49

alisonelizabeth approved these changes Aug 16, 2022

View reviewed changes

cjcenizal merged commit f77afae into elastic:main Aug 16, 2022

cjcenizal deleted the bug/watcher-stuck-firing-state branch August 16, 2022 01:25

kibanamachine added v8.5.0 backport:skip This PR does not require backporting labels Aug 16, 2022

This was referenced Aug 16, 2022

[Watcher] Clarify meanings of the various Watch states #17966

Closed

[Watcher UI] Watches state icon stuck in "Firing" #27758

Closed

This was referenced Aug 18, 2022

[DOCS] Adds 8.4.0 release notes #137621

Merged

Update Watcher docs to reflect new states #139435

Merged

alisonelizabeth mentioned this pull request Mar 20, 2023

[Watcher] Improve usability of simulate watch panel #153327

Closed

This was referenced Sep 17, 2024

[Watcher] Watch execution history shows OK when condition is met, but throttle in effect #59691

Closed

[Watcher UI] - State is OK but Execution History is in Error #77225

Closed

	defaultMessage: 'OK, acked, throttled, or error state',
	defaultMessage: 'OK, acknowledged, throttled, or error',

	defaultMessage: `When this action was last executed`,
	defaultMessage: `The last time this action was executed`,

	'Information about whether the watch was throttled, acknowledged, or failed to execute',
	'Whether the action was throttled, acknowledged, or failed to execute',

Fix Watcher stuck firing state #138563

Fix Watcher stuck firing state #138563

Uh oh!

Conversation

cjcenizal commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release note

Summary

Data changes

UI changes

Watch list

Execution history panel

Action statuses panel

Simulate watch results flyout

Steps to test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 11, 2022

Uh oh!

alisonelizabeth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchaps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Aug 15, 2022

Uh oh!

gchaps Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchaps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cjcenizal commented Aug 10, 2022 •

edited

Loading

gchaps Aug 15, 2022 •

edited

Loading