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.

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

replace System.out.println with LOGGER #451

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

mawinter69
Copy link
Contributor

The System.out.println ends up on stdout of the controller jvm but almost all other logout will end on stderr.
Also the formatting of the timestamp is hard coded and usually differs from the normal logging output format.
By using the LOGGER we get well formatted log messages with a proper log level which makes it easier for log analyzers.

jenkins.log
Before:

2023-11-10 22:02:58.987+0000 [id=33]	INFO	hudson.lifecycle.Lifecycle#onReady: Jenkins is fully up and running
[11/10/23 23:02:59] SSH Launch of linuxaarch64 on redacted failed in 416 ms
2023-11-10 22:03:10.879+0000 [id=104]	INFO	hudson.slaves.SlaveComputer#tryReconnect: Attempting to reconnect linuxaarch64
[11/10/23 23:03:11] SSH Launch of linuxaarch64 on redacted failed in 469 ms
[11/10/23 23:03:12] SSH Launch of linuxx86_64 on redacted completed in 17.831 ms

After:

2023-11-10 23:31:28.268+0000 [id=83]	INFO	jenkins.InitReactorRunner$1#onAttained: Completed initialization
2023-11-10 23:31:28.419+0000 [id=33]	INFO	hudson.slaves.SlaveComputer#tryReconnect: Attempting to reconnect linuxaarch64
2023-11-10 23:31:28.726+0000 [id=33]	INFO	hudson.lifecycle.Lifecycle#onReady: Jenkins is fully up and running
2023-11-10 23:31:28.804+0000 [id=131]	WARNING	h.plugins.sshslaves.SSHLauncher#launch: SSH Launch of linuxaarch64 on redacted failed in 375 ms

The completed message is missing now from the log by default (due to Level.FINE)

Submitter checklist

  • JIRA issue is well described
  • Appropriate autotests or explanation to why this change has no tests

The System.out.println ends up on stdout of the controller jvm but
almost all other logout will end on stderr.
Also the formatting of the timestamp is hard coded and usually differs from
the normal logging output format.
By using the LOGGER we get well formatted log messages and all is in
stderr.
@mawinter69 mawinter69 requested a review from a team as a code owner November 11, 2023 00:07
@kuisathaverat
Copy link
Contributor

I have rejected this changes several times. The stdout of a remote agent it is not the same output that uses a logger. You changes only work if your agent is runnin in the Jenkins Controler, if the agent is in other machine you will lost those messages.

@mawinter69
Copy link
Contributor Author

mawinter69 commented Nov 11, 2023

Actually the changes are in the launch method which is code that runs on the controller not the agent. 2 of the 3 System.out are about failure to launch the agent so how should this ever be code that runs on the agent jvm? Looking at the plugin code I don't see any code at all that runs on the agent.
Currently all these messages appear in the controller log (see my before above), and they do not appear in the agent log itself.
Any my PR doesn't change that, the messages still appear in the controller log as before.

@kuisathaverat
Copy link
Contributor

I will repeat my self again #273 (comment)

#273
#253

@mawinter69
Copy link
Contributor Author

How is the behaviour of remoting here of any interest? You use System.out.println for the error case where you have no channel!
As said before: the code where the System.out statements are located is running in the controller jvm. If that would be running in the agent jvm I would say yes, it is ok to use System.out.println to make sure it is transferred to the agents log. But the System.out statements are only in the controller log and not in the agents log currently.
The 2 mentioned PRs changed the output to appear in the agent log and not the controller log.

@mawinter69
Copy link
Contributor Author

I have rejected this changes several times. The stdout of a remote agent it is not the same output that uses a logger. You changes only work if your agent is runnin in the Jenkins Controler, if the agent is in other machine you will lost those messages.

I tested my changes and my agent is not running in the Jenkins controller and the output is still there in the controller logs as before.

@kuisathaverat
Copy link
Contributor

sorry for the delay, I found time to test your changes and see if everything is fine, I have tested it with the latest LTS version. For some reason, I thought those messages were sent to the remote channel(stdout in the agent process) but it is not so the change to use a logger is a nice improvement.

https://github.com/kuisathaverat/jenkins-issues/tree/main/pr-451

@kuisathaverat kuisathaverat merged commit d0c264a into jenkinsci:main Nov 17, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants