Handle warnings from task manager without stack traces. #30692

njd5475 · 2019-02-11T18:23:25Z

Summary

This PR adds catches to for the initialization errors that are thrown by the Task Manager. So that the logs are not plagued with stack traces.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~This was checked for cross-browser compatibility, including a check against IE11~~
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
~~Unit or functional tests were updated or added to match the most common scenarios~~
~~This was checked for keyboard-only and screenreader accessibilityz~~

For maintainers

~~This was checked for breaking API changes and was labeled appropriately~~
~~This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-02-11T18:39:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-11T20:08:47Z

Pinging @elastic/kibana-platform

Swallow stack traces and do not throw errors.

njd5475 · 2019-02-11T23:50:48Z

retest

elasticmachine · 2019-02-12T00:38:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan

LGTM

* Handle warnings from task manager without stack traces. * Fix typescript errors. * Catch no living connection errors. Swallow stack traces and do not throw errors. * Should use string substitution instead of concatenation.

epixa · 2019-02-12T18:21:45Z

x-pack/plugins/maps/server/maps_telemetry/telemetry_task.js

+        state: { stats: {}, runs: 0 },
+      });
+    }catch(e) {
+      server.log(['warning', 'maps'], `Error scheduling telemetry task, received ${e.message}`);


Couple things:

This is using the tag maps, and I assume it should be using a tag relevant to task manager.

I don't think swallowing all possible exceptions and treating them as warnings is the right approach here, or pretty much ever. There are specific errors we'd expect to see in the course of normal operation (like Elasticsearch not being available or authentication being rejected), but if something truly exceptional occurs, we'll want it to be treated properly as an error.

this is the maps collector, so that part is right

I agree this shouldn't be swallowed, but on the other hand, I hope the timing errors or connection handling can be handled more by Task Manager

The timing errors should be avoidable, by reworking the way the task manager gets initialized. So that we completely avoid the chicken or the egg problem. But I agree that it would be nice to handle these inside the task manager rather than put this on the consumers of the task manager service.

epixa · 2019-02-12T18:22:53Z

x-pack/plugins/task_manager/task_manager.ts

@@ -99,7 +99,7 @@ export class TaskManager {
          })
          .catch((err: Error) => {
            // FIXME: check the type of error to make sure it's actually an ES error
-            logger.warning(err.message);
+            logger.warning(`PollError ${err.message}`);


This sort of thing should be handled by a logger tag rather than string concatenation. This ensures folks can reliably filter/drill-down alerts based on type.

epixa · 2019-02-12T18:26:27Z

@njd5475 I know this is already merged, but please address the feedback I just posted in a followup PR. /cc @tsullivan

epixa · 2019-02-12T18:48:59Z

Also, please add some tests for this behavior

) * Handle warnings from task manager without stack traces. * Fix typescript errors. * Catch no living connection errors. Swallow stack traces and do not throw errors. * Should use string substitution instead of concatenation.

tsullivan · 2019-03-05T17:46:01Z

Coming back to this after testing a 6.7.0 build.

I start Kibana and Elasticsearch at the same time, and it takes Elasticsearch awhile to start all of the shards.

There are 2 plugins that try to schedule their own tasks, and they hit a problem that gets somewhat obscured by this change:

�[36mkibana_1         |�[0m {"type":"log","@timestamp":"2019-03-05T17:02:58Z","tags":["warning","maps"],"pid":1,"message":"Error scheduling telemetry task, received NotInitialized: Tasks cannot be scheduled until after task manager is initialized!"}

�[36mkibana_1         |�[0m {"type":"log","@timestamp":"2019-03-05T17:02:58Z","tags":["warning","telemetry"],"pid":1,"message":"Error scheduling task, received NotInitialized: Tasks cannot be scheduled until after task manager is initialized!"}

How plugins can schedule their own built-in tasks unfortunately seems to be a mystery. Ideally, waiting for the kbnServer.afterPluginsInit event can work (1), but it's not reliable since there could still be no connection to ES. All the plugins can init before Elasticsearch has any shards ready. When the plugins try to schedule their tasks, Task Manager just throws them away and logs this error.

Question for Platform: Should plugins wait on some other event before scheduling built-in tasks? Should they keep retrying on their own? All task scheduling goes through taskManager.schedule, so it would be great for the consumer if Task Manager "managed" the retrying. (2)

(1) and helps the Task Manager know that it has all task types registered by this point. An unrecognized task type is a bug, not a race condition
(2) also, the Task Manager handles retrying when task execution throws an error - so retries seem to be in the Task Manager domain.

cc @alexfrancoeur

Handle warnings from task manager without stack traces.

c1ec3bd

njd5475 added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Feb 11, 2019

njd5475 added v7.0.0 v6.7.0 v6.7.1 v8.0.0 v7.2.0 and removed v6.7.0 labels Feb 11, 2019

njd5475 added 3 commits February 11, 2019 15:41

Fix typescript errors.

354e8c6

Catch no living connection errors.

e55442e

Swallow stack traces and do not throw errors.

Should use string substitution instead of concatenation.

052fc54

njd5475 requested a review from tsullivan February 12, 2019 14:33

tsullivan approved these changes Feb 12, 2019

View reviewed changes

njd5475 merged commit 057815f into elastic:master Feb 12, 2019

njd5475 mentioned this pull request Feb 12, 2019

[7.x] Handle warnings from task manager without stack traces. (#30692) #30869

Merged

njd5475 mentioned this pull request Feb 12, 2019

[7.0] Handle warnings from task manager without stack traces. (#30692) #30870

Merged

njd5475 mentioned this pull request Feb 12, 2019

[6.7] Handle warnings from task manager without stack traces. (#30692) #30871

Merged

epixa reviewed Feb 12, 2019

View reviewed changes

epixa added non-issue Indicates to automation that a pull request should not appear in the release notes and removed v6.7.1 labels Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle warnings from task manager without stack traces. #30692

Handle warnings from task manager without stack traces. #30692

njd5475 commented Feb 11, 2019 •

edited

Loading

elasticmachine commented Feb 11, 2019

elasticmachine commented Feb 11, 2019

njd5475 commented Feb 11, 2019

elasticmachine commented Feb 12, 2019

tsullivan left a comment

epixa Feb 12, 2019

tsullivan Feb 12, 2019

njd5475 Feb 12, 2019 •

edited

Loading

epixa Feb 12, 2019

epixa commented Feb 12, 2019

epixa commented Feb 12, 2019

tsullivan commented Mar 5, 2019 •

edited

Loading

Handle warnings from task manager without stack traces. #30692

Handle warnings from task manager without stack traces. #30692

Conversation

njd5475 commented Feb 11, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 11, 2019

💔 Build Failed

elasticmachine commented Feb 11, 2019

njd5475 commented Feb 11, 2019

elasticmachine commented Feb 12, 2019

💚 Build Succeeded

tsullivan left a comment

Choose a reason for hiding this comment

epixa Feb 12, 2019

Choose a reason for hiding this comment

tsullivan Feb 12, 2019

Choose a reason for hiding this comment

njd5475 Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

epixa Feb 12, 2019

Choose a reason for hiding this comment

epixa commented Feb 12, 2019

epixa commented Feb 12, 2019

tsullivan commented Mar 5, 2019 • edited Loading

njd5475 commented Feb 11, 2019 •

edited

Loading

njd5475 Feb 12, 2019 •

edited

Loading

tsullivan commented Mar 5, 2019 •

edited

Loading