feat: add pvm logging #271
Conversation
| // wrap mutator with logging | ||
| var m polkavm.Mutator = i | ||
| if i.log != nil { | ||
| m = NewLogger(i, i.log) |
There was a problem hiding this comment.
Creating a new logger every step?
| @@ -0,0 +1,578 @@ | |||
| package interpreter | |||
There was a problem hiding this comment.
I am not sure about the whole approach. Decorating every function seems wrong. Are we going to do this everywhere? Seems to me it's better just add logging in the Instance functions.
There was a problem hiding this comment.
Interesting, I actually prefer this approach because it abstracts logging into it's own file instead of polluting the codebase with logs and can be easily plugged in and out when necessary without refactoring a lot. I think going forward we need to discuss this more thoroughly with the team and go with a convention that the majority prefers
There was a problem hiding this comment.
So for every function we have in the repo we are going to create a new struct that decorates with logging?
There was a problem hiding this comment.
of course not :) this pattern is used with interfaces, usually for services to track the incoming and outgoing args. For this project we don't really use interfaces for this purpose except for the Mutator interface in pvm so I understand your concern
There was a problem hiding this comment.
I'm also not too sure about this approach, seems clearer to me if the logging is just mixed in, but that might be just me. :)
| @@ -43,6 +49,8 @@ type Instance struct { | |||
| jumpTable []uint64 // j | |||
| bitmask jam.BitSequence // k | |||
| basicBlockInstructions map[uint64]struct{} // ϖ | |||
|
|
|||
| log *zerolog.Logger | |||
There was a problem hiding this comment.
This feels wrong. Imo the instance should not care if we are logging or not. Is the idea that we save on performance to run without zerolog, compared to running with Disabled?
There was a problem hiding this comment.
This is necessary in order to be able to inject the logger inside the instance, how else are we supposed to access the logger? use a global instance?
There was a problem hiding this comment.
Sorry, I should have explained properly. I mean the whole approach, putting a pointer here, so later you have to check if i.log != nil and change the behaviour. Zerolog is designed to not be used as pointer and just log stuff, unless you silence it. It's also safe that way that it works as a no-op logger if you instance has no logger passed to it.
There was a problem hiding this comment.
Ok, I didn't know this library is not expecting to pass the logger instance as a pointer, most loggers that I worked with are expecting a pointer
|
|
||
| func WithLogger(log zerolog.Logger) Option { | ||
| return func(i *Instance) { | ||
| i.log = &log |
There was a problem hiding this comment.
This is pointer to a copy of the Logger not the actual Logger.
No description provided.