vsp: Don't use a global logger. #2256
Conversation
Keep references to the logger inside the VSP client rather than using a global reference.
| @@ -59,7 +61,7 @@ type Config struct { | |||
| Params *chaincfg.Params | |||
| } | |||
|
|
|||
| func New(cfg Config) (*Client, error) { | |||
| func New(cfg Config, log slog.Logger) (*Client, error) { | |||
There was a problem hiding this comment.
do you think it would make more sense to provide the logger in the config? and if it's nil, either don't log anything (probably the best default) or have some default logger to stdout or something (i'm less thrilled by this idea)
There was a problem hiding this comment.
I added it here to avoid adding it in every place where a vsp.Config instance is created (there are a lot of them - server.go is peppered).
Just to explain my reasoning...
Personally I don't view a logger as logically being a config item. I view config as a set of static values which tweak/toggle various behaviours of the component they are passed in to. The values in config may be mandatory or they may have defaults.
A logger is an independent component which implements its own behaviour (and probably has its own config internally). I put logger in the same "not config" category as database/RPC connections, caches, clocks, random number generators. Distinct from config, I'd consider these to be non-optional. They are typically fundamental things your component needs to do its job.
That said, this is all pretty subjective. I'm happy to change it if you like, just let me know what your preferred solution is.
Perhaps we could default to slog.Disabled if you want logger to be optional?
There was a problem hiding this comment.
it's fine as is, just wanted to run the idea past you for your opinion
internal/vsp/feepayment.go
Outdated
| @@ -95,6 +96,7 @@ type feePayment struct { | |||
| timerMu sync.Mutex | |||
| timer *time.Timer | |||
|
|
|||
| log slog.Logger | |||
There was a problem hiding this comment.
the fee payment holds a pointer to the client, we can access the logger through that instead of copying the interface value for every fee payment. i don't think there is any reason that one fee payment should use a different logger from any other, and it reduces the pressure on the GC a small bit since it's less pointers that it needs to follow.
There was a problem hiding this comment.
I've made the suggested change.
I included logger in feePayment because in future I want to try and remove the circular reference between Client and feePayment, but that can wait for another day.
Use the existing pointer in Client to access the logger, rather than keeping a separate (duplicated) pointer in feePayment.
Keep references to the logger inside the VSP client rather than using a global reference.