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

Add Leveled/Scoped logging facility #236

Merged
merged 1 commit into from
Dec 30, 2018
Merged

Add Leveled/Scoped logging facility #236

merged 1 commit into from
Dec 30, 2018

Conversation

mjmac
Copy link
Contributor

@mjmac mjmac commented Nov 19, 2018

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.

@coveralls
Copy link

coveralls commented Nov 19, 2018

Pull Request Test Coverage Report for Build 1592

  • 143 of 201 (71.14%) changed or added relevant lines in 6 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 73.112%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/ice/candidate.go 0 2 0.0%
pkg/ice/candidatepair.go 0 2 0.0%
rtcpeerconnection.go 5 9 55.56%
pkg/ice/agent.go 8 21 38.1%
pkg/logging/leveled.go 92 106 86.79%
pkg/logging/scoped.go 38 61 62.3%
Files with Coverage Reduction New Missed Lines %
pkg/ice/agent.go 1 78.51%
Totals Coverage Status
Change from base Build 1583: 0.3%
Covered Lines: 3834
Relevant Lines: 5244

💛 - Coveralls

@Sean-Der
Copy link
Member

@mjmac are you still interested in working on this? No rush (would much rather have outside contributors) but would love to see this implemented!

@mjmac
Copy link
Contributor Author

mjmac commented Dec 21, 2018

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.

@Sean-Der
Copy link
Member

@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 fmt.Println and fmt.Printf sprinkled through the library. Users get concerned with the tracing messages we have, and would make development so much easier as well.

no rush

@backkem backkem changed the title WIP: Debug logger [WIP] Debug logger Dec 24, 2018
@mjmac mjmac force-pushed the debugLogger branch 3 times, most recently from c0d487e to 0868ece Compare December 27, 2018 20:57
@mjmac mjmac changed the title [WIP] Debug logger Add Leveled/Scoped logging facility Dec 27, 2018
@mjmac
Copy link
Contributor Author

mjmac commented Dec 27, 2018

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 mjmac force-pushed the debugLogger branch 5 times, most recently from f7ac26a to 4bed034 Compare December 28, 2018 02:47
return registry.loggers[scope]
}

func init() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjmac @backkem

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, i'd also prefer avoiding environment variables in the library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mjmac force-pushed the debugLogger branch 2 times, most recently from 98b1384 to b5211a7 Compare December 28, 2018 16:05
Copy link
Member

@Sean-Der Sean-Der left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@mjmac mjmac merged commit b98909a into pion:master Dec 30, 2018
@mjmac mjmac deleted the debugLogger branch December 30, 2018 16:15
@mjmac
Copy link
Contributor Author

mjmac commented Dec 30, 2018

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants