fix: include transfer ID in deposit logs #1220
Conversation
milapsheth
requested review from
cgorenflo,
haiyizxx,
jcs47 and
fish-sammy
as code owners
x/evm/keeper/msg_server.go
Outdated
| @@ -1459,7 +1462,7 @@ func (s msgServer) SignCommands(c context.Context, req *types.SignCommandsReques | |||
|
|
|||
| commandList := types.CommandIDsToStrings(commandBatch.GetCommandIDs()) | |||
| for _, commandID := range commandList { | |||
| s.Logger(ctx).Debug("signing command batch", "commandBatchID", batchedCommandsIDHex, "commandID", commandID) | |||
| s.Logger(ctx).Debug("signing command %s in batch %s", commandID, batchedCommandsIDHex) | |||
There was a problem hiding this comment.
why take out from attribute?
There was a problem hiding this comment.
For almost everything else we put the info in the log message so it's easy to search it. e.g. put a command batch hex in search query, and all corresponding logs show up. Makes it much easier to debug instead of searching by message in some places and by attributes in others. Also, easy to glance at the logs whereas you have to open each one to see the attribute.
There was a problem hiding this comment.
can search use facet though?
There was a problem hiding this comment.
I feel like using attributes should be the way to go, perhaps gradually, but feel free to ignore if you disagree. Getting used to using attributes, we may find it easier to work with cosmos sdk's logging when needed.
There was a problem hiding this comment.
Agree we should be using facets but given that we pay per new line character is there a downside to including information in the message content? It makes cross-referencing IDs between services much easier.
But data like this should always at minimum be in the attributes, not exclusively in the message content.
Using facets effectively requires us to have
- comprehensive set of query templates including the facet names (no one can remember them all)
- consistent attribute keys (facet names)
We don't have either at this stage - we need to do a log audit imo
There was a problem hiding this comment.
Yep, logs should be reworked properly at some point, so we have consistency for now.
|
request change because I approved earlier 😄 |
|
and edit the pr title pls |
Co-authored-by: haiyizxx <haiyi@axelar.network>
milapsheth
changed the title
|
| @@ -348,7 +347,7 @@ func (k Keeper) GetAvailableOperators(ctx sdk.Context, keyIDs ...exported.KeyID) | |||
| } | |||
|
|
|||
| if !k.operatorHasKeys(ctx, address, keyIDs...) { | |||
| k.Logger(ctx).Debug(fmt.Sprintf("excluding validator %s due absent keys", validator)) | |||
| k.Logger(ctx).Debug(fmt.Sprintf("excluding validator %s due to absent keys", validator)) | |||
There was a problem hiding this comment.
don't want to make this info level?
There was a problem hiding this comment.
Info/Debug is not particularly useful rn unless we need to show on dashboard. Logging needs to be revisited in general.
* fix: include transfer ID in event * add command id to deposit confirmation * add comment * revert bug fix to make log changes consenssus compatible * fix issue * update more logs * Update x/evm/keeper/msg_server.go Co-authored-by: haiyizxx <haiyi@axelar.network> * fixed some logs and made some info level Co-authored-by: haiyizxx <haiyi@axelar.network>
* fix: include transfer ID in event * add command id to deposit confirmation * add comment * revert bug fix to make log changes consenssus compatible * fix issue * update more logs * Update x/evm/keeper/msg_server.go Co-authored-by: haiyizxx <haiyi@axelar.network> * fixed some logs and made some info level Co-authored-by: haiyizxx <haiyi@axelar.network> Co-authored-by: Milap Sheth <milap@axelar.network> Co-authored-by: haiyizxx <haiyi@axelar.network>
Description
Todos
Steps to Test
Expected Behaviour
Other Notes