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

Implement generalized event and log interfaces #272

Closed
wants to merge 2 commits into from

Conversation

Goblinlordx
Copy link

@Goblinlordx Goblinlordx commented Aug 28, 2018

Ref: #271

  • Add log package containing generalized internal (Event) and external (Logger) interfaces
  • Implement a fairly generalized and flexible set of events for usage inside package (event)
  • Implement a simple leveled default logger (leveled_logger)

This PR is more to get feedback on how you feel about this type of implementation. I feel I made it pretty flexible. The idea is, when the client is instantiated, the config would have a Logger property as it does already. I made a Logger interface for this so anyone can make their own Logger. The Logger only receives Events and handles them as desired.

The simplest implementation could have a NOOP style implementation of Log that would never log anything. A more complex version could introspect the event and do it's own filtering and formatting as desired.

Event is primarily for internal implementation. Types (which need to log something) can have a copy of the Logger passed to them when they are instantiated. After that they can easily call the Logger with an Event they created.

I added a few basic Event types to do some basic event logging and error logging.

Below is an example usage of the event and leveled_logger subpackages:

package main

import (
	"github.com/anacrolix/torrent/event"
	"github.com/anacrolix/torrent/leveled_logger"
)

func main() {
	l := leveled_logger.NewLeveledLogger(event.DebugLevel)
	l.Log(event.Debug("Test2"))
	l.Log(event.Info("test"))
	l.Log(event.Error("test"))
}

@anacrolix
Copy link
Owner

I don't really want an logger implemented in the torrent repo unless it has behaviour required specific to the repo. The level/filter stuff I did with github.com/anacrolix/log, perhaps you can look there and see if that meets your needs?

@Goblinlordx
Copy link
Author

Goblinlordx commented Aug 28, 2018

Logger is just to adapt events (internal interface) to an external interface. The Event implementation was needed to be able to pass level information (which is known at call site) through the interface without the Logger needing to know implementation details of the Event (it doesn't need to know levels were used to tag Events).

The Logger interface is what does the actual logging. leveled_logger is the only thing doing actual logging. Is that the only part you are referring to not having?

Also, I think ideally, none of this would be necessary and errors would be returned where necessary or events would passed out of the library via a provided callback. Essentially, the Logger interface I added provides that callback mechanism (which doesn't currently exist).

If a logger is provided without a simple interface (like is currently done) that means anyone providing there own needs to have a very large API surface to cover. The idea is that internally everything would only interact with the Logger (inside this library) via the very simple Logger interface.

The log package you mentioned is fine and can be used inside the default logger but the Logger is required to prevent implementation details (larger than the interface) from leaking inside the library.

@Goblinlordx
Copy link
Author

Goblinlordx commented Aug 28, 2018

To be clearer, only Event and Logger interfaces would be used internally. This allows the internal implementation to add level information without the Logger needing to handle or even know that the levels exist. The Logger can also be implemented without any knowledge of how Events are implemented (or if they have any associated data with them like level information.

I just did implement Events to cover most use cases I saw and an extremely simple Logger (leveled_logger) that aligns with the Logger interface so it could be used as a default implementation.

@Goblinlordx
Copy link
Author

All that said, event does not necessarily need to be part of this library but the log implementation you mentioned doesn't fit the need for having a small API surface for anyone to provide their own logger.

@anacrolix
Copy link
Owner

I think the idea is good, the logging in this repo (and possibly across most of my repos) is inadequate, but I haven't settled on a solution, and I don't think this is the one I want. Thanks for the contribution.

@anacrolix anacrolix closed this May 29, 2019
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.

2 participants