[server/logging] intercept ECONNRESET messages and downgrade them by spalger · Pull Request #8785 · elastic/kibana

spalger · 2016-10-20T19:26:22Z

Implements a LogInterceptor that sees every log message before it is filtered by the squeeze stream. This allows us to transform log messages we might not have control over, like the log messages that come from hapi itself, before they are processed by the standard log message pipeline.

tylersmalley · 2016-10-20T20:07:42Z

src/server/logging/do_tags_match.js

+  }
+
+  const unmatchedEventTags = event.tags.slice(0);
+  const unmatchedExpectedTags = [];


The tags seem to have a perceived order. Is there a reason for not requiring the order to match as well?

If we require the order to match, I think we could potentially use _.isEqual

I suppose I could sort and then use _.isEqual, sounds good to me!

I am thinking we don't even need to sort. The expected tags would need to match the order of the events tags. Trying to not do more than we need to.

kobelb

LGTM

One minor question/suggestion

kobelb · 2016-10-20T19:32:34Z

src/server/logging/do_tags_match.js

+    if (i > -1) {
+      unmatchedEventTags.splice(i, 1);
+    } else {
+      unmatchedExpectedTags.push(t);


Could we just return false here?

tylersmalley · 2016-10-20T20:11:52Z

src/server/logging/do_tags_match.js

+    }
+  });
+
+  return unmatchedEventTags.concat(unmatchedExpectedTags).length === 0;


We could check if the length of unmatchedEventTags is 0

Hmm, I suppose we could, since the lengths of both arrays is the same.

tylersmalley · 2016-10-20T20:37:29Z

src/server/logging/log_interceptor.js

