Make TaskManager health status error/warning reasons more visible #154045

ersin-erdal · 2023-03-30T12:50:39Z

With this PR we make some of the debug logs warn and return the message to the health API as reason to add in the status summary message.

ersin-erdal · 2023-03-31T10:31:03Z

x-pack/plugins/task_manager/server/lib/log_health_metrics.test.ts

@@ -41,16 +41,30 @@ describe('logHealthMetrics', () => {
    const { calculateHealthStatus } = jest.requireMock('./calculate_health_status');

    // We must change from OK to Warning
-    (calculateHealthStatus as jest.Mock<HealthStatus>).mockImplementation(() => HealthStatus.OK);


These mocks were no-op, never stubbed the real function.

It doesn't look like we changed anything though - except to change the return type to an object and not just the status. The parenthesized function invocations to mockImplementation() definitely look a little weird, but was there some kind of problem with them? They look like they could work.

But they do look confusing! :-). I wonder if there's a cleanup we can do here - in another issue/PR ...

elasticmachine · 2023-03-31T10:33:13Z

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

pmuellr · 2023-03-31T21:58:40Z

\o/

Hacked a "bad reason" message into the code (because I said so!), saw this from /api/status:

{
  "status": {
    "...": "...",
    "plugins": {
      "...": "...",
      "taskManager": {
        "level": "degraded",
        "summary": "Task Manager is unhealthy - Reason: because I said so!"
      }
    }
  }
}

…89-tm-health-logs

ersin-erdal · 2023-04-11T15:11:39Z

\o/

Hacked a "bad reason" message into the code (because I said so!), saw this from /api/status:

{
  "status": {
    "...": "...",
    "plugins": {
      "...": "...",
      "taskManager": {
        "level": "degraded",
        "summary": "Task Manager is unhealthy - Reason: because I said so!"
      }
    }
  }
}

Sorry, I forgot to ping you Patrick.
I pushed a fix for this last week. Now it uses only the reasons we defined in the code.

pmuellr

LGTM

x-pack/plugins/task_manager/server/lib/calculate_health_status.ts

x-pack/plugins/task_manager/server/lib/log_health_metrics.ts

pmuellr · 2023-03-31T18:30:27Z

x-pack/plugins/task_manager/server/monitoring/task_run_statistics.ts

        `Health Status error threshold has been exceeded, resultFrequencySummary.Failed (${resultFrequencySummary.Failed}) is greater than error_threshold (${executionErrorThreshold.error_threshold})`
      );
    } else {
-      logger.debug(
+      logger.warn(
        `Health Status warn threshold has been exceeded, resultFrequencySummary.Failed (${resultFrequencySummary.Failed}) is greater than warn_threshold (${executionErrorThreshold.warn_threshold})`
      );
    }


Interesting that we don't return a negative HealthStatus here. But feels like it makes sense, since the task errors could very likely be caused by the user somehow, and not systemic (invalid parameters used in a rule / connector) - we shouldn't set the TM plugin status based on that. But I bet we used to :-)

x-pack/plugins/task_manager/server/routes/health.ts

pmuellr · 2023-03-31T22:07:48Z

x-pack/plugins/task_manager/server/lib/log_health_metrics.test.ts

@@ -41,16 +41,30 @@ describe('logHealthMetrics', () => {
    const { calculateHealthStatus } = jest.requireMock('./calculate_health_status');

    // We must change from OK to Warning
-    (calculateHealthStatus as jest.Mock<HealthStatus>).mockImplementation(() => HealthStatus.OK);


It doesn't look like we changed anything though - except to change the return type to an object and not just the status. The parenthesized function invocations to mockImplementation() definitely look a little weird, but was there some kind of problem with them? They look like they could work.

But they do look confusing! :-). I wonder if there's a cleanup we can do here - in another issue/PR ...

pmuellr · 2023-03-31T22:09:31Z

x-pack/plugins/task_manager/server/monitoring/task_run_statistics.test.ts

@@ -375,24 +375,24 @@ describe('Task Run Statistics', () => {
              { Success: 40, RetryScheduled: 40, Failed: 20, status: 'OK' },
            ]);

-            expect(logger.debug).toHaveBeenCalledTimes(5);
-            expect(logger.debug).toHaveBeenNthCalledWith(
+            expect(logger.warn).toHaveBeenCalledTimes(5);


I think I'm remembering now that these MIGHT have been warns before, but they were SDH generators so we changed them to debug. But even if so, seems fair to me!

kibana-ci · 2023-04-18T12:50:11Z

💚 Build Succeeded

Buildkite Build
Commit: ed667eb

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	432	435	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	512	515	+3

History

💚 Build #119065 succeeded c4cd4fc
💛 Build #117233 was flaky ea33657
💔 Build #117228 failed 7cf2e31
💛 Build #116768 was flaky 1b5e5cf
💔 Build #116734 failed 2f3762f

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

cc @ersin-erdal

…astic#154045) Resolves: elastic#152289 With this PR we make some of the `debug` logs `warn` and return the message to the health API as reason to add in the status summary message.

resolves elastic#156112 Change task manager logging on status errors from `warn` to `debug. Making this change as we recently changed from `debug` to `warn` in elastic#154045 . But this ended up too noisy, especially at Kibana startup.

…157762) resolves #156112 Change task manager logging on status errors from `warn` to `debug. Making this change as we recently changed from `debug` to `warn` in #154045 . But this ended up too noisy, especially at Kibana startup.

…lastic#157762) resolves elastic#156112 Change task manager logging on status errors from `warn` to `debug. Making this change as we recently changed from `debug` to `warn` in elastic#154045 . But this ended up too noisy, especially at Kibana startup. (cherry picked from commit b542862)

…anges (#157762) (#157800) # Backport This will backport the following commits from `main` to `8.8`: - [[ResponseOps][Task Manager] stop spamming the logs on status changes (#157762)](#157762)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Patrick Mueller <patrick.mueller@elastic.co>

…157762) resolves #156112 Change task manager logging on status errors from `warn` to `debug. Making this change as we recently changed from `debug` to `warn` in #154045 . But this ended up too noisy, especially at Kibana startup.

ersin-erdal added 2 commits March 29, 2023 14:55

fix nits

7840216

return reason for health metrics

2f3762f

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.8.0 labels Mar 30, 2023

ersin-erdal self-assigned this Mar 30, 2023

ersin-erdal and others added 2 commits March 30, 2023 16:11

Merge branch 'main' into 152289-tm-health-logs

1b5e5cf

Merge branch 'main' into 152289-tm-health-logs

9a1df12

ersin-erdal commented Mar 31, 2023

View reviewed changes

ersin-erdal marked this pull request as ready for review March 31, 2023 10:33

ersin-erdal requested a review from a team as a code owner March 31, 2023 10:33

ersin-erdal added 3 commits April 1, 2023 01:24

read reasons only from the capacity estimator

1ea7be8

Merge remote-tracking branch 'origin/152289-tm-health-logs' into 1522…

7cf2e31

…89-tm-health-logs

Merge branch 'main' into 152289-tm-health-logs

ea33657

Merge branch 'main' into 152289-tm-health-logs

c4cd4fc

pmuellr approved these changes Apr 17, 2023

View reviewed changes

Merge branch 'main' into 152289-tm-health-logs

ed667eb

ersin-erdal merged commit c76a68b into elastic:main Apr 18, 2023

kibanamachine added the backport:skip This commit does not require backporting label Apr 18, 2023

ersin-erdal deleted the 152289-tm-health-logs branch April 18, 2023 12:57

pmuellr mentioned this pull request Apr 27, 2023

[ResponseOps] task manager spamming the Kibana log at startup #156112

Closed

pmuellr mentioned this pull request May 15, 2023

[ResponseOps][Task Manager] stop spamming the logs on status changes #157762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TaskManager health status error/warning reasons more visible #154045

Make TaskManager health status error/warning reasons more visible #154045

ersin-erdal commented Mar 30, 2023 •

edited

Loading

ersin-erdal Mar 31, 2023

pmuellr Mar 31, 2023

elasticmachine commented Mar 31, 2023

pmuellr commented Mar 31, 2023

ersin-erdal commented Apr 11, 2023

\o/

pmuellr left a comment

pmuellr Mar 31, 2023

pmuellr Mar 31, 2023

pmuellr Mar 31, 2023

kibana-ci commented Apr 18, 2023

ESLint disabled line counts

Total ESLint disabled count

Make TaskManager health status error/warning reasons more visible #154045

Make TaskManager health status error/warning reasons more visible #154045

Conversation

ersin-erdal commented Mar 30, 2023 • edited Loading

ersin-erdal Mar 31, 2023

Choose a reason for hiding this comment

pmuellr Mar 31, 2023

Choose a reason for hiding this comment

elasticmachine commented Mar 31, 2023

pmuellr commented Mar 31, 2023

\o/

ersin-erdal commented Apr 11, 2023

\o/

pmuellr left a comment

Choose a reason for hiding this comment

pmuellr Mar 31, 2023

Choose a reason for hiding this comment

pmuellr Mar 31, 2023

Choose a reason for hiding this comment

pmuellr Mar 31, 2023

Choose a reason for hiding this comment

kibana-ci commented Apr 18, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

ersin-erdal commented Mar 30, 2023 •

edited

Loading