Logging: Configure the node name when we have it #32983

nik9000 · 2018-08-20T12:07:07Z

Change the logging infrastructure to handle when the node name isn't
available in elasticsearch.yml. In that case the node name is not
available until long after logging is configured. The biggest change is
that the node name logging no longer fixed at pattern build time.
Instead it is read from a SetOnce on every print. If it is unset it is
printed as unknown so we have something that fits in the pattern.
On normal startup we don't log anything until the node name is available
so we never see the unknowns but we will see if if something fails
catastrophically before the we've recovered the node id or if you set
the log level to DEBUG or TRACE.

Closes #32793

Change the logging infrastructure to handle when the node name isn't available in `elasticsearch.yml`. In that case the node name is not available until long after logging is configured. The biggest change is that the node name logging no longer fixed at pattern build time. Instead it is read from a `SetOnce` on every print. If it is unset it is printed as `-----` so we have something that fits in the pattern. On normal startup we don't log anything until the node name is available so we never see the `-----`s.

elasticmachine · 2018-08-20T12:07:09Z

Pinging @elastic/es-core-infra

nik9000 · 2018-08-20T12:59:40Z

Failure is legit a thing I hadn't properly fixed up.

rjernst

This contains a lot of hacks that muck up node initialization ordering. For example, we have slowly been removing places where we initialize temporary copies of environment/settings, yet this adds to it.

Instead, I think we could move nodeid reading/generation out of NodeEnvironment? It is a trivial method, and it could be passed into NodeEnvironment once we have read it at the beginning of Node construction.

rjernst · 2018-08-20T18:18:50Z

server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java

-        NodeNamePatternConverter.setNodeName(nodeName);
+        /*
+         * Initialize the node name early if was set in elasticsearch.yml.
+         * If it wasn't, we'll set it further in startup.


nit: further in startup -> later in startup?

rjernst · 2018-08-20T18:20:21Z

server/src/main/java/org/elasticsearch/common/logging/NodeNamePatternConverter.java

+             * This is ok because we don't generally log anything before
+             * we have the node name.
+             */
+            nodeName = "-------";


maybe something like notset or unknown would be more obvious to users what is going on?

yeah. I'll see if I can do that.

rjernst · 2018-08-20T18:29:12Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy

                'pidfile'                      : node.pidFile,
                'path.repo'                    : "${node.sharedDir}/repo",
                'path.shared_data'             : "${node.sharedDir}/",
                // Define a node attribute so we can test that it exists
                'node.attr.testattr'           : 'test'
        ]