+import { get, isEqual, sortBy } from 'lodash';
+
+function doTagsMatch(event, tags) {
+  return isEqual(sortBy(get(event, 'tags')), sortBy(tags));


Repeating here since my comment is hidden after the last update:

Is there a reason why we need to support not matching the order?

We don't control the code that sends the tags, and I don't see why we wouldn't.

It just doesn't seem necessary to add the additional overhead while limiting control. Should the tag order change in Hapi we can always update them. In the future if we want to be less greedy with the matching we can then sort.

kobelb · 2016-10-20T20:41:17Z

LGTM

tylersmalley · 2016-10-20T20:56:50Z

src/server/logging/__tests__/log_interceptor.js

@@ -0,0 +1,55 @@
+import expect from 'expect.js';
+import { shuffle } from 'lodash';


shuffle is no longer being used.

tylersmalley

LGTM!

kobelb · 2016-10-20T21:01:55Z

src/server/logging/log_interceptor.js

+   *  @param {object} - log event
+   */
+  downgradeIfEconnreset(event) {
+    const isClientError = doTagsMatch(event, ['error', 'client', 'connection']);


Hapi is using the following order ['connection', 'client', 'error'] per https://github.com/hapijs/hapi/blob/master/lib/connection.js#L96

ugh, I looked at the order in the log output, which is sorted by color I think :/

Good catch! I have seen it so much today it's just a blur.

kobelb · 2016-10-20T21:06:45Z

LGTM - Ship It!!

--------- **Commit 1:** [server/logging] intercept ECONNRESET messages and downgrade them * Original sha: 38bcad9 * Authored by spalger <email@spalger.com> on 2016-10-20T19:24:16Z **Commit 2:** [server/logging] remove doTagsMatch() helper * Original sha: a8eea58 * Authored by spalger <email@spalger.com> on 2016-10-20T20:19:33Z **Commit 3:** [server/logging] add tests for log interceptor * Original sha: 866abcc * Authored by spalger <email@spalger.com> on 2016-10-20T20:35:35Z **Commit 4:** [server/logging] only match tags that are in the right order * Original sha: 1973ed8 * Authored by spalger <email@spalger.com> on 2016-10-20T20:50:26Z **Commit 5:** [server/logging] remove unused dependency * Original sha: dbe5f6f * Authored by spalger <email@spalger.com> on 2016-10-20T20:58:59Z **Commit 6:** [server/logging] fix the order we expect tags to be in * Original sha: 76e09e6 * Authored by spalger <email@spalger.com> on 2016-10-20T21:04:58Z

[backport] PR #8785 to 5.0 - [server/logging] intercept ECONNRESET messages and downgrade them

[backport] PR #8785 to 5.x - [server/logging] intercept ECONNRESET messages and downgrade them

With the latest node/hapi upgrades, ECONNRESET errors are being logged whereas they weren't before.

epixa · 2016-12-22T14:07:14Z

4.6 9f5cf2c

--------- **Commit 1:** [server/logging] intercept ECONNRESET messages and downgrade them * Original sha: 77dc71aa714369e40f3660c655b9e08b00010699 [formerly 38bcad9] * Authored by spalger <email@spalger.com> on 2016-10-20T19:24:16Z **Commit 2:** [server/logging] remove doTagsMatch() helper * Original sha: 0bde8381146b8c4d391360cb4d5feda9b1e3d3cd [formerly a8eea58] * Authored by spalger <email@spalger.com> on 2016-10-20T20:19:33Z **Commit 3:** [server/logging] add tests for log interceptor * Original sha: 6c529b0f47582bc863f025f5fe83b07f59af6fd8 [formerly 866abcc] * Authored by spalger <email@spalger.com> on 2016-10-20T20:35:35Z **Commit 4:** [server/logging] only match tags that are in the right order * Original sha: 04b7da8b4cb502e25fc65714c3e8fa9cba5725ca [formerly 1973ed8] * Authored by spalger <email@spalger.com> on 2016-10-20T20:50:26Z **Commit 5:** [server/logging] remove unused dependency * Original sha: 7990a50ec74343b3b1a3200873132377dfbda20e [formerly dbe5f6f] * Authored by spalger <email@spalger.com> on 2016-10-20T20:58:59Z **Commit 6:** [server/logging] fix the order we expect tags to be in * Original sha: f21a523883ef26c8ca7d461c54da8d66fb0076c5 [formerly 76e09e6] * Authored by spalger <email@spalger.com> on 2016-10-20T21:04:58Z Former-commit-id: 6326821

[backport] PR elastic#8785 to 5.x - [server/logging] intercept ECONNRESET messages and downgrade them Former-commit-id: a5080ee

[server/logging] intercept ECONNRESET messages and downgrade them

38bcad9

spalger added review v5.0.0 v5.1.0 v6.0.0 labels Oct 20, 2016

spalger assigned tylersmalley and kobelb Oct 20, 2016

tylersmalley reviewed Oct 20, 2016

View reviewed changes

kobelb reviewed Oct 20, 2016

View reviewed changes

tylersmalley reviewed Oct 20, 2016

View reviewed changes

spalger added 2 commits October 20, 2016 13:19

[server/logging] remove doTagsMatch() helper

a8eea58

[server/logging] add tests for log interceptor

866abcc

tylersmalley reviewed Oct 20, 2016

View reviewed changes

[server/logging] only match tags that are in the right order

1973ed8

tylersmalley reviewed Oct 20, 2016

View reviewed changes

[server/logging] remove unused dependency

dbe5f6f

tylersmalley approved these changes Oct 20, 2016

View reviewed changes

kobelb reviewed Oct 20, 2016

View reviewed changes

[server/logging] fix the order we expect tags to be in

76e09e6

tylersmalley approved these changes Oct 20, 2016

View reviewed changes

spalger merged commit 7c482af into elastic:master Oct 20, 2016

This was referenced Oct 20, 2016

[backport] PR #8785 to 5.0 - [server/logging] intercept ECONNRESET messages and downgrade them #8786

Merged

[backport] PR #8785 to 5.x - [server/logging] intercept ECONNRESET messages and downgrade them #8787

Merged

epixa added a commit that referenced this pull request Oct 20, 2016

Merge pull request #8786 from elastic/jasper/backport/8785/5.0

a69ab2d

[backport] PR #8785 to 5.0 - [server/logging] intercept ECONNRESET messages and downgrade them

epixa added a commit that referenced this pull request Oct 20, 2016

Merge pull request #8787 from elastic/jasper/backport/8785/5.x

a5080ee

[backport] PR #8785 to 5.x - [server/logging] intercept ECONNRESET messages and downgrade them

spalger mentioned this pull request Nov 10, 2016

[server/logger] downgrade EPIPE errors to debug level #9023

Merged

epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016

epixa mentioned this pull request Dec 22, 2016

"Error: write ECANCELED" in logs, after refreshing a page #9609

Closed

epixa added a commit that referenced this pull request Dec 22, 2016

server: downgrade ECONNRESET messages (#8785)

9f5cf2c

With the latest node/hapi upgrades, ECONNRESET errors are being logged whereas they weren't before.

epixa added the v4.6.4 label Dec 22, 2016

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Merge pull request elastic#8787 from elastic/jasper/backport/8785/5.x

8a002b0

[backport] PR elastic#8785 to 5.x - [server/logging] intercept ECONNRESET messages and downgrade them Former-commit-id: a5080ee

spalger deleted the implement/server-log-interceptor branch October 18, 2019 17:40

		@@ -0,0 +1,55 @@
		import expect from 'expect.js';
		import { shuffle } from 'lodash';

Conversation

spalger commented Oct 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobelb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobelb commented Oct 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylersmalley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobelb commented Oct 20, 2016

Uh oh!

epixa commented Dec 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

spalger Oct 20, 2016 •

edited

Loading