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

[Watcher] Add support for ERROR and UNKNOWN watch action statuses #55092

Merged
merged 5 commits into from
Jan 22, 2020
Merged

[Watcher] Add support for ERROR and UNKNOWN watch action statuses #55092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
71dbb4c
add support for additional watch action statuses
alisonelizabeth Jan 16, 2020
2d32f53
update tests + fix i18n
alisonelizabeth Jan 16, 2020
5f08e61
Update x-pack/legacy/plugins/watcher/server/np_ready/models/action_st…
alisonelizabeth Jan 21, 2020
21be8fd
Merge branch 'master' of github.com:elastic/kibana into watcher/actio…
alisonelizabeth Jan 21, 2020
91a2773
Merge branch 'master' into watcher/action_status_bugfix
elasticmachine Jan 21, 2020
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
5 changes: 5 additions & 0 deletions x-pack/legacy/plugins/watcher/common/constants/action_states.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,9 @@ export const ACTION_STATES: { [key: string]: string } = {
CONFIG_ERROR: i18n.translate('xpack.watcher.constants.actionStates.configErrorStateText', {
defaultMessage: 'Config error',
}),

// Action status is unknown; we should never end up in this state
UNKNOWN: i18n.translate('xpack.watcher.constants.actionStates.unknownStateText', {
defaultMessage: 'Unknown',
}),
};
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/watcher/public/np_ready/application/components/watch_status.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ function StatusIcon({ status }: { status: string }) {
return <EuiIcon type="minusInCircleFilled" color="subdued" />;
case WATCH_STATES.CONFIG_ERROR:
case WATCH_STATES.ERROR:
case ACTION_STATES.UNKNOWN:
return <EuiIcon type="cross" color="subdued" />;
case ACTION_STATES.CONFIG_ERROR:
case ACTION_STATES.ERROR:
return <EuiIcon type="crossInACircleFilled" color="danger" />;
Expand Down
53 changes: 34 additions & 19 deletions ...ck/legacy/plugins/watcher/server/np_ready/models/action_status/__tests__/action_status.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,39 @@ describe('action_status', () => {
};
});

