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

Log exception messages in several locations #4400

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

drlongle
Copy link
Contributor

@drlongle drlongle commented Jan 26, 2023

High Level Overview of Change

This PR resolves #3213 and logs exception messages at several locations.

Context of Change

At several locations in the code base, we catch an exception but do not log the exception messages. This PR adds code that logs the exception messages. This can be useful for analysis or debugging. The additional logging can have a (small) negative performance impact.

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)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

@intelliot
Copy link
Collaborator

This is Testable because we can try to trigger some of the exceptions and confirm that they are logged as expected

src/ripple/app/ledger/Ledger.cpp Show resolved Hide resolved
src/ripple/app/ledger/Ledger.cpp Show resolved Hide resolved
src/ripple/app/ledger/Ledger.h Outdated Show resolved Hide resolved
src/ripple/app/ledger/impl/LedgerToJson.cpp Show resolved Hide resolved
src/ripple/app/ledger/impl/OpenLedger.cpp Outdated Show resolved Hide resolved
src/ripple/app/misc/impl/Manifest.cpp Show resolved Hide resolved
src/ripple/app/misc/impl/Manifest.cpp Show resolved Hide resolved
src/ripple/app/misc/impl/ValidatorSite.cpp Show resolved Hide resolved
Copy link
Collaborator

@mtrippled mtrippled left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@@ -3566,7 +3570,7 @@ PeerImp::processLedgerRequest(std::shared_ptr<protocol::TMGetLedger> const& m)
<< "processLedgerRequest: getNodeFat returns false";
}
}
catch (std::exception& e)
catch (std::exception const& e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.

@intelliot
Copy link
Collaborator

@drlongle at your convenience, please do the following:

  1. resolve the merge conflicts
  2. write a proposed commit message (for the entire squashed PR)
  3. comment here with that commit message, and confirm that you believe this PR is ready to merge

@drlongle
Copy link
Contributor Author

I have resolved the conflicts and squashed the commits. The PR is ready to merge.

@drlongle drlongle added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Mar 29, 2023
@intelliot
Copy link
Collaborator

Suggested commit message:

Add logging for exceptions: (#4400)

Log exception messages at several locations.

Previously, these were locations where an exception was caught, but the
exception message was not logged. Logging the exception messages can be
useful for analysis or debugging. The additional logging can have a
small negative performance impact.

Fix #3213.

@drlongle please confirm this looks good, or suggest an alternative.

@drlongle
Copy link
Contributor Author

@intelliot Looks good. Thanks!

@intelliot intelliot merged commit 8bfdbcb into XRPLF:develop Mar 30, 2023
@drlongle drlongle deleted the log-exception-messages branch March 31, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Should log exception message in several locations
5 participants