swarm, swap: add addresses to logs #1806
Conversation
# Conflicts: # swap/swap.go # swap/swap_test.go
swap/swap.go
Outdated
| _, err := s.processAndVerifyCheque(cheque, p) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| auditLog.Debug("received cheque processed and verified", "peer", p.ID().String()) | ||
| p.logger.Debug("received cheque, processed and verified") |
There was a problem hiding this comment.
wouldn't it be good to log which cheque was processed and verified? without any identifying details this logline doesn't say much
There was a problem hiding this comment.
we could add honey as in line 307, but other than that, i'm not sure which field could identify a cheque in a way that is useful for logs.
i am certain that knowing a cheque has arrived/been sent is useful already.
tagging @ralph-pichler and @Eknir for insight
There was a problem hiding this comment.
The CumulativePayout and Contract fields should be enough to uniquely identify which cheque it is. Instead of Contract p.beneficiary might also be useful (this would be the signer of the cheque here).
There was a problem hiding this comment.
added Beneficiary and CumulativePayout to this Debug log entry
thanks guys
# Conflicts: # swap/swap.go # swap/swap_test.go # swarm.go
# Conflicts: # swap/swap.go
swap/swap.go
Outdated
| @@ -118,14 +131,15 @@ func swapRotatingFileHandler(logdir string) (log.Handler, error) { | |||
| } | |||
|
|
|||
| // new - swap constructor without integrity check | |||
| func new(stateStore state.Store, owner *Owner, backend contract.Backend, params *Params) *Swap { | |||
| func new(stateStore state.Store, owner *Owner, backend contract.Backend, params *Params, logger log.Logger) *Swap { | |||
There was a problem hiding this comment.
This is not from this PR, but we need to rename this function, it creates a conflict with the builtin new keyword of go if it would be used inside this package.
Call it newInstance or something else you prefer. You can also create a different PR for this if you prefer.
There was a problem hiding this comment.
pushed this change and added it to the PR description. thanks
swap/swap.go
Outdated
| @@ -63,6 +62,7 @@ type Swap struct { | |||
| params *Params // economic and operational parameters | |||
| contract swap.Contract // reference to the smart contract | |||
| honeyPriceOracle HoneyOracle // oracle which resolves the price of honey (in Wei) | |||
| logger log.Logger // logger for swap related messages and audit trail | |||
There was a problem hiding this comment.
Can we not keep the swapLogger package wide and only do the peerLogger per instance?
There was a problem hiding this comment.
i think we can, but i'm not sure i see reason to.
could you please explain why this would be preferable? in general i try to avoid global variables and the like.
There was a problem hiding this comment.
Because the logger is shared, and is not in any way a property of Swap. Then we also don't need to pass it around methods.
There was a problem hiding this comment.
it's a logger specifically for swap, but yes, maybe it doesn't belong in the struct itself (although if the logpath does, one could argue for this).
it certainly doesn't need to be there.
i've changed it back to a global variable.
| type Params struct { | ||
| OverlayAddr []byte // this node's base address |
There was a problem hiding this comment.
To be honest Params is poorly designed and was meant as an organizing thing for economic parametrization (thresholds, cashing strategy, amounts, etc.).
We have been refactoring the Swap constructor all the time recently, that is why instead of making a huuuuuge constructor with maaaany parameters (resulting in many changes), we started adding mixed parameters to this generic Params struct.
I would like to see a refactoring of this, but it does not need to be in this PR. Conceptually OverlayAddr does not belong to this struct, but LogPath neither does.
There was a problem hiding this comment.
i agree.
i never really understood this Params business—indeed, i only placed OverlayAddr here since it seemed to fit in well next to logpath as an "infrastructure" sort of variable for the Swap struct 🤷♂️
we can have them moved to the Swap struct directly if you prefer, i don't mind.
or: we can revisit this issue later down the road, when refactoring as you say
There was a problem hiding this comment.
created issue #1877 for this
# Conflicts: # swap/swap_test.go
This PR aims to resolve #1623, which is about adding node and peer addresses to SWAP log entries in order to be able to debug more easily (this includes simulations).
Changes:
bzzeth) the swapPeerstruct now has its own logger; using a context specific to each peer, we can log each id automatically this way.newfunction (swap/swap.go:134) tonewSwapInstance.Notes: