Bcfr 945 improve contract reader logs #14546
Conversation
EasterTheBunny
requested review from
krehermann
and removed request for
a team
|
| @@ -173,14 +173,26 @@ func (cr *chainReader) Unbind(ctx context.Context, bindings []commontypes.BoundC | |||
| func (cr *chainReader) GetLatestValue(ctx context.Context, readName string, confidenceLevel primitives.ConfidenceLevel, params any, returnVal any) error { | |||
| binding, address, err := cr.bindings.GetReader(readName) | |||
| if err != nil { | |||
| cr.lggr.Debug(err.Error()) | |||
There was a problem hiding this comment.
Is there more to say here?
| cr.lggr.Debug(err.Error()) | |
| cr.lggr.Debugw("message","err",err) |
There was a problem hiding this comment.
Maybe this logging should be the responsibility of the caller. I did it here because this was the request as I understood it. My primary goal was to put more structured information in the errors themselves and ensure they are wrapped with the correct types for the loop interface.
| return binding.GetLatestValue(ctx, common.HexToAddress(address), confidenceLevel, params, returnVal) | ||
| err = binding.GetLatestValue(ctx, common.HexToAddress(address), confidenceLevel, params, returnVal) | ||
| if err != nil { | ||
| cr.lggr.Debug(err.Error()) |
There was a problem hiding this comment.
I'm not a fan of this pattern since it leads to double reporting of errors. What is the use case for logging the error that we are already returning? How will a node op know that this is not a distinct error from the one returned, if it ends up being logged as well?
There was a problem hiding this comment.
I agree. Can we avoid logging here? @ilija42
There was a problem hiding this comment.
Alternatively, we could generalize this a bit to be a GRPC "interceptor" that can cover every service, rather than just chain reader. This avoids the double error problem, because all of the interceptor logs come from the same place and the same logger name, so it is clear that this is a middleman and not the source or the handler of the error.
There was a problem hiding this comment.
What do you think about wrapping errors with extra detail all the way up to the beginning of the stack, instead of logging them at the origin? So we would get something like this "failed to get latest value for xy at address xy ,err: failed to convert params xy to nativeOnchainTypes, err: failed to create codec type for item type xy"
Then, if it's a user input error, simply return it, but if it's an internal error, log it and return a generic internal error message. The goal is to make debugging this without opening up a debugger possible 😆
cc @jmank88 not sure how we usually do this
That is why I tried adding a lot more information about each error to give better hints at where the error occurred in the call stack. Everything is a read error, but each error gets a bit more detail as it moves down the stack and also picks up input parameters as well. My main concern is security of printing some of the call parameters and return values to logs. That is what it is doing right now. |
BCFR-945 improve logs and error handling for contract reader
All errors are structured as a
ReadErrorwith extra detail on the contract, method/event, parameters, etc. This data is now provided in the debug log.