[WIP] RTCPeerConnection Logging #223
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| package logging | ||
|
|
||
| import ( | ||
| "io/ioutil" | ||
| "log" | ||
| "os" | ||
| ) | ||
|
|
||
| type logLevel int | ||
|
|
||
| const( | ||
| Error = iota + 1 | ||
| Warn | ||
| Info | ||
| Debug | ||
| Trace | ||
| ) | ||
|
|
||
| // RTCPeerLogger is the default logging struct for rtcpeerconnection. | ||
| type RTCPeerLogger struct{ | ||
| info, debug, warning, | ||
| trace, error *log.Logger | ||
| } | ||
|
|
||
| // webrtcLogger is the interface that must be implemented if a custom | ||
| // logger is used. | ||
| type webrtcLogger interface{ | ||
|
There was a problem hiding this comment. I'm confused about what this interface is used for -- it's not exported, so it can only be used internally to the logging package. But I don't see any use of it in logging. There was a problem hiding this comment. I was setting it a loging interface in order to have method in RTCPeerConnection which accepts the interface, and the user can pass a struct that allows them to do logging however they want. Ultimately a user would call a function pc.setLogger(myCustomLogger). |
||
| Info(msg string) | ||
| Trace(msg string) | ||
| Debug(msg string) | ||
| Warning(msg string) | ||
| Error(msg string) | ||
| } | ||
|
|
||
| func (pclog *RTCPeerLogger) Debug(debug string){ | ||
| pclog.debug.Println(debug) | ||
|
There was a problem hiding this comment. This is clear and simple, and the use of ioutil.Discard as the default writer makes sense from a functionality perspective. I have concerns about this approach from a performance perspective, however. For occasional logging e.g. at the Info/Warning level, I think overhead would be negligible. For copious logging at the Trace/Debug levels, I'm sure that we would start to see a lot of wasted resources spent formatting things in Println() just to throw them away. Have a look at the source of printArg() to see how much work is happening behind the scenes -- it's pretty interesting. I would propose instead using SetLoglevel() to set a level in the logger and then only calling Println() for the appropriate levels. There was a problem hiding this comment. Yeah I read "Discard is an io.Writer on which all Write calls succeed without doing anything." and thought that it was not affecting performace. However if this is not the case then yeah we don't want to impact performance. This is my first time messing with the go logger, but if we can set a level in the logger and use that to determine what to print then that sounds good. |
||
| } | ||
|
|
||
| func (pclog *RTCPeerLogger) Error(error string){ | ||
| pclog.error.Println(error) | ||
| } | ||
|
|
||
| func (pclog *RTCPeerLogger) Info(info string){ | ||
| pclog.info.Println(info) | ||
| } | ||
|
|
||
| func (pclog *RTCPeerLogger) Warning(warning string){ | ||
| pclog.warning.Println(warning) | ||
| } | ||
|
|
||
| func (pclog *RTCPeerLogger) Trace(trace string){ | ||
| pclog.trace.Println(trace) | ||
| } | ||
|
|
||
| func (pclog *RTCPeerLogger) SetLogLevel(l logLevel) { | ||
|
There was a problem hiding this comment. This is a good first pass, but unfortunately it would reconfigure the logging destinations every time SetLogLevel() is called. If we're going to go down the road of setting separate loggers for each log level, then I think it would make more sense to do this setup in a NewLogger() function and keep SetLogLevel() focused on simply changing the active logging level. One way to make this nicely configurable might be to add With...() methods on your logger. So for example your NewLogger() would return a logger configured for opinionated and sane defaults, but that logger would have methods for doing things like setting the loglevel (e.g. .WithLogLevel(logging.Debug)), and logging destinations/configuration for various levels. For example, one might define a logger like so: log := logging.NewLeveledLogger().
WithLogLevel(logging.LogLevelDebug).
WithDebugLogger(log.New(os.Stderr, "custom ", log.Lmicroseconds|log.Llongfile|log.LUTC))
log.Debug("o hai")There was a problem hiding this comment. In the case that we do use levels for loging are you suggesting that we create logers like this? Tracelog := logging.NewLeveledLogger(). Debuglog := logging.NewLeveledLogger(). |
||
|
|
||
| debugOut := ioutil.Discard | ||
| infoOut := ioutil.Discard | ||
| traceOut := ioutil.Discard | ||
| warningOut := ioutil.Discard | ||
| errorOut := ioutil.Discard | ||
|
|
||
| switch l { | ||
| case Error: | ||
| errorOut = os.Stdout | ||
| case Warn: | ||
| errorOut = os.Stdout | ||
| warningOut = os.Stdout | ||
| case Info: | ||
| errorOut = os.Stdout | ||
| warningOut = os.Stdout | ||
| infoOut = os.Stdout | ||
| case Debug: | ||
| errorOut = os.Stdout | ||
| warningOut = os.Stdout | ||
| infoOut = os.Stdout | ||
| debugOut = os.Stdout | ||
| case Trace: | ||
| errorOut = os.Stdout | ||
| warningOut = os.Stdout | ||
| infoOut = os.Stdout | ||
| debugOut = os.Stdout | ||
| traceOut = os.Stdout | ||
| } | ||
|
|
||
| pclog.debug = log.New(debugOut, "DEBUG: ", log.Lshortfile) | ||
| pclog.info = log.New(infoOut, "INFO: ", log.Lshortfile) | ||
| pclog.trace = log.New(traceOut, "TRACE: ", log.Lshortfile) | ||
| pclog.warning = log.New(warningOut,"WARNING: ", log.Lshortfile) | ||
| pclog.error = log.New(errorOut, "ERROR: ", log.Lshortfile) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the overall goal should be to provide a generalized logging solution for various subsystems in pions-WebRTC, right? Rather than naming this RTCPeerLogger I'd suggest something like LeveledLogger? That's a little awkward to import as logging.LeveledLogger, but not the worst imo.