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.

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

Add timeSinceSlotStart field to "Synced new block..." log #8420

Merged
merged 3 commits into from
Feb 10, 2021

Conversation

terencechain
Copy link
Member

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Add

Which issues(s) does this PR fix?

Add timeSinceSlotStart field to "Synced new block..." log, this enhances debugging capability for both propagation issue and attesting issue.

Other notes for review

Screen Shot 2021-02-09 at 10 00 11 AM

@terencechain terencechain self-assigned this Feb 9, 2021
@terencechain terencechain requested a review from a team as a code owner February 9, 2021 18:08
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

Super nice addition

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

❗ No coverage uploaded for pull request base (develop@ae028d9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #8420   +/-   ##
==========================================
  Coverage           ?   57.99%           
==========================================
  Files              ?      454           
  Lines              ?    32263           
  Branches           ?        0           
==========================================
  Hits               ?    18710           
  Misses             ?    10691           
  Partials           ?     2862           

@prylabs-bulldozer prylabs-bulldozer bot merged commit d44ab1a into develop Feb 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the time-since-slot-start-field branch February 10, 2021 17:24
@potuz
Copy link
Contributor

potuz commented Feb 12, 2021

Oh I am late hadn't seen this, I am sorry for being a pain in the ass but I think this is a bad addition. It is information that is currently included in the very same log message, so it adds size to the journal to get no new information. It only saves the viewer the time to do a subtraction and most users will not care about this info. The ones that do can do that subtraction.

It would be much more useful if we included a timeSinceBlockReceived log, this is new information that would be valuable diagnosing bad votes.

@terencechain
Copy link
Member Author

Oh I am late hadn't seen this, I am sorry for being a pain in the ass but I think this is a bad addition. It is information that is currently included in the very same log message, so it adds size to the journal to get no new information. It only saves the viewer the time to do a subtraction and most users will not care about this info. The ones that do can do that subtraction.

It would be much more useful if we included a timeSinceBlockReceived log, this is new information that would be valuable diagnosing bad votes.

Its just one additional field per slot (few bytes), is the local storage really that expensive? Keep in mind that we'll remove 4 fields here:
#8330

Still a net gain after all. I personally prefer this than doing math in my head with the timestamp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants