Propagate logrus logger to aws and grpc packages #60
Conversation
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
==========================================
- Coverage 69.82% 68.67% -1.16%
==========================================
Files 6 6
Lines 759 779 +20
==========================================
+ Hits 530 535 +5
- Misses 210 222 +12
- Partials 19 22 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Overall looks good, just a couple things, can we log errors with Errorf instead of Infof and i see we use logrus package but in toolkit we have logger wich already setuped to use common format for atlas logs, what about using it ?
just one thing per what i see we use logrus package but we h
| func newPublisher(snsClient snsiface.SNSAPI, topic string, opts ...PublisherOption) (*publisher, error) { | ||
| p := publisher{ | ||
| sns: snsClient, | ||
| logger: logrus.StandardLogger(), |
There was a problem hiding this comment.
I see this logger will be overrided, but maybe it will be better to use toolkit logger with some default level (logrus.InfoLevel) as a default logger? It up to you, for me it seems more consistent
There was a problem hiding this comment.
I thought about this. I decided that using the StandardLogger seemed to make more sense as the default. This also allows using the global Logrus functions to change the behavior of this internal logging without having to make further changes (https://github.com/sirupsen/logrus/blob/master/exported.go#L14-L37)
| func newSubscriber(snsClient snsiface.SNSAPI, sqsClient sqsiface.SQSAPI, topic, subscriptionID string) (*awsSubscriber, error) { | ||
| subscriber := awsSubscriber{sns: snsClient, sqs: sqsClient, topic: topic, subscriptionID: subscriptionID} | ||
| func newSubscriber(snsClient snsiface.SNSAPI, sqsClient sqsiface.SQSAPI, topic, subscriptionID string, opts ...SubscriberOption) (*awsSubscriber, error) { | ||
| subscriber := awsSubscriber{sns: snsClient, sqs: sqsClient, topic: topic, subscriptionID: subscriptionID, logger: logrus.StandardLogger()} |
There was a problem hiding this comment.
I see this logger will be overrided, but maybe it will be better to use toolkit logger with some default level (logrus.InfoLevel) as a default logger? It up to you, for me it seems more consistent
Uses the
Optionspattern to simplify the function signature when the default logger is fine.