-
Notifications
You must be signed in to change notification settings - Fork 60
Clean up logging #986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up logging #986
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Arne, this looks good to me modulo the comments.
Let's take the change and use the context-based logs.
Thanks.
@@ -110,7 +110,7 @@ func (c *FinalityManager[V]) runStatusListener(ctx context.Context) { | |||
c.logger.Debugf("check vault status for [%d] transactions", len(txIDs)) | |||
statuses, err := c.vault.Statuses(newCtx, txIDs...) | |||
if err != nil { | |||
c.logger.Errorf("error fetching statuses: %w", err) | |||
c.logger.Errorf("error fetching statuses: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, Arne, while updating these logs, check also if a context.Context is available and in case, use the context logger. In this case, it would be ErrorfContext(ctx,...
This, in addition to writing a log, will also generate an event in the traces.
Please, make sure that the string printed is printable
. Arguments might need to sanitized depending on the cases.
@@ -56,7 +56,7 @@ func (db *SimpleKeyDataStore) PutData(ctx context.Context, key string, data []by | |||
Row(key, data). | |||
OnConflictDoNothing(). | |||
Format() | |||
logger.Debug(query, params) | |||
logger.Debug(query, string(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case, we don't need the DebugfContext because an event in the trace will be added by the call to ExecContext later.
Signed-off-by: Angelo De Caro <[email protected]>
Signed-off-by: Angelo De Caro <[email protected]>
Signed-off-by: Arne Rutjes <[email protected]>
Signed-off-by: Arne Rutjes <[email protected]>
Signed-off-by: Arne Rutjes <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
implements #981