Add Leveled/Scoped logging facility #236
Conversation
Pull Request Test Coverage Report for Build 1592
💛 - Coveralls |
|
@mjmac are you still interested in working on this? No rush (would much rather have outside contributors) but would love to see this implemented! |
|
Hi! Yes, sorry... I've gotten pulled off onto other things for the moment. I should have some downtime this coming week to clean this up and rework it. I think we can combine the approaches @aguilEA and I took to make something useful pretty quickly. I still think that we don't need a full-featured user-facing logging solution, because user-focused logging should ideally happen in the callbacks. But I think we can start with something which is better than nothing and iterate from there. |
|
@mjmac no worries at all! We all have full time jobs, and with the holidays spare time is pretty hard to come by :) Totally agree though, I would love to merge an MVP just so we don't have no rush |
mjmac
force-pushed
the
debugLogger
branch
3 times, most recently
from
c0d487e
to
0868ece
Compare
|
OK, feeling pretty good about this now. Trying to strike a balance between configurability and convention. The most important thing to me is that we should be able to sprinkle low-level logging statements throughout the code without worrying about performance impact when we're not actively debugging. |
mjmac
force-pushed
the
debugLogger
branch
5 times, most recently
from
f7ac26a
to
4bed034
Compare
| return registry.loggers[scope] | ||
| } | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
What do you think of using the settings engine to init this instead? https://github.com/pions/webrtc/blob/master/settingengine.go
I see this as the our way of setting non-standard WebRTC things, also makes it easier to demo in our examples/ I would have no problem parsing args/env variables in the examples themselves though
There was a problem hiding this comment.
Yea, i'd also prefer avoiding environment variables in the library.
There was a problem hiding this comment.
one pro-environment-variable argument is this: Gstreamer (and glib) do it, so people used to working with multimedia packages from these libraries will expect environment variables (or at least it's a sideways move)
As long as it's well documented...
There was a problem hiding this comment.
I don't love it, to be honest. It doesn't feel very ergonomic from a developer perspective. Environment variables have a long history of use for this sort of thing on unix-like platforms. It would be really annoying to me to have to fiddle with a configuration file every time I wanted to tweak logging while I was hacking on something.
That having been said, I'm open to the idea of a compromise that enables both use cases. I don't mind making the internals a bit more complicated in order to make life easier for users and developers.
How would the settings engine work for this? If you guys would propose examples of the way you would like to be able to configure things I can figure out how to implement it.
There was a problem hiding this comment.
I added some package-level functions to allow for more imperative control of logging behavior:
logging.SetDefaultWriter(w io.Writer) -- sets a package-level default logging destination; if a given logger was not initialized with its own writer, then it will use this, and it is safe to change at runtime
logging.SetLogLevelForScope(scope string, level LogLevel) -- sets the desired logging level for the given scope
I think with those, user code should be able to tweak logging behavior without having to use or know about environment variables. Thoughts?
mjmac
force-pushed
the
debugLogger
branch
2 times, most recently
from
98b1384
to
b5211a7
Compare
There was a problem hiding this comment.
I am 100% with merging this, in the future we can extend/figure out the best way to do
- Logging at the RTCPeerConnection level
- Possibly allow users to set the message callback (I want to exit the first time an error happens something like
G_DEBUG=fatal_warnings)
I think the version right now is a two way door though, nothing is stopping us from coming back :) again thank you so much for working on this feature, this is going to completely unblock some people who aren't able to debug ICE issues currently.
I would also keep in mind that we might be moving ice to its own package github.com/pions/ice, so we might need to pass the logger in a constructor. I was thinking we might need to do this for all 'pions' libraries (DTLS, SCTP....)
Allow package-scoped logging at user-defined levels. Logs are at ERROR by default, for critical errors which should be handled by users. More verbose logging may be enabled via combinations of environment variables, e.g. PIONS_LOG_TRACE=ice PIONS_LOG_DEBUG=pc,dtls PIONS_LOG_INFO=all <cmd> Relates to pion#222
|
OK, I can merge it as-is and we can iterate some more as we find out how we want to use it. I wonder if we should just put this in its own separate package, though. There's nothing webrtc-specific about it. e.g. pions/logging? |
Not sure about this yet, going to live with it for a bit locally to see how it feels. Updated the Ice agent with debug logging as an example.