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

Detailed consensus data correlated together. #5302

Merged
merged 2 commits into from
Feb 27, 2025

Conversation

mtrippled
Copy link
Collaborator

@mtrippled mtrippled commented Feb 20, 2025

Combine multiple related debug log data points into a single
message. Allows quick correlation of events that
previously were either not logged or, if logged, strewn across multiple lines, making correlation difficult. The Heartbeat Timer and consensus ledger accept processing each have this capability.

Also guarantees that log entries will be written if the node is a validator, regardless of log severity level. Otherwise, the level of these messages is at INFO severity.

High Level Overview of Change

Enable logging of multiple data points so that they are emitted to the debug log at the same time. Validators will have these messages always emitted, regardless
of other configuration of debug log severity level.

Context of Change

The context is to facilitate data collection and troubleshooting. Currently, this much
of the data in the PR is not being logged. And for that which is being logged, it's
written all over the debug log on multiple lines. This makes it difficult to correlate
consensus-related events.

And because this change emits the logs automatically for validators, no special configuration of severity level needs to be made.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

New, verbose log entries are created. They can be grepped with the string "ConsensusLogger". They are emitted approximately every second, with an additional entry for each consensus "accept" phase. It increases logging requirements. Estimated 5MB/hr.

To see the log messages, grep for:
"ConsensusLogger: Heartbeat Timer" and "ConsensusLogger: onAccept"

If configured as a validator, all debug log severity levels will cause these messages to be emitted (though I'm not sure if kDisabled is over-ridden). To test, configure at warning, error, or fatal, and the log messages should still be emitted. They'll still say they're NFO (info) level, but they bypass the filter. If not configured as a validator, verify that the messages only appear for "info" level or more verbose.

Before committing to this, the community should be informed of the new feature since it will require more disk space, at least for validator operators or people with very terse logging configured. However, the disk requirements are not very large: roughly 5MB per hour. This is < 1GB/week, which is quite small for the benefit.

@mtrippled
Copy link
Collaborator Author

Rebased against a more recent commit.

@mtrippled mtrippled reopened this Feb 20, 2025
Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 52.43902% with 117 lines in your changes missing coverage. Please review.

Project coverage is 78.1%. Comparing base (0a1ca06) to head (6beab0b).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/consensus/Consensus.h 75.6% 31 Missing ⚠️
src/xrpld/app/consensus/RCLConsensus.cpp 3.4% 28 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp 6.9% 27 Missing ⚠️
src/xrpld/consensus/Consensus.cpp 54.7% 24 Missing ⚠️
include/xrpl/beast/utility/WrappedSink.h 0.0% 2 Missing ⚠️
src/libxrpl/basics/Log.cpp 0.0% 2 Missing ⚠️
src/xrpld/app/consensus/RCLConsensus.h 0.0% 2 Missing ⚠️
src/libxrpl/beast/utility/src/beast_Journal.cpp 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5302     +/-   ##
=========================================
- Coverage     78.2%   78.1%   -0.1%     
=========================================
  Files          790     790             
  Lines        67741   67908    +167     
  Branches      8178    8226     +48     
=========================================
+ Hits         52969   53032     +63     
- Misses       14771   14876    +105     
+ Partials         1       0      -1     
Files with missing lines Coverage Δ
include/xrpl/basics/Log.h 71.4% <ø> (ø)
include/xrpl/beast/utility/Journal.h 96.5% <ø> (ø)
src/xrpld/app/main/Application.cpp 69.2% <100.0%> (-0.1%) ⬇️
src/xrpld/app/misc/NetworkOPs.h 100.0% <ø> (ø)
src/libxrpl/beast/utility/src/beast_Journal.cpp 58.0% <0.0%> (-1.2%) ⬇️
include/xrpl/beast/utility/WrappedSink.h 52.4% <0.0%> (-5.5%) ⬇️
src/libxrpl/basics/Log.cpp 18.4% <0.0%> (-0.2%) ⬇️
src/xrpld/app/consensus/RCLConsensus.h 84.0% <0.0%> (-7.3%) ⬇️
src/xrpld/consensus/Consensus.cpp 74.0% <54.7%> (-24.4%) ⬇️
src/xrpld/app/misc/NetworkOPs.cpp 69.1% <6.9%> (-0.7%) ⬇️
... and 2 more

... and 6 files with indirect coverage changes

Impacted file tree graph

@mtrippled mtrippled force-pushed the log-consensus branch 2 times, most recently from be1c95c to caedb46 Compare February 20, 2025 04:00
@mtrippled mtrippled requested a review from vlntb February 23, 2025 20:29
Copy link
Collaborator

@vlntb vlntb left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this PR.
Can you sign the commits please @mtrippled ?

@mtrippled
Copy link
Collaborator Author

I'm happy to approve this PR. Can you sign the commits please @mtrippled ?

I've already signed them. I think the problem might be that the email address I'm using in my git profile, and so gpg signature, isn't approved as a committer.

@mtrippled mtrippled force-pushed the log-consensus branch 3 times, most recently from aa8325f to 15097f4 Compare February 24, 2025 18:23
@mtrippled
Copy link
Collaborator Author

I'm happy to approve this PR. Can you sign the commits please @mtrippled ?

I've signed them, pushed, they say "verified" to me.

@vlntb vlntb self-requested a review February 24, 2025 18:27
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Just a few handfuls of nitpicky suggestions and a few questions.

@mtrippled mtrippled force-pushed the log-consensus branch 5 times, most recently from c5f575a to c983838 Compare February 25, 2025 02:50
@mtrippled mtrippled requested a review from ximinez February 25, 2025 02:51
Copy link
Collaborator

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

@mtrippled Go ahead and add the "Ready to Merge" label to the PR when you're ready.

Suggested commit message:

Log detailed correlated consensus data together (#5302)

Combine multiple related debug log data points into a single
message. Allows quick correlation of events that
previously were either not logged or, if logged, strewn
across multiple lines, making correlation difficult.
The Heartbeat Timer and consensus ledger accept processing
each have this capability.

Also guarantees that log entries will be written if the
node is a validator, regardless of log severity level.
Otherwise, the level of these messages is at INFO severity.

Combine multiple related debug log data points into a single
message. Allows quick correlation of events that
previously were either not logged or, if logged, strewn
across multiple lines, making correlation difficult.
The Heartbeat Timer and consensus ledger accept processing
each have this capability.

Also guarantees that log entries will be written if the
node is a validator, regardless of log severity level.
Otherwise, the level of these messages is at INFO severity.
@mtrippled mtrippled added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Feb 27, 2025
…onsensus

* upstream/develop:
  Set version to 2.4.0-rc3
  fix: Acquire previously failed transaction set from network as new proposal arrives (5318)
  Fix Replace `assert` with `XRPL_ASSERT` (5312)
  fix: Remove 'new parent hash' assert (5313)
  Set version to 2.4.0-rc2
  Add logging and improve counting of amendment votes from UNL (5173)
@ximinez ximinez merged commit af018c7 into XRPLF:develop Feb 27, 2025
23 of 24 checks passed
This was referenced Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants