Allow daemon to log to stdout as journald can still log the output #2591

mtorromeo · 2021-03-19T15:32:23Z

When trying to use janus within a systemd service unit, the best option is to use the forking type which means starting janus with -b to "daemonize" the process.

Even if I do this I don't want to log the process output to a file as journald is already capable of storing and timestamping the log from the process' stdout, as demonstrated by this journal logs (captured with a patched janus):

Mar 19 16:41:51 myserver systemd[1]: Starting Janus WebRTC Server...
Mar 19 16:41:51 myserver janus[1306664]: Janus commit: not-a-git-repo
Mar 19 16:41:51 myserver janus[1306664]: Compiled on:  Fri Mar 19 15:41:32 UTC 2021
Mar 19 16:41:51 myserver janus[1306664]: Running Janus as a daemon
Mar 19 16:41:51 myserver janus[1306664]: Logger plugins folder: /usr/lib64/janus/loggers
Mar 19 16:41:51 myserver janus[1306664]: [WARN]      Couldn't access logger plugins folder...
Mar 19 16:41:51 myserver janus[1306664]: ---------------------------------------------------
Mar 19 16:41:51 myserver janus[1306664]:   Starting Meetecho Janus (WebRTC Server) v0.10.10
Mar 19 16:41:51 myserver janus[1306664]: ---------------------------------------------------
Mar 19 16:41:51 myserver janus[1306664]: Checking command line arguments...
Mar 19 16:41:51 myserver janus[1306664]: Debug/log level is 4
Mar 19 16:41:51 myserver janus[1306664]: Debug/log timestamps are disabled
Mar 19 16:41:51 myserver janus[1306664]: Debug/log colors are disabled
Mar 19 16:41:51 myserver janus[1306664]: Adding 'vmnet' to the ICE ignore list...
Mar 19 16:41:51 myserver janus[1306664]: Using X.X.X.X as local IP...
Mar 19 16:41:51 myserver janus[1306664]: Token based authentication disabled
Mar 19 16:41:51 myserver janus[1306664]: Initializing recorder code
Mar 19 16:41:51 myserver janus[1306664]: Initializing ICE stuff (Full mode, ICE-TCP candidates disabled, half-trickle, IPv6 support disabled)
Mar 19 16:41:51 myserver janus[1306664]: TURN REST API backend: (disabled)
Mar 19 16:41:51 myserver janus[1306664]: Crypto: OpenSSL >= 1.1.0
...

Janus instead aborts the process lamenting the following:

Running Janus as a daemon but no log file provided, giving up...

BTW I think that I should be allowed to not log to anything if I really wanted to but this is beside the point.

januscla · 2021-03-19T15:32:24Z

Thanks for your contribution, @mtorromeo! Please make sure you sign our CLA, as it's a required step before we can merge this.

lminiero · 2021-03-19T16:15:32Z

I think this will break generic "putting into background", that is, in contexts different than yours: IIRC we had to close stdout/stderr in that case, because otherwise we would keep on printing even when put into background.

lminiero · 2021-03-19T16:16:52Z

BTW I think that I should be allowed to not log to anything if I really wanted to but this is beside the point.

You are allowed not to log anything:

	#log_to_stdout = false					# Whether the Janus output should be written
											# to stdout or not (default=true)
	#log_to_file = "/path/to/janus.log"		# Whether to use a log file or not

If you set log_to_stdout to FALSE in janus.jcfg and don't provide any file, we won't log anything.

mtorromeo · 2021-03-19T16:27:24Z

If you set log_to_stdout to FALSE in janus.jcfg and don't provide any file, we won't log anything.

not while demonizing, unless I'm misinterpreting something.

There's this check that blocks the process in such a situation:

	if(daemonize && logfile == NULL) {
		g_print("Running Janus as a daemon but no log file provided, giving up...\n");
		exit(1);
	}

But, that's not what I want anyway. I want to log but to the journal (via stdout or otherwise).

This is only my opinion but manually sending a process to background doesn't seem like the ideal use case, and you can always explicitly configure "log_to_stdout = false" if you need to (like in a rc.d style init script), but the current implementation doesn' allow me to explicitly use "log_to_stdout = true".

What about a flag to force outputting to stdout then?

lminiero · 2021-03-19T16:39:12Z

What about a flag to force outputting to stdout then?

That sounds like a more acceptable solution, as it wouldn't impact existing deployments, and should give you the flexibility you need.

Re: this check,

	if(daemonize && logfile == NULL) {
		g_print("Running Janus as a daemon but no log file provided, giving up...\n");
		exit(1);
 	}

I guess it can be removed, since as you pointed out people may not want any logging at all.

mtorromeo · 2021-03-31T12:13:29Z

Would this new implementation work for you?

lminiero

Much more compact, thanks! I added a few notes inline.

lminiero · 2021-04-01T16:04:32Z

janus.ggo

@@ -2,6 +2,7 @@
 option "daemon" b "Launch Janus in background as a daemon" flag off
 option "pid-file" p "Open the specified PID file when starting Janus (default=none)" string typestr="path" optional
 option "disable-stdout" N "Disable stdout based logging" flag off
+option "log-stdout" - "Log to stdout" flag off


Not sure that - is the right one-char shortcut here: wouldn't it be confused with what - usually stands for in scripts and applications? Of course, I do realize almost all letters of the alphabet are gone, we used too many 😄

A couple of side notes:

For each command-line property there should be a janus.jcfg equivalent

New command line properties should be documented in janus.1 (the man file) and added to the summary in the README as well

Not sure that - is the right one-char shortcut here

- means "no short option". Quoting the info page of gengetopt:

'short' The short option, a single upper or lower case char, or a digit. If a '-' is specified, then no short option is considered for the long option (thus long options with no associated short options are allowed). Since version 2.22 you can also specify '?' as the short option.

For each command-line property there should be a janus.jcfg equivalent

There is already a log_to_stdout options in janus.jcfg but it defaults to true while the command line flag defaults to false. This issue exists only because I had to preserve the current behaviour where daemonize would disable stdout even if log_to_stdout was set to true.

Should I just rename the command line flag to --log-to-stdout instead of --log-stdout and leave everything else as-is?

- means "no short option". Quoting the info page of gengetopt:

Ah thanks for the heads up, I didn't know about that!

There is already a log_to_stdout options in janus.jcfg but it defaults to true while the command line flag defaults to false. This issue exists only because I had to preserve the current behaviour where daemonize would disable stdout even if log_to_stdout was set to true.

In that case I think it may be worth making it clearer in the description of the property should that this is a way to make sure stdout remains alive even, or especially, when demonizing (since that's basically the only thing it's used for). The "Log to stdout" text doesn't make that clear enough IMO.

Should I just rename the command line flag to --log-to-stdout instead of --log-stdout and leave everything else as-is?

Makes sense!

I did change the --log-stdout flag to --log-to-stdout in the end. That's because the analogous configuration log_to_file has the matching flag --log-file and I think it makes sense to keep it consistent.

Are you sure the right changes were pushed? I still see the old name. Besides I see the updated description in the man-file, but not in the .ggo one?

That's what I was explaining.

I thought that changing --log-stdout to --log-to-stdout was not consistent with --log-file that is NOT named --log-to-file despite its configuration option being log_to_file.

So I did NOT change anything here. Do you still want to rename it even in light of this argument?

Sorry, maybe you were referring to the description and not the name. I just pushed the update for that as well.

Do you still want to rename it even in light of this argument?

No, I'm fine with that. I asked because you wrote:

I did change the --log-stdout flag to --log-to-stdout in the end.

but the name was still the old one. Did you mean "I didn't change... in the end"?
Description looks good, thanks!

Did you mean "I didn't change... in the end"?

Oh, yeah, didn't notice the typo. Sorry about that.

mtorromeo · 2021-04-02T10:21:37Z

janus.1

+.BR \-L ", " \-\-log-stdout
+Log to stdout, even when the process is daemonized (default=off)
+.TP


@lminiero Is this description good enough?

…monizing the process

lminiero · 2021-04-02T10:44:17Z

Changes look fine to me! If you can confirm it now addresses your requirement, it's good to merge for me 👍

mtorromeo · 2021-04-02T10:51:08Z

Yes, the patch has been tested and works as intended

mtorromeo · 2021-04-02T10:52:45Z

For context, this is the systemd service file I am using:

[Unit]
Description=Janus WebRTC Server
Documentation=https://janus.conf.meetecho.com/
After=network.target

[Service]
Type=forking
User=janus
ExecStart=/usr/bin/janus -ob --log-stdout
Restart=on-failure
LimitNOFILE=65536

[Install]
WantedBy=multi-user.target

lminiero · 2021-04-02T15:40:02Z

Merging then, thanks!

mtorromeo force-pushed the allow-daemon-stdout branch 2 times, most recently from a9e0c30 to 4003b67 Compare March 19, 2021 15:46

mtorromeo force-pushed the allow-daemon-stdout branch from 4003b67 to 269721d Compare March 31, 2021 12:12

lminiero reviewed Apr 1, 2021

View reviewed changes

mtorromeo force-pushed the allow-daemon-stdout branch from 269721d to 11c289a Compare April 2, 2021 10:18

mtorromeo commented Apr 2, 2021

View reviewed changes

Added new --log-stdout flag that enabled stdout logging even when dae…

2180a67

…monizing the process

mtorromeo force-pushed the allow-daemon-stdout branch from 11c289a to 2180a67 Compare April 2, 2021 10:30

lminiero merged commit 4dd379a into meetecho:master Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow daemon to log to stdout as journald can still log the output #2591

Allow daemon to log to stdout as journald can still log the output #2591

mtorromeo commented Mar 19, 2021 •

edited

Loading

januscla commented Mar 19, 2021

lminiero commented Mar 19, 2021

lminiero commented Mar 19, 2021

mtorromeo commented Mar 19, 2021

lminiero commented Mar 19, 2021

mtorromeo commented Mar 31, 2021

lminiero left a comment

lminiero Apr 1, 2021

mtorromeo Apr 1, 2021

lminiero Apr 1, 2021 •

edited

Loading

mtorromeo Apr 2, 2021

lminiero Apr 2, 2021 •

edited

Loading

mtorromeo Apr 2, 2021

mtorromeo Apr 2, 2021

lminiero Apr 2, 2021

mtorromeo Apr 2, 2021

mtorromeo Apr 2, 2021

lminiero commented Apr 2, 2021

mtorromeo commented Apr 2, 2021

mtorromeo commented Apr 2, 2021

lminiero commented Apr 2, 2021

Allow daemon to log to stdout as journald can still log the output #2591

Allow daemon to log to stdout as journald can still log the output #2591

Conversation

mtorromeo commented Mar 19, 2021 • edited Loading

januscla commented Mar 19, 2021

lminiero commented Mar 19, 2021

lminiero commented Mar 19, 2021

mtorromeo commented Mar 19, 2021

lminiero commented Mar 19, 2021

mtorromeo commented Mar 31, 2021

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero Apr 2, 2021 • 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

lminiero commented Apr 2, 2021

mtorromeo commented Apr 2, 2021

mtorromeo commented Apr 2, 2021

lminiero commented Apr 2, 2021

mtorromeo commented Mar 19, 2021 •

edited

Loading

lminiero Apr 1, 2021 •

edited

Loading

lminiero Apr 2, 2021 •

edited

Loading