[WFCORE-6579] Allow process controller log file capture initial manag… #5789

yersan · 2023-12-04T11:17:47Z

…ed process Standard Out and Err

Jira issue: https://issues.redhat.com/browse/WFCORE-6579

yersan · 2023-12-04T11:18:51Z

CC @bstansberry / @jamezp I ended up with the above, let me know your thoughts, thanks!

wildfly-ci · 2023-12-04T11:26:45Z

Core -> WildFly Preview Integration Build 13125 outcome was UNKNOWN using a merge of e7e7033
Summary: Canceled (Exit code 143 (Step: Build & test full (Maven)) (new)) Build time: 00:06:40

wildfly-ci · 2023-12-04T11:26:46Z

Core -> Full Integration Build 13056 outcome was UNKNOWN using a merge of e7e7033
Summary: Canceled (Exit code 1 (Step: Build & test full (Maven))) Build time: 00:06:37

wildfly-ci · 2023-12-04T11:26:51Z

Core -> Full Integration Build 13285 outcome was UNKNOWN using a merge of e7e7033
Summary: Canceled (Tests passed: 1027, ignored: 5; exit code 143 (Step: Build & test full (Maven)) (new)) Build time: 00:06:45

yersan · 2023-12-04T11:36:09Z

server/src/main/java/org/jboss/as/server/DomainServerMain.java

+        // Signalling the Process Controller to switch also the System.err is optional since after installing StdioContext,
+        // there won't be more logs sent to the standard Err.


thinking a bit more, although optional, the aggregation of the StdError and Stout and where they are redirected will be handled by the logging.configuration file, so assuming that both will be redirected to the Stout is weak, since I guess that can be changed via the logging configuration file and instead of using handler.CONSOLE.target=SYSTEM_OUT use handler.CONSOLE.target=SYSTEM_ERR, so probably this should be changed.

jamezp · 2023-12-05T00:06:08Z

host-controller/src/main/java/org/jboss/as/host/controller/Main.java

+        // This message is not used to display any information on the standard output, it signals the Process Controller
+        // to switch the standard output of this managed process from the process controller log to the process controller
+        // standard output.
+        // Once the StdioContext gets installed, both the stout and stderr of this managed process will be captured,


stdout instead of stout.

jamezp · 2023-12-05T01:35:19Z

process-controller/src/main/java/org/jboss/as/process/ManagedProcess.java

+        logSystemErr = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".system.err");
+        logSystemOut = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".system.out");


Not critical, should we use stdout and stderr instead? I don't have a strong opinion, I'm just curious :)

We also might want to consider the order to have the process name last to make filtering easier. For example you could just skip stdout with logger.org.jboss.as.process.system.out.level=WARN

Not critical, should we use stdout and stderr instead?

I am fine with both. xxx.system.out and xxx.system.err are very intuitive to me due to the similitude with the Java methods, if xxx.system.stdout and xxx.system.stderr are more related to logging world, I am fine with them too. I do not have any preference.

We also might want to consider the order to have the process name last to make filtering easier. For example you could just skip stdout with logger.org.jboss.as.process.system.out.level=WARN

Thanks, that's a smarter choice. I agree with you.

We already have the .status category defined as "org.jboss.as.process." + processName + ".status". Should we change it too?

That would make all the related categories consistent, although would affect existing configurations. However, since are logging categories, any existing configuration can be changed to adapt to the new version and that would not affect server behaviour, so I am in favor of adapting those three.

@jamezp

I've changed the categories to xxx.system.stdout and xxx.system.stderr.

However, I left the "org.jboss.as.process." + processName + ".system.sterr" and "org.jboss.as.process." + processName + ".system.stout" as proposed originally since we already have "org.jboss.as.process." + processName + ".status" and users could be familiar with that category.

I prefer being conservative here since this is an enhancement and I don't want to introduce a change on the .status category or have categories working differently than the status one. I think the usage of them is unlikely, but if they are needed I think is preferable a consistent with what we previously have.

wildfly-ci · 2023-12-11T17:03:15Z

Core -> WildFly Preview Integration Build 13140 outcome was FAILURE using a merge of 5018b3a
Summary: Exit code 1 (Step: Build & test full (Maven)) (new) Build time: 00:02:04

wildfly-ci · 2023-12-11T17:03:15Z

Core -> Full Integration Build 13071 outcome was FAILURE using a merge of 5018b3a
Summary: Exit code 1 (Step: Build & test full (Maven)) (new) Build time: 00:02:06

wildfly-ci · 2023-12-11T17:04:28Z

Core -> Full Integration Build 13300 outcome was FAILURE using a merge of 5018b3a
Summary: Exit code 1 (Step: Build & test full (Maven)) (new) Build time: 00:03:15

bstansberry

I'll happily defer to you and @jamezp re the 'system.stdxxx' thing, but my input is just 'stdxxx' is best. Admins consume logs, and they may not even know about the Java System class. OTOH I expect a Java developer interested in domain logging would understand what stdout and stderr mean. Those are generic terms:

https://en.wikipedia.org/wiki/Standard_streams

bstansberry · 2023-12-11T17:22:59Z

process-controller/src/main/java/org/jboss/as/process/ManagedProcess.java

@@ -107,7 +125,9 @@ enum State {
        this.pcAuthKey = pcAuthKey;
        isPrivileged = privileged;
        respawnPolicy = respawn ? RespawnPolicy.RESPAWN : RespawnPolicy.NONE;
-        log = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".status");
+        logStatus = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".status");
+        logSystemErr = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".system.sterr");


s/sterr/stderr

Done, thanks!

bstansberry · 2023-12-11T17:23:17Z

process-controller/src/main/java/org/jboss/as/process/ManagedProcess.java

-        log = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".status");
+        logStatus = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".status");
+        logSystemErr = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".system.sterr");
+        logSystemOut = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".system.stout");


s/stout/stdout

+1 to

"org.jboss.as.process." + processName + ""

We don't want to mess with the existing 'status' and these should be consistent.

Done, thanks!

jamezp · 2023-12-11T17:38:58Z

process-controller/src/main/java/org/jboss/as/process/CommandLineConstants.java

@@ -121,6 +121,9 @@ public class CommandLineConstants {
    public static final String PREFER_IPV4_STACK = "java.net.preferIPv4Stack";
    public static final String PREFER_IPV6_ADDRESSES = "java.net.preferIPv6Addresses";

+    public static final String MANAGED_PROCESS_SYSTEM_ERROR_TO_LOG = "jboss.domain.managed-process.system.sterr.to.process-controller.log";


sterr should be stderr

Done, thanks!

jamezp · 2023-12-11T17:39:09Z

process-controller/src/main/java/org/jboss/as/process/CommandLineConstants.java

@@ -121,6 +121,9 @@ public class CommandLineConstants {
    public static final String PREFER_IPV4_STACK = "java.net.preferIPv4Stack";
    public static final String PREFER_IPV6_ADDRESSES = "java.net.preferIPv6Addresses";

+    public static final String MANAGED_PROCESS_SYSTEM_ERROR_TO_LOG = "jboss.domain.managed-process.system.sterr.to.process-controller.log";
+    public static final String MANAGED_PROCESS_SYSTEM_OUT_TO_LOG = "jboss.domain.managed-process.system.stout.to.process-controller.log";


stout should be stdout

Done, thanks!

…ed process Standard Out and Err Jira issue: https://issues.redhat.com/browse/WFCORE-6579

yersan · 2023-12-12T11:52:37Z

Thanks @jamezp and @bstansberry !

github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Dec 4, 2023

yersan force-pushed the WFCORE-6579 branch from e7e7033 to 6deb6e2 Compare December 4, 2023 11:27

yersan commented Dec 4, 2023

View reviewed changes

jamezp reviewed Dec 5, 2023

View reviewed changes

yersan force-pushed the WFCORE-6579 branch from 6deb6e2 to 5018b3a Compare December 11, 2023 16:57

yersan requested a review from jamezp December 11, 2023 16:57

bstansberry requested changes Dec 11, 2023

View reviewed changes

yersan force-pushed the WFCORE-6579 branch from 5018b3a to cd1ae9a Compare December 11, 2023 17:37

jamezp reviewed Dec 11, 2023

View reviewed changes

[WFCORE-6579] Allow process controller log file capture initial manag…

cf05f57

…ed process Standard Out and Err Jira issue: https://issues.redhat.com/browse/WFCORE-6579

yersan force-pushed the WFCORE-6579 branch from cd1ae9a to cf05f57 Compare December 11, 2023 17:50

jamezp approved these changes Dec 11, 2023

View reviewed changes

yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Dec 12, 2023

yersan merged commit f62fbf7 into wildfly:main Dec 12, 2023
12 checks passed

yersan deleted the WFCORE-6579 branch December 12, 2023 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFCORE-6579] Allow process controller log file capture initial manag… #5789

[WFCORE-6579] Allow process controller log file capture initial manag… #5789

yersan commented Dec 4, 2023

yersan commented Dec 4, 2023

wildfly-ci commented Dec 4, 2023

wildfly-ci commented Dec 4, 2023

wildfly-ci commented Dec 4, 2023

yersan Dec 4, 2023

yersan Dec 11, 2023

jamezp Dec 5, 2023

yersan Dec 11, 2023

jamezp Dec 5, 2023

yersan Dec 5, 2023

yersan Dec 11, 2023

wildfly-ci commented Dec 11, 2023

wildfly-ci commented Dec 11, 2023

wildfly-ci commented Dec 11, 2023

bstansberry left a comment •

edited

Loading

bstansberry Dec 11, 2023

yersan Dec 11, 2023

bstansberry Dec 11, 2023

bstansberry Dec 11, 2023

yersan Dec 11, 2023

jamezp Dec 11, 2023

yersan Dec 11, 2023

jamezp Dec 11, 2023

yersan Dec 11, 2023

yersan commented Dec 12, 2023

		// Signalling the Process Controller to switch also the System.err is optional since after installing StdioContext,
		// there won't be more logs sent to the standard Err.

		logSystemErr = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".system.err");
		logSystemOut = Logger.getMessageLogger(ProcessLogger.class, "org.jboss.as.process." + processName + ".system.out");

[WFCORE-6579] Allow process controller log file capture initial manag… #5789

[WFCORE-6579] Allow process controller log file capture initial manag… #5789

Conversation

yersan commented Dec 4, 2023

yersan commented Dec 4, 2023

wildfly-ci commented Dec 4, 2023

wildfly-ci commented Dec 4, 2023

wildfly-ci commented Dec 4, 2023

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

wildfly-ci commented Dec 11, 2023

wildfly-ci commented Dec 11, 2023

wildfly-ci commented Dec 11, 2023

bstansberry left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yersan commented Dec 12, 2023

bstansberry left a comment •

edited

Loading