[Merged by Bors] - Use zap logger in txs package #5973
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5973 +/- ##
=========================================
- Coverage 80.8% 80.8% -0.1%
=========================================
Files 287 290 +3
Lines 29905 30572 +667
=========================================
+ Hits 24183 24713 +530
- Misses 4139 4228 +89
- Partials 1583 1631 +48 ☔ View full report in Codecov by Sentry. |
txs/cache.go
Outdated
| db *sql.Database, | ||
| nextNonce, newBalance uint64, | ||
| applied types.LayerID, | ||
| ) error { | ||
| logger = logger.WithFields(ac.addr) | ||
| logger.With().Debug("resetting to nonce", log.Uint64("nonce", nextNonce)) | ||
| logger = logger.With(zap.Stringer("address", ac.addr)) |
There was a problem hiding this comment.
Note: this causes ac.addr.String() to be called every time, regardless of whether the logs are emitted. This might be inefficient if it happens on a hot path (I don't know - does it?). This logger emits logs down the stream only in error conditions and with the debug level, both shouldn't normally happen.
The addPendingFromNonce is a method of accountCache and it has access to the address field. How about instead of adding the address field to the logger, add it directly in the log calls to avoid the mentioned inefficiencies?
There was a problem hiding this comment.
Well it depends - the new code is equivalent to the previous one.
WithFields just unwrapped the provided fields and passed them to the internal zap.Logger by using With. So the old code also eagerly called String() on the ac.addr field.
The advantage of calling it eagerly is that the expensive String method on ac.addr is called only exactly once no matter how many log lines are printed with the new logger. Passing zap.Stringer("address", ac.addr)) to every log call later is much more expensive if they are actually printed and if they are not it doesn't matter.
|
bors merge |
## Motivation In the continued effort to replace our custom `log.Log` with `zap.Logger` I updated the `txs` package to not depend on our logger any more.
|
Pull request successfully merged into develop. Build succeeded:
|
spacemesh-bors
bot
changed the title
Motivation
In the continued effort to replace our custom
log.Logwithzap.LoggerI updated thetxspackage to not depend on our logger any more.Description
Only logging was updated, no functional changes to the package
Test Plan
existing tests pass
TODO