Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Consistent logging of full root in lookup debug logs #5700

Merged
merged 5 commits into from
May 6, 2024
Merged

Consistent logging of full root in lookup debug logs #5700

merged 5 commits into from
May 6, 2024

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
@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 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

@mergify queue

@mergify Mergify
Copy link

mergify bot commented May 6, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

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
27 checks passed
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@realbigsean realbigsean realbigsean approved these changes

@jimmygchen jimmygchen jimmygchen 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