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.

Sign up for GitHub

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

Jump to bottom

Prevent excessive ES version warnings #8865

Merged
merged 3 commits into from
Oct 28, 2016
Merged

Prevent excessive ES version warnings #8865

merged 3 commits into from
Oct 28, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Oct 27, 2016

Warnings are currently spamming the log if we detect a possible compatibility issue, repeating the same warning on every health check

2016-10-27 13_59_30

@cjcenizal cjcenizal self-assigned this Oct 27, 2016
cjcenizal
cjcenizal reviewed Oct 27, 2016
View reviewed changes
src/core_plugins/elasticsearch/lib/__tests__/check_es_version.js
sinon.assert.callCount(server.log, 4);
expect(server.log.getCall(2).args[0]).to.contain('debug');
expect(server.log.getCall(3).args[0]).to.contain('warning');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet tests brah

@epixa epixa added the v5.0.1 label Oct 27, 2016
@epixa
Copy link
Contributor

epixa commented Oct 27, 2016

I marked this as a 5.0.1 as well - it's a pretty big deal from an operational perspective if we're spamming logs with unnecessary data, especially when you consider things like multi-tenant setups.

cjcenizal
cjcenizal approved these changes Oct 27, 2016
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Looks great overall, just had a couple small suggestions.

src/core_plugins/elasticsearch/lib/check_es_version.js
getHumanizedNodeNames,
nodes: simplifiedNodes,
});
const warningNodeNames = getHumanizedNodeNames(simplifiedNodes).join(', ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment?

// Don't show the same warning over and over again.

src/core_plugins/elasticsearch/lib/check_es_version.js
nodes: simplifiedNodes,
});
const warningNodeNames = getHumanizedNodeNames(simplifiedNodes).join(', ');
if (lastWarnedAboutNodes.get(server) !== warningNodeNames) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is server a reference to the same object every time this function is called? If so maybe a weakMap is overkill, and we just cache a reference with a regular variable, e.g. prevWarningNodeNames !== warningNodeNames.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in multi-tenancy situations

cjcenizal
cjcenizal reviewed Oct 27, 2016
View reviewed changes
src/core_plugins/elasticsearch/lib/check_es_version.js
});
// Don't show the same warning over and over again.
const warningNodeNames = getHumanizedNodeNames(simplifiedNodes).join(', ');
if (lastWarnedNodesForServer.get(server) !== warningNodeNames) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth some sort of comment here about how multi-tenancy comes into play... if it's written in a way so that even I can understand it, then it's a foolproof comment. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe such a comment belongs where lastWarnedNodesForServer is defined.

@jbudz
Copy link
Member

jbudz commented Oct 28, 2016

One comment above, LGTM otherwise.

@spalger spalger changed the title Limit es version warnings Prevent excessive ES version warnings Oct 28, 2016
@spalger spalger merged commit c5ca160 into elastic:master Oct 28, 2016
elastic-jasper added a commit that referenced this pull request Oct 28, 2016
@elastic-jasper
Backport PR #8865
2960e97 
---------

**Commit 1:**
[es/versionCheck] prevent spamming logs with compatibility warnings

* Original sha: bb95cf8
* Authored by spalger <email@spalger.com> on 2016-10-27T20:48:36Z

**Commit 2:**
[es/versionCheck] clarify new var and reason for check

* Original sha: cda9594
* Authored by spalger <email@spalger.com> on 2016-10-27T22:36:43Z

**Commit 3:**
[es/versionCheck] explain why we need to track per-server

* Original sha: 5eb9ccd
* Authored by spalger <email@spalger.com> on 2016-10-27T23:30:41Z
elastic-jasper added a commit that referenced this pull request Oct 28, 2016
@elastic-jasper
Backport PR #8865
bd90363 
---------

**Commit 1:**
[es/versionCheck] prevent spamming logs with compatibility warnings

* Original sha: bb95cf8
* Authored by spalger <email@spalger.com> on 2016-10-27T20:48:36Z

**Commit 2:**
[es/versionCheck] clarify new var and reason for check

* Original sha: cda9594
* Authored by spalger <email@spalger.com> on 2016-10-27T22:36:43Z

**Commit 3:**
[es/versionCheck] explain why we need to track per-server

* Original sha: 5eb9ccd
* Authored by spalger <email@spalger.com> on 2016-10-27T23:30:41Z
This was referenced Oct 28, 2016
Merged
Merged
spalger pushed a commit that referenced this pull request Oct 28, 2016
@elastic-jasper @spalger
Backport PR #8865 (#8875)
236557b 
---------

**Commit 1:**
[es/versionCheck] prevent spamming logs with compatibility warnings

* Original sha: bb95cf8
* Authored by spalger <email@spalger.com> on 2016-10-27T20:48:36Z

**Commit 2:**
[es/versionCheck] clarify new var and reason for check

* Original sha: cda9594
* Authored by spalger <email@spalger.com> on 2016-10-27T22:36:43Z

**Commit 3:**
[es/versionCheck] explain why we need to track per-server

* Original sha: 5eb9ccd
* Authored by spalger <email@spalger.com> on 2016-10-27T23:30:41Z
spalger pushed a commit that referenced this pull request Oct 28, 2016
@elastic-jasper @spalger
[backport] PR #8865 to 5.0 - Prevent excessive ES version warnings
5a67ac2 
---------

**Commit 1:**
[es/versionCheck] prevent spamming logs with compatibility warnings

* Original sha: bb95cf8
* Authored by spalger <email@spalger.com> on 2016-10-27T20:48:36Z

**Commit 2:**
[es/versionCheck] clarify new var and reason for check

* Original sha: cda9594
* Authored by spalger <email@spalger.com> on 2016-10-27T22:36:43Z

**Commit 3:**
[es/versionCheck] explain why we need to track per-server

* Original sha: 5eb9ccd
* Authored by spalger <email@spalger.com> on 2016-10-27T23:30:41Z
@jbudz jbudz mentioned this pull request Oct 28, 2016
Closed
nreese pushed a commit to nreese/kibana that referenced this pull request Nov 10, 2016
@spalger @nreese
Prevent excessive ES version warnings (elastic#8865)
d0acb58 
* [es/versionCheck] prevent spamming logs with compatibility warnings

* [es/versionCheck] clarify new var and reason for check

* [es/versionCheck] explain why we need to track per-server
@epixa epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
@elastic-jasper @spalger
Backport PR elastic#8865 (elastic#8875)
bbdbc3b 
---------

**Commit 1:**
[es/versionCheck] prevent spamming logs with compatibility warnings

* Original sha: bb95cf8
* Authored by spalger <email@spalger.com> on 2016-10-27T20:48:36Z

**Commit 2:**
[es/versionCheck] clarify new var and reason for check

* Original sha: cda9594
* Authored by spalger <email@spalger.com> on 2016-10-27T22:36:43Z

**Commit 3:**
[es/versionCheck] explain why we need to track per-server

* Original sha: 5eb9ccd
* Authored by spalger <email@spalger.com> on 2016-10-27T23:30:41Z

Former-commit-id: 236557b
@ctindel ctindel mentioned this pull request Oct 18, 2017
Closed
@spalger spalger deleted the fix/limit-es-version-warnings branch October 18, 2019 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjcenizal cjcenizal cjcenizal approved these changes

Assignees

@cjcenizal cjcenizal

@jbudz jbudz

Labels
review v5.0.1 v5.1.1 v6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spalger @epixa @jbudz @cjcenizal