it(`correctly calculates ACTION_STATES.ERROR`, () => {
upstreamJson.actionStatusJson.last_execution.successful = false;
const actionStatus = ActionStatus.fromUpstreamJson(upstreamJson);
describe(`correctly calculates ACTION_STATES.ERROR`, () => {
it('lastExecutionSuccessful is equal to false', () => {
upstreamJson.actionStatusJson.last_execution.successful = false;
const actionStatus = ActionStatus.fromUpstreamJson(upstreamJson);
expect(actionStatus.state).to.be(ACTION_STATES.ERROR);
});

expect(actionStatus.state).to.be(ACTION_STATES.ERROR);
it('action is acked and lastAcknowledged is less than lastExecution', () => {
const actionStatus = ActionStatus.fromUpstreamJson({
...upstreamJson,
actionStatusJson: {
ack: {
state: 'acked',
timestamp: '2017-03-01T00:00:00.000Z',
},
last_execution: {
timestamp: '2017-03-02T00:00:00.000Z',
},
},
});
expect(actionStatus.state).to.be(ACTION_STATES.ERROR);
});

it('action is ackable and lastSuccessfulExecution is less than lastExecution', () => {
delete upstreamJson.actionStatusJson.last_throttle;
upstreamJson.actionStatusJson.ack.state = 'ackable';
upstreamJson.actionStatusJson.last_successful_execution.timestamp =
'2017-03-01T00:00:00.000Z';
upstreamJson.actionStatusJson.last_execution.timestamp = '2017-03-02T00:00:00.000Z';
const actionStatus = ActionStatus.fromUpstreamJson(upstreamJson);

expect(actionStatus.state).to.be(ACTION_STATES.ERROR);
});
});

it('correctly calculates ACTION_STATES.CONFIG_ERROR', () => {
Expand Down Expand Up @@ -192,18 +220,7 @@ describe('action_status', () => {
});
});

it(`correctly calculates ACTION_STATES.ERROR`, () => {
delete upstreamJson.actionStatusJson.last_throttle;
upstreamJson.actionStatusJson.ack.state = 'ackable';
upstreamJson.actionStatusJson.last_successful_execution.timestamp =
'2017-03-01T00:00:00.000Z';
upstreamJson.actionStatusJson.last_execution.timestamp = '2017-03-02T00:00:00.000Z';
const actionStatus = ActionStatus.fromUpstreamJson(upstreamJson);

expect(actionStatus.state).to.be(ACTION_STATES.ERROR);
});

it(`throws an error if it can not determine ACTION_STATE`, () => {
it(`correctly calculates ACTION_STATES.UNKNOWN if it can not determine state`, () => {
upstreamJson = {
id: 'my-action',
actionStatusJson: {
Expand All @@ -213,9 +230,7 @@ describe('action_status', () => {
};
const actionStatus = ActionStatus.fromUpstreamJson(upstreamJson);

expect(() => {
actionStatus.state;
}).to.throwError(/could not determine action status/i);
expect(actionStatus.state).to.be(ACTION_STATES.UNKNOWN);
});
});

Expand Down
21 changes: 8 additions & 13 deletions x-pack/legacy/plugins/watcher/server/np_ready/models/action_status/action_status.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { get } from 'lodash';
import { badImplementation, badRequest } from 'boom';
import { badRequest } from 'boom';
import { getMoment } from '../../../../common/lib/get_moment';
import { ACTION_STATES } from '../../../../common/constants';
import { i18n } from '@kbn/i18n';
Expand Down Expand Up @@ -46,6 +46,11 @@ export class ActionStatus {
return ACTION_STATES.ACKNOWLEDGED;
}

// A user could potentially land in this state if running on multiple nodes and timing is off
if (ackState === 'acked' && this.lastAcknowledged < this.lastExecution) {
return ACTION_STATES.ERROR;
}

if (ackState === 'ackable' && this.lastThrottled >= this.lastExecution) {
return ACTION_STATES.THROTTLED;
}
Expand All @@ -58,20 +63,10 @@ export class ActionStatus {
return ACTION_STATES.ERROR;
}

// At this point, we cannot determine the action status so we thrown an error.
// At this point, we cannot determine the action status so mark it as "unknown".
// We should never get to this point in the code. If we do, it means we are
// missing an action status and the logic to determine it.
throw badImplementation(
i18n.translate(
'xpack.watcher.models.actionStatus.notDetermineActionStatusBadImplementationMessage',
{
defaultMessage: 'Could not determine action status; action = {actionStatusJson}',
values: {
actionStatusJson: JSON.stringify(actionStatusJson),
},
}
)
);
return ACTION_STATES.UNKNOWN;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed more appropriate to mark the action in an unknown state, rather than throwing an error and making the watch list view unusable.

The only potential outstanding question is what the overall watch status should return in this scenario. I did not make any code changes to how this is handled, so it currently will return OK. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely agree with rather keeping the view usable in case of a bad status (i.e., catering for this condition here definitely seems like the correct thing to do). Overall I am happy with this approach 👍

The one advantage of throwing in this case, however, is that we were able to discover this bug which I think may have remained hidden otherwise. So being totally silent also has a cost (as you've noted).

Judging how action_status and watch_status hang together, it would be cool to be able to say that if the last action status was unknown, then the watch_status is unknown? Remaining in this state should be a red flag to the user + they can still use the UI, we should communicate it's a possible bug. This could be a separate unit of work to this fix though!

Let me know what you think? We could revert this line for now and address later, or commit and address later. I'm happy with either for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jloleysens I like this idea a lot - thanks! I opened #55410 to address separately.

}

get isAckable() {
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/ja-JP.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12893,7 +12893,6 @@
"xpack.watcher.deleteSelectedWatchesConfirmModal.deleteButtonLabel": "{numWatchesToDelete, plural, one {ウォッチ} other {# ウォッチ}}を削除 ",
"xpack.watcher.deleteSelectedWatchesConfirmModal.descriptionText": "{numWatchesToDelete, plural, one {削除されたウォッチ} other {削除されたウォッチ}}は回復できません",
"xpack.watcher.models.actionStatus.actionStatusJsonPropertyMissingBadRequestMessage": "JSON引数には\"{missingProperty}\"プロパティが含まれている必要があります",
"xpack.watcher.models.actionStatus.notDetermineActionStatusBadImplementationMessage": "アクションステータスを把握できませんでした; action = {actionStatusJson}",
"xpack.watcher.models.baseAction.selectMessageText": "アクションを実行します。",
"xpack.watcher.models.baseAction.simulateButtonLabel": "今すぐこのアクションをシミュレート",
"xpack.watcher.models.baseAction.simulateMessage": "アクション {id} のシミュレーションが完了しました",
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/zh-CN.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12892,7 +12892,6 @@
"xpack.watcher.deleteSelectedWatchesConfirmModal.deleteButtonLabel": "删除 {numWatchesToDelete, plural, one {个监视} other {# 个监视}} ",
"xpack.watcher.deleteSelectedWatchesConfirmModal.descriptionText": "无法恢复{numWatchesToDelete, plural, one {已删除监视} other {已删除监视}}。",
"xpack.watcher.models.actionStatus.actionStatusJsonPropertyMissingBadRequestMessage": "JSON 参数必须包含“{missingProperty}”属性",
"xpack.watcher.models.actionStatus.notDetermineActionStatusBadImplementationMessage": "无法确定操作状态；操作 = {actionStatusJson}",
"xpack.watcher.models.baseAction.selectMessageText": "执行操作。",
"xpack.watcher.models.baseAction.simulateButtonLabel": "立即模拟此操作",
"xpack.watcher.models.baseAction.simulateMessage": "已成功模拟操作 {id}",
Expand Down