Skip to content

Consistent logging of full root in lookup debug logs#5700

Merged
mergify[bot] merged 5 commits intosigp:unstablefrom
dapplion:lookup-sync-logging
May 6, 2024
Merged

Consistent logging of full root in lookup debug logs#5700
mergify[bot] merged 5 commits intosigp:unstablefrom
dapplion:lookup-sync-logging

Conversation

@dapplion
Copy link
Collaborator

@dapplion dapplion commented May 3, 2024

Issue Addressed

From offline discussions:

I think we should aim for consistency either way. sean proposed:

  • full root on debug logs
  • shorten root on info logs

Proposed Changes

  • Consistent logging of full root in lookup debug logs
  • Fix clippy warning
  • Add missing warn log on unexpected cas

@dapplion dapplion requested a review from realbigsean May 3, 2024 01:05
@dapplion dapplion force-pushed the lookup-sync-logging branch from 1eab61e to 1b855bf Compare May 3, 2024 04:44
@michaelsproul michaelsproul added ready-for-review The code is ready for review v5.2.0 Q2 2024 labels May 5, 2024
realbigsean
realbigsean approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

jimmygchen
jimmygchen reviewed May 6, 2024
View reviewed changes
jimmygchen
jimmygchen requested changes May 6, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvements! I've just got a question above.

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 6, 2024
jimmygchen
jimmygchen approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels May 6, 2024
@realbigsean
Copy link
Member

realbigsean commented May 6, 2024

@mergify queue

@mergify
Copy link

mergify bot commented May 6, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 436d54e

mergify bot added a commit that referenced this pull request May 6, 2024
@mergify
Merge of #5700
0b49f0d
@mergify mergify bot mentioned this pull request May 6, 2024
Closed
5 tasks
@mergify mergify bot merged commit 436d54e into sigp:unstable May 6, 2024
@dapplion
Copy link
Collaborator Author

dapplion commented May 8, 2024 •
edited
Loading 

May 07 17:26:28.005 DEBG Received lookup processing result       result: Err(ParentUnknown(RpcBlock(0x3a35cce1b7ce7090e1c698e27887b468a635ea3d808c9fd3653f1b11319eb486))),
id: 2046, block_root: 0x3a35cce1b7ce7090e1c698e27887b468a635ea3d808c9fd3653f1b11319eb486, 
component: Block, service: lookup_sync, service: sync

Declaring the service two times does not overwrite the label, but log it twice. Not sure if this is the intended behavior. Elastic search can deal with it fine, as service = [lookup_sync, sync] and a filter by service is "lookup_sync" works.

@dapplion dapplion deleted the lookup-sync-logging branch January 24, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimmygchen jimmygchen jimmygchen approved these changes

@realbigsean realbigsean realbigsean approved these changes

Assignees

No one assigned

Labels

ready-for-merge This PR is ready to merge. v5.2.0 Q2 2024

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dapplion @realbigsean @jimmygchen @michaelsproul