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

Antithesis instrumentation improvements #5213

Merged
merged 9 commits into from
Dec 16, 2024

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Dec 4, 2024

High Level Overview of Change

Minor changes relating to feedback received late in #5042

Context of Change

  • Rename ASSERT to XRPL_ASSERT
  • Upgrade to Anthithesis SDK 0.4.4
  • Use new 0.4.4 features
    • automatic cast to bool, like assert
  • Add instrumentation workflow to verify build with instrumentation enabled

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

@Bronek Bronek force-pushed the feature/instrumentation_improvements branch from e357eeb to 10366ae Compare December 4, 2024 15:56
Used to verify the Antithesis instrumentation build
@Bronek Bronek force-pushed the feature/instrumentation_improvements branch from 10366ae to 1653844 Compare December 4, 2024 15:58
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 89.34531% with 83 lines in your changes missing coverage. Please review.

Project coverage is 77.9%. Comparing base (ea1fffe) to head (955ffec).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/overlay/detail/PeerImp.cpp 0.0% 12 Missing ⚠️
src/xrpld/app/ledger/detail/LedgerMaster.cpp 41.7% 7 Missing ⚠️
src/xrpld/app/misc/detail/ValidatorList.cpp 80.0% 5 Missing ⚠️
src/xrpld/overlay/detail/OverlayImpl.cpp 37.5% 5 Missing ⚠️
src/xrpld/peerfinder/detail/Logic.h 60.0% 4 Missing ⚠️
include/xrpl/protocol/FeeUnits.h 0.0% 3 Missing ⚠️
src/libxrpl/protocol/Rate2.cpp 50.0% 3 Missing ⚠️
src/xrpld/app/ledger/LedgerHistory.cpp 76.9% 3 Missing ⚠️
src/xrpld/app/consensus/RCLConsensus.cpp 66.7% 2 Missing ⚠️
src/xrpld/app/ledger/ConsensusTransSetSF.cpp 0.0% 2 Missing ⚠️
... and 33 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5213     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          783     783             
  Lines        66677   66677             
  Branches      8108    8108             
=========================================
- Hits         51921   51918      -3     
- Misses       14756   14759      +3     
Files with missing lines Coverage Δ
include/xrpl/basics/Buffer.h 100.0% <100.0%> (ø)
include/xrpl/basics/SlabAllocator.h 94.6% <100.0%> (ø)
include/xrpl/basics/Slice.h 96.8% <100.0%> (ø)
include/xrpl/basics/partitioned_unordered_map.h 99.2% <100.0%> (ø)
include/xrpl/basics/random.h 100.0% <100.0%> (ø)
include/xrpl/basics/scope.h 100.0% <100.0%> (ø)
include/xrpl/basics/spinlock.h 93.5% <100.0%> (ø)
include/xrpl/beast/asio/io_latency_probe.h 96.8% <100.0%> (ø)
include/xrpl/beast/clock/manual_clock.h 100.0% <100.0%> (ø)
.../beast/container/detail/aged_unordered_container.h 96.5% <100.0%> (ø)
... and 203 more

... and 1 file with indirect coverage changes

Impacted file tree graph

@Bronek Bronek force-pushed the feature/instrumentation_improvements branch from d69bf8e to eb9464c Compare December 4, 2024 17:15
@Bronek Bronek force-pushed the feature/instrumentation_improvements branch from eb9464c to 6855fd5 Compare December 4, 2024 17:19
@Bronek Bronek requested review from godexsoft and ximinez December 4, 2024 19:04
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.

Man, I wish I could ignore the ASSERT -> XRPL_ASSERT changes the way I can ignore whitespace. This PR would be like 50 lines long.

include/xrpl/beast/utility/instrumentation.h Outdated Show resolved Hide resolved
src/xrpld/peerfinder/detail/Logic.h Outdated Show resolved Hide resolved
.github/workflows/instrumentation.yml Show resolved Hide resolved
@Bronek
Copy link
Collaborator Author

Bronek commented Dec 5, 2024

Man, I wish I could ignore the ASSERT -> XRPL_ASSERT changes the way I can ignore whitespace. This PR would be like 50 lines long.

You could repeat what I've done

for i in $(git grep -Hwn ASSERT | grep -oE '^[^.]+.(cpp|h|ipp)' | sort -u); do sed -i "s|\bASSERT\b|XRPL_ASSERT|g" "$i" && clang-format -i "$i"; done

... and then compare results.

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 from here. There's a conflict that needs to be resolved, though.

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

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

Thanks for leaving ASSERT available for clients. This looks good from Clio's perspective 👍

@ximinez ximinez 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 Dec 16, 2024
Copy link
Collaborator

@vvysokikh1 vvysokikh1 left a comment

Choose a reason for hiding this comment

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

lgtm

@ximinez ximinez merged commit eabca84 into XRPLF:develop Dec 16, 2024
20 of 21 checks passed
@ximinez ximinez mentioned this pull request Jan 16, 2025
5 tasks
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