+        if (node.config.addStandardNodeName) {


Could we do this without any new setting by changing the injection of settings to remove/omit when the value is null? This would then generally allow any settings to be removed.

I think that sounds more confusing then adding the setting. I mean, it is more flexible but I don't know that we need that flexibility.

Fewer settings on NodeConfiguration is better, IMO. It makes less logic on node setup.

nik9000 · 2018-08-20T18:32:09Z

I think we could move nodeid reading/generation out of NodeEnvironment?

I'll investigate that.

nik9000 · 2018-08-21T18:34:15Z

I think we could move nodeid reading/generation out of NodeEnvironment?

I'll investigate that.

A few of us talking and settled on a way to do move the automatic node name generation way earlier in the startup process but that way can't be backported to 6.5 and something must be backported because we're broken now if node name isn't in elasticsearch.yml. So we said that we were ok with the Consumer<String> hack that I used here, at least for now. With the intent that I'll propose something that'll remove it in master and it'll stay in 6.x forever.

nik9000 · 2018-08-21T18:35:20Z

server/src/main/java/org/elasticsearch/common/logging/NodeNamePatternConverter.java

-public class NodeNamePatternConverter extends LogEventPatternConverter {
-    private static final SetOnce<String> NODE_NAME = new SetOnce<>();
+public final class NodeNamePatternConverter extends LogEventPatternConverter {
+    /**


I'm not happy with this at all but it is another thing that I think we'll be able to remove it master in a follow up.

@jasontedor could you take a look at this when you get a chance? I'm well aware it isn't good but I'm not clear on whether it is disastrous or simply "something we shouldn't keep around for long".

I talked with @jasontedor and he'd prefer the volatile read over this so I'm switching back to it.

This reverts commit de97aad.

That doesn't work with the evil tests.

nik9000 · 2018-08-24T16:33:37Z

@jasontedor this is ready for you to have a look now.

nik9000 · 2018-09-05T13:53:31Z

@jasontedor could you have a look at this when you get a chance? I'm going to resolve the merge conflict on it now.

jasontedor

LGTM. One nit, I'm sorry.

jasontedor · 2018-09-07T08:41:21Z

...figured-node-name/src/test/java/org/elasticsearch/unconfigurednodename/NodeNameInLogsIT.java

+ * under the License.
+ */
+
+package org.elasticsearch.unconfigurednodename;


One nit, so sorry, can you change the suffix on this package name to unconfigured_node_name.

I always thought package names with underscores was somehow discouraged but I checked around and that doesn't seem to be the case. Adding the underscores makes it easier to read too so I'm just going to have to get used to them. I'll switch it like you ask.

Change the logging infrastructure to handle when the node name isn't available in `elasticsearch.yml`. In that case the node name is not available until long after logging is configured. The biggest change is that the node name logging no longer fixed at pattern build time. Instead it is read from a `SetOnce` on every print. If it is unset it is printed as `unknown` so we have something that fits in the pattern. On normal startup we don't log anything until the node name is available so we never see the `unknown`s.

nik9000 · 2018-09-08T01:23:41Z

Thanks for reviewing @jasontedor! I've finished the backport. I'll start work on the master-only clean up follow up to this on Monday.

jasontedor · 2018-09-08T02:05:07Z

Thanks a lot @nik9000, this is great work and I am sorry that it took some time for me to get to a review.

nik9000 · 2018-09-08T02:05:45Z

Good things take time.

* master: (30 commits) Include fallback settings when checking dependencies (elastic#33522) [DOCS] Fixing formatting issues in breaking changes CRUD: Disable wait for refresh tests with delete Test: Fix test name (elastic#33510) HLRC: split ingest request converters (elastic#33435) Logging: Configure the node name when we have it (elastic#32983) HLRC: split xpack request converters (elastic#33444) HLRC: split watcher request converters (elastic#33442) HLRC: add enable and disable user API support (elastic#33481) [DOCS] Fixes formatting error TEST: Ensure merge triggered in _source retention test (elastic#33487) [ML] Add a file structure determination endpoint (elastic#33471) HLRC: ML Forecast Job (elastic#33506) HLRC: split migration request converters (elastic#33436) HLRC: split snapshot request converters (elastic#33439) Make Watcher validation message copy/pasteable Removes redundant test method in SQL tests (elastic#33498) HLRC: ML Post Data (elastic#33443) Pass Directory instead of DirectoryService to Store (elastic#33466) Collapse package structure for metrics aggs (elastic#33463) ...

Back in #32983 I broke running the integ-test-zip tests against an external cluster by adding a test that reads the contents of the log file. This fixes running against an external cluster by explicitly skipping that test if running against an external cluster.

nik9000 added 2 commits August 18, 2018 07:42

Merge branch 'master' into logger_node_name_not_set

20fc7cc

nik9000 added >bug blocker :Core/Infra/Logging Log management and logging utilities v7.0.0 v6.5.0 labels Aug 20, 2018

nik9000 requested a review from rjernst August 20, 2018 12:07

nik9000 added 2 commits August 20, 2018 11:56

Merge branch 'master' into logger_node_name_not_set

fe9f0d7

Cleanup

7d8ce29

rjernst reviewed Aug 20, 2018

View reviewed changes

nik9000 added 5 commits August 20, 2018 15:26

Unknown

2ef93a0

Remove volatile

1b4eb20

Cleanup

85f722e

Redo settings changes

d9bf742

Comment

de97aad

nik9000 commented Aug 21, 2018

View reviewed changes

nik9000 added 6 commits August 21, 2018 16:52

Merge branch 'master' into logger_node_name_not_set

8c01b28

Merge branch 'master' into logger_node_name_not_set

bd42188

Revert "Comment"

3aa19f7

This reverts commit de97aad.

Switch back to volatile read

2fdfe51

Remove init from configureLogging

04eca10

That doesn't work with the evil tests.

Revert weird extra

3685ddd

Merge branch 'master' into logger_node_name_not_set

6013c50

jasontedor approved these changes Sep 7, 2018

View reviewed changes

nik9000 added 2 commits September 7, 2018 09:58

Merge branch 'master' into logger_node_name_not_set

9ba6da6

Rename package

c374a3d

nik9000 merged commit 190ea9a into elastic:master Sep 7, 2018

nik9000 added the backport pending label Sep 7, 2018

nik9000 removed the backport pending label Sep 8, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging: Configure the node name when we have it #32983

Logging: Configure the node name when we have it #32983

nik9000 commented Aug 20, 2018 •

edited

Loading

elasticmachine commented Aug 20, 2018

nik9000 commented Aug 20, 2018

rjernst left a comment

rjernst Aug 20, 2018

rjernst Aug 20, 2018

nik9000 Aug 20, 2018

rjernst Aug 20, 2018

nik9000 Aug 20, 2018

rjernst Aug 20, 2018

nik9000 commented Aug 20, 2018

nik9000 commented Aug 21, 2018

nik9000 Aug 21, 2018

nik9000 Aug 21, 2018

nik9000 Aug 24, 2018

nik9000 commented Aug 24, 2018

nik9000 commented Sep 5, 2018

jasontedor left a comment

jasontedor Sep 7, 2018

nik9000 Sep 7, 2018

nik9000 commented Sep 8, 2018

jasontedor commented Sep 8, 2018

nik9000 commented Sep 8, 2018

Logging: Configure the node name when we have it #32983

Logging: Configure the node name when we have it #32983

Conversation

nik9000 commented Aug 20, 2018 • edited Loading

elasticmachine commented Aug 20, 2018

nik9000 commented Aug 20, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Aug 20, 2018

nik9000 commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Aug 24, 2018

nik9000 commented Sep 5, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Sep 8, 2018

jasontedor commented Sep 8, 2018

nik9000 commented Sep 8, 2018

nik9000 commented Aug 20, 2018 •

edited

Loading