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

New logging system (addresses #5874) #5921

Closed
wants to merge 3 commits into from
Closed

Conversation

ezrast
Copy link

@ezrast ezrast commented Apr 5, 2018

Here's my first stab at a better Logger class for the standard library as discussed in #5874. Control goes from Logger -> Handler -> Adapter, where Logger stores a class name and provides user-friendly methods, Handler filters by severity, and Adapter formats the message to an IO.

One detail I wasn't sure whether to address - how much do we care about backwards compatibility? As-is this breaks 100% of programs using Logger, but I can add a slightly ugly constructor that will preserve some of the simpler use cases.

@straight-shoota
Copy link
Member

This is a great initiative! I didn't look in every detail, but it generally looks good and gives us a basis for further discussions.

Backwards compatibility is no issue before 1.0 is release. But the question is rather, should the current, simple logger functionality be provided in the future as well? It could certainly be done by providing a simple API for singular Logger use. But I don't know about use cases... if there are any, it can be re-added later. For now it would probably be best to make a clear cut.

A few notes:

  • Logger.new(component) should better be Logger.get(component) to make clear that it does not create a new logger but rather taps into an existing logging system.
  • Part of the second commit 8913426 should be reverted. Adapter and Formatter need to be independent components. Combining their behaviour with inheritance means you can't reuse the same formatter for different backends which limits interchangeability.
  • At this point it would probably make sense to combine the individual log message parts in a struct Logger::Record, so Adapter can use the method signature write(record : Logger::Record) instead of passing in half a dozen arguments.

@ezrast
Copy link
Author

ezrast commented Apr 5, 2018

@straight-shoota, thanks for the feedback.
Right now the extremely simple use case is easier than on master; you just do logger = Logger.new; logger.info "Hello". But if you want to write to an IO other than STDERR that's more complicated. I'll probably go back and add a constructor that takes an IO and builds a handler with an appropriate IOAdapter automatically.

  • In the current implementation, if you do Logger.new("foo") twice, you will get two separate Logger objects. Since the objects are immutable this is an implementation detail, and we could replace new with a method that does a hash lookup and returns an existing Logger if possible, but should we? Why bother?
  • My assumption for getting rid of Formatter was that if you weren't using an IOAdapter you're probably doing something that requires formatting specific to your logging system anyway so it might as well be baked into the Adapter class. But I'll go ahead and revert this anyway; there are probably use cases here I'm not seeing.
  • I started to implement Logger::Record but then didn't bother because it only simplifies two methods (Adapter#write and Adapter#format) at the cost of adding code to two other methods (both forms of Handler#log). You can't build the Record earlier than Handler#log. I'll take a look and see how Java handles this - I do want to be able to add more context to a log record without it bloating all the method signatures.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 5, 2018

Ad 1) I didn't mean fetching a cached object. Logger.get should still initialize a new Logger instance (could probably even be a struct). I just propose to rename the constructor to .get to make clear it is not actually creating an independent logging setup. It's really just a minor detail about naming the API method.

Ad 2) There are probable use cases where you want to format a log record but not write it to an IO (send formatted log records as UDP messages). It would certainly be possible to use an IO wrapper for such an Adapter, but that's just bloating things up and defeats the purpose.

Ad 3) A record also simplifies usage in an adapter/formatter handlers. And it should also have serialization (JSON/YAML mapping) - but that can come in a later PR.

@RX14
Copy link
Member

RX14 commented Apr 6, 2018

I really think we should have a longer think and finalise design in #5874 before we put too much effort into this PR

@ezrast
Copy link
Author

ezrast commented Apr 10, 2018

@RX14 Agreed, I just wanted to put something down on paper (figuratively) for the purposes of discussion. I don't mind redoing things if this turns out not to be what we want.

@ezrast
Copy link
Author

ezrast commented Oct 2, 2018

Closing in favor of #6901

@ezrast ezrast closed this Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants