Log to stderr by default, not stdout

[adriancable]

Go's log package writes to stderr by default (like all/most loggers). However pion's logger by default writes to stdout by default, even though pion's packages that use the logger mostly (but not always) override this to stderr. So projects using both will end up with a mix of log messages to stdout and stderr, which isn't desirable when trying to separate out the log output. This PR changes the default logging fd to stderr, and also changes the logger test to be consistent with pion's usage in writing to stderr.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 53.43%. Comparing base (6d08bc6) to head (3575446).

Files	Patch %	Lines
logger.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #80   +/-   ##
=======================================
  Coverage   53.43%   53.43%           
=======================================
  Files           2        2           
  Lines         131      131           
=======================================
  Hits           70       70           
  Misses         59       59           
  Partials        2        2           

Flag	Coverage Δ
go	53.43% <0.00%> (ø)
wasm	53.43% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Sean-Der]

@adriancable I am all for more sensible defaults! Are we able to make this change with zero user impact? We can go through and make sure all the callers in Pion stay with stdout/stderr as they are now.

Should we tag this as /v2? I just want to make sure someone doesn't update and then lose all their logs.

[Sean-Der]

Commit message just got flagged as too long. Mind breaking this up into lines of 72 chars?

Log to stderr by default, not stdout

Go's log package writes to stderr by default (like all/most loggers). However pion's logger by default writes to stdout by default, even though pion's packages that use the logger override this to stderr. So projects using both will end up with a mix of log messages to stdout and stderr, which isn't desirable when trying to separate out the log output. The unit test for the pion logger also writes to stdout, which is inconsistent with pion's actual usage whereby the default output is overridden with stderr. This PR changes the default logging fd to stderr, and also changes the logger test to be consistent with pion's usage in writing to stderr.

[adriancable]

@Sean-Der - at least in the past, pion's own use of this was inconsistent. Some packages set the logger output to stderr, but some to stdout. I'll compile a list of what everything does right now, then we can decide what it should do. I think at least all pion's packages should log to the same fd.

[Sean-Der]

I agree they should log all to the same fd, but we can't change it on users in a minor/patch release (I think)

I think it is fair game for a major release though!

[adriancable]

@Sean-Der - I actually can't reproduce the original issue whereby some of pion's logging output was going to stdout instead of stderr. I swear I am not crazy! About 6 months ago I was having strange problems with an RTP stream being emitted on stdout, and when I looked at the output I found some pion logging stuff intermingled with the binary data on stdout. This would also have broken (maybe subtly) e.g. pkg/packetdump/packet_dumper.go in pion/interceptor, which also writes RTP data to stdout.

However now I can't reproduce it. Everything seems to go to stderr - which makes this PR a lot less important. Maybe this got fixed at the package level in the past 6 months or so?

[adriancable]

@Sean-Der - so I dug more into this. I now understand what's happening, but to be honest, I am not sure whether the right interpretation of the issue is 'operator error', or a Pion bug, or something else.

Summary: because I use stdout for my own stuff, I want to use stderr for logging. Because Pion's logger defaults to stdout (which I think isn't right, but as you say would be a breaking change - so definitely defer any change to a major release), I do:

s := webrtc.SettingEngine{}
...
loggerFactory := logging.NewDefaultLoggerFactory()
loggerFactory.Writer = os.Stderr
s.LoggerFactory = loggerFactory

This takes care of almost everything. But, interceptors don't know about / use the SettingEngine, so they still log to stdout no matter what, e.g. pkg/report/sender_interceptor.go. And, unlike for everything else there doesn't seem to be any way to tell the interceptor loggers to write to stderr instead.

So for my project the 'fix' was simply to stop using stdout myself, because it wasn't possible for me to guarantee stdout wouldn't be contaminated by pion's log output.

But I think the actual answer is to add an analogous option to the interceptors where we can specify a LoggerFactory. I'll look into this.

[Sean-Der]

Oh the pion/interceptor usage is a bug. It should respect the SettingEngine. I can help take that on :) I think that is a bug/worth fixing instantly. People are probably losing logs today because of it.

Yea the stderr is unfortunate. I am all for breaking it for /v4 I will start a wiki page of things we want for v4. I think it is worth doing!