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

Event log shows a failure when ruleResultService.addLastRunError adds an error #179551

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Mar 27, 2024

Resolves: #174035

As the new lastRun status holder for rule execution can set the outcome of a successful execution as failed
We have to do the same for the old executionStatus as well.

This PR, sets rule execution status as error when there are error messages reported by the ruleResultService.addLastRunError, even if no errors are thrown!
So the event-log and task metrics shows the correct results.

To verify

Please follow the steps in the issue to reproduce.

@ersin-erdal ersin-erdal added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.14.0 labels Mar 27, 2024
@ersin-erdal ersin-erdal self-assigned this Mar 27, 2024
@ersin-erdal
Copy link
Contributor Author

/ci

@ersin-erdal
Copy link
Contributor Author

/ci

@ersin-erdal
Copy link
Contributor Author

/ci

@ersin-erdal ersin-erdal marked this pull request as ready for review March 28, 2024 13:16
@ersin-erdal ersin-erdal requested a review from a team as a code owner March 28, 2024 13:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@pmuellr
Copy link
Member

pmuellr commented Mar 28, 2024

We should really add a function test for this, but I understand we're in a bit of a crunch - if that's too much for now, let's open an issue to add one.

@pmuellr
Copy link
Member

pmuellr commented Mar 28, 2024

Everything looks good here, except I'm seeing the TM metrics show an error from the rule I instrumented, to show up as a USER error, not framework:

          "alerting:__index-threshold": {
            "success": 0,
            "not_timed_out": 44,
            "total": 44,
            "total_errors": 44,
            "user_errors": 44,
            "framework_errors": 0
          },

I instrumented index threshold rule by adding a throw new Error('artificial error for testing') - so I'm not sure if in that case it's ok for it to be a user error or not. Seems likely, but thought I'd mention it.

update: this is expected. The code to set the user to be framework is only when they added the addLastRunError, but didn't actually throw an error

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, beyond adding a function test (via separate issue is fine).

Tested with the scenario described in the referenced issue, as well as an instrumented executor that explicitly throws errors - works as expected both way ...

@ersin-erdal
Copy link
Contributor Author

LGTM, beyond adding a function test (via separate issue is fine).

Tested with the scenario described in the referenced issue, as well as an instrumented executor that explicitly throws errors - works as expected both way ...

Created a PR to align the error types: #179656

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @ersin-erdal

@ersin-erdal ersin-erdal merged commit b0d1cb1 into elastic:main Mar 28, 2024
34 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 28, 2024
@ersin-erdal ersin-erdal deleted the 174035-add-last-run-error branch March 28, 2024 18:57
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 release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.14.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

The ruleResultService.addLastRunError(...) doesn't show failures in the event log and task metrics
5 participants