hyperledger-labs / go-perun

🌔 Perun's Blockchain-Agnostic State Channels Framework in Go.
https://perun.network/
Apache License 2.0
55 stars 18 forks source link

[log] WithPrefix Logger #18

Open ggwpez opened 3 years ago

ggwpez commented 3 years ago

Type  IMPROVEMENT

Affects[log]

Current situation
It is often difficult to tell multiple parties apart in the log output.
For example we have Alices logger:
log.WithField("role", "Alice").Info("test")
which produces:
INFO[0000] test role=Alice

Suggestion
I suggest to add a WithPrefix(string) function, such that the above would become
log.WithPrefix("Alice").Info("test") which produces:
INFO[0000] Alice: test

Recursively it would look like this:
log.WithPrefix("Alice").WithPrefix("client").Info("test"):
INFO[0000] Alice.client: test or INFO[0000] Alice: client: test

To simplify the above, a WithPrefixes([]string) or WithPrefices([]string) should be added.

Justification  Logs are more readable with a prefix and the entity that does the logging is not a real Field in the first place.

manoranjith commented 3 years ago

Hi @ggwpez I was also facing the similar problem of not being able to multiple parties apart by looking at the logs. I like your idea of using prefix over field. However, I am not sure if the approach you are considering will work in the following scenarios (as in case of perun-node):

  1. The node instantiates multiple clients within the same instance of go-perun. Each of the client instances will be assigned to different users (Eg: Alice, Bob).
  2. In this case, both the client instances will have same logger instance, since client.New derives the logger internally from the log package.

One possible solution I could see to address this aspect is to explicitly pass a logger instance to the client.New API.

ggwpez commented 3 years ago

This was mainly aimed for internal logging, since we often use hierarchical loggers there.
You can already set a logger which will be used by client.New with log.Set. Here is an example. It should best be done in an init() function.
If you only want to set the log-level, log/logrus.Set is enough. @manoranjith

manoranjith commented 3 years ago

I do set the logger for the go-perun framework this way. Still my understanding is that, when I create two instances of the client within a single instance of the node, then both clients will be using the same logger.

Does your suggestions imply that I will be able to set different logger instances for each of the client instance ? @ggwpez

manoranjith commented 3 years ago

Because, when same instance of logger is used, I see no possibility to tell the which log entries corresponding to which client instance.

ggwpez commented 3 years ago

Now i understood what you mean. You could call log.Set multiple times, but this is ugly.
go-perun does currently not have a straightforward way to do this.