use services.Config.NewService/Engine (part 2) #14117
Conversation
jmank88
force-pushed
the
service-engine-2
branch
from
93247cb to
643e0a4
Compare
jmank88
force-pushed
the
service-engine-2
branch
from
643e0a4 to
737cb57
Compare
jmank88
force-pushed
the
service-engine-2
branch
from
737cb57 to
4d961b5
Compare
jmank88
force-pushed
the
service-engine-2
branch
from
4d961b5 to
d703ff6
Compare
|
| @@ -56,15 +56,13 @@ func NewWriteTarget(lggr logger.Logger, id string, cr commontypes.ContractReader | |||
| "Write target.", | |||
| ) | |||
|
|
|||
| logger := lggr.Named("WriteTarget") | |||
There was a problem hiding this comment.
why did the methods on the instance disappear? the new convention is less intuitive to me.
There was a problem hiding this comment.
The common package logger interface captures only the simple methods implemented by zap. A Sugared logger can still be constructed from any instance to provide these operations as methods, but making one is messier than just calling the helper when there is just one thing to do.
| commontypes "github.com/smartcontractkit/chainlink-common/pkg/types" | ||
| "github.com/smartcontractkit/chainlink-common/pkg/types/query/primitives" | ||
| "github.com/smartcontractkit/chainlink/v2/core/logger" |
There was a problem hiding this comment.
can we delete this pkg entirely? confusing to have multiple ways to do the same thing
There was a problem hiding this comment.
No, it serves a different extended purpose, but it can be reduced later if we convert everything to use the simpler interface from common.
| @@ -39,7 +38,7 @@ type transmitter struct { | |||
|
|
|||
| func NewTransmitter(lggr logger.Logger, fromAccount string) Transmitter { | |||
| return &transmitter{ | |||
| lggr.Named("DummyTransmitter"), | |||
| logger.Named(lggr, "DummyTransmitter"), | |||
There was a problem hiding this comment.
why has this cascaded to so many places? i only see a handful of NewEngine calls in this PR. does this PR intermix disparate concerns - are all these logger chains strictly necessary in this specific change?
There was a problem hiding this comment.
I followed the compiler errors, so all are necessary, unless we have cases where unrelated things are in the same file I suppose - but that seems like a separate issue.
jmank88
requested review from
krehermann,
samsondav,
EasterTheBunny and
cedric-cordenier
https://smartcontract-it.atlassian.net/browse/BCF-3337
This PR upgrades just a few more types to use
services.Engine. This also triggered many cascading logger use changes from core to common. This was pre-existing technical debt that needed to be done anyways.Follow up from part one: