sc-6427 Demote protocol errors to warnings #101
Conversation
There was a problem hiding this comment.
@pdeziel looks good to me - my methodology was to look at the error we were returning to the user. If the error was FailedPrecondition then I'm happy with Warn, but if it's Internal, then I think we need to keep it at Error. Thanks for making this change, hopefully this will reduce the amount of messages we get on slack!
| @@ -266,14 +266,14 @@ func (s *Server) sendTransfer(req *pb.TransferRequest, account db.Account, parti | |||
| // Conduct a GDS lookup if necessary to get the endpoint | |||
| var peer *peers.Peer | |||
| if peer, err = s.peers.Search(beneficiary.Provider.Name); err != nil { | |||
| log.Error().Err(err).Msg("could not search peer from directory service") | |||
| log.Warn().Err(err).Msg("could not search peer from directory service") | |||
There was a problem hiding this comment.
I think this should remain Error since we're returning codes.Internal here and we may want to look at why the rVASP cannot connect to the directory service.
There was a problem hiding this comment.
I guess it could just be that the given beneficiary VASP doesn't exist in the directory service?
There was a problem hiding this comment.
That's one possibility (the other is cannot connect) - we should return not found instead of internal if the beneficiary VASP doesn't exist in the directory; Internal otherwise?
There was a problem hiding this comment.
I like that idea, but peers.Search doesn't give us an easy way to distinguish the two currently. Should we just keep it as an internal error for now?
There was a problem hiding this comment.
Gotcha, this is Peers from the trisacrypto/trisa repo right? Let's shift to FailedPrecondition instead of internal then (kind of the middle ground between not found and internal) and keep it as warn.
| @@ -460,7 +460,7 @@ func (s *Server) sendError(req *pb.TransferRequest, account db.Account) (rep *pb | |||
| // Conduct a GDS lookup if necessary to get the endpoint | |||
| var peer *peers.Peer | |||
| if peer, err = s.peers.Search(beneficiary.Provider.Name); err != nil { | |||
| log.Error().Err(err).Msg("could not search peer from directory service") | |||
| log.Warn().Err(err).Msg("could not search peer from directory service") | |||
There was a problem hiding this comment.
See above - I think this should remain an Error since we return codes.Internal.
pkg/rvasp/rvasp.go
Outdated
| @@ -503,7 +503,7 @@ func (s *Server) respondAsync(peer *peers.Peer, payload *protocol.Payload, ident | |||
|
|
|||
| // Verify that the transaction has not expired | |||
| if now.Before(xfer.NotBefore) || now.After(xfer.NotAfter) { | |||
| log.Error().Err(err).Msg("received expired async transaction") | |||
| log.Warn().Err(err).Msg("received expired async transaction") | |||
There was a problem hiding this comment.
This should probably just be a Debug -- also, where is err coming from? Is it just nil?
| log.Warn().Err(err).Msg("could not find originator account") | ||
| return nil, protocol.Errorf(protocol.InternalError, "could not find originator account: %s", err) |
There was a problem hiding this comment.
Even though we're returning a protocol.InternalError, we're returning a response so I think this should remain an Warn.
pkg/rvasp/trisa.go
Outdated
| @@ -606,7 +606,7 @@ func (s *TRISA) sendAsync(tx *db.Transaction) (err error) { | |||
| if tx.State == pb.TransactionState_PENDING_SENT { | |||
| var account *db.Account | |||
| if account, err = tx.GetAccount(s.parent.db); err != nil { | |||
| log.Error().Err(err).Msg("could not fetch beneficiary account") | |||
| log.Warn().Err(err).Msg("could not fetch beneficiary account") | |||
There was a problem hiding this comment.
This is a tough one, if the error is sql.ErrNoRows (e.g. not found) I think it should be a Debug -- if it's a database error, e.g. couldn't connect, schema invalid, etc. it should be an Error. Perhaps you could just put a note with a TODO: check the type of error returned and log appropropriately for the future?
This demotes some of the log errors to warnings to avoid spam from the google cloud alerts.