[Watcher] Fix logic that determines watch error state #67952
Conversation
alisonelizabeth
added
release_note:fix
Feature:Watcher
v8.0.0
Team:Kibana Management
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
jloleysens
left a comment
jloleysens
left a comment
There was a problem hiding this comment.
Did not test, but happy with the addition of the tests!
Great work @alisonelizabeth
| it('lastExecutionSuccessful is equal to false', () => { | ||
| it('lastExecutionSuccessful is equal to false and it is the most recent execution', () => { | ||
| upstreamJson.actionStatusJson.last_execution.successful = false; | ||
| upstreamJson.actionStatusJson.last_execution.timestamp = |
There was a problem hiding this comment.
I'm not 100% sure what this assignment is doing?
There was a problem hiding this comment.
Not sure what I was going for there either 😂 . Removed.
| if ( | ||
| this.lastExecutionSuccessful === false && | ||
| this.lastCheckedRawFormat === this.lastExecutionRawFormat | ||
| ) { |
There was a problem hiding this comment.
From this it looks like lastExecutionSuccessful can be false but we have an edge case where lastCheckedRawFormat come after that? I.o.w. the last execution could be successful even though the lastExecutionSuccessful field is false?
There was a problem hiding this comment.
So the watch only "executes" when the condition is met, but the watch condition is checked on a given interval. So the last time it was executed could be false, but the next time it is checked the condition may not be met so it doesn't execute. Does that make sense? I also tried to explain this in the PR description, but I'm probably not doing the best job so let me know if you need me to clarify more 😅 .
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
4 participants
Fixes #22209
Fixes #25134
Fixes #18462
How to review
The top two linked issues include different ways to test.
I would like confirmation that the logic change is reasonable for the end user. Previously, the UI was determining an "error" status only by the
last_execution_successfulflag. This could be misleading as a user could end up in a situation where thelast_execution_successfulis indeedfalse, but the issue causing the original error has since been resolved and the condition is no longer being met. I have updated the logic so that the "error" state will only appear if thelast_execution_successfulisfalse, and the last time the watch condition was checked is equal to the last execution. Please let me know if you see any issues with this, or if you can think of any case that this wouldn't cover.Release note
This change fixes an issue in Watcher, where a watch status or action status was incorrectly marked as "Error".