From c01f8ae99cec4e91e308340948e296eb99e9da0d Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Apr 2026 18:14:00 +0100 Subject: [PATCH] fix(telemetry): address code review findings for Phase 4 consensus tracing Fix quorum attribute to use actual validator quorum instead of proposer count, add missing ConsensusState::Expired handling in haveConsensus() span, move ConsensusSpanNames.h to xrpld/consensus/ to resolve levelization cycle, remove unused constants, enrich proposal receive span with sequence, and correct stale documentation references. Co-Authored-By: Claude Opus 4.6 --- .github/scripts/levelization/generate.py | 0 .github/scripts/levelization/results/loops.txt | 3 --- .github/scripts/levelization/results/ordering.txt | 1 + OpenTelemetryPlan/02-design-decisions.md | 4 ++-- OpenTelemetryPlan/06-implementation-phases.md | 13 +++++++------ OpenTelemetryPlan/Phase4_taskList.md | 2 +- src/xrpld/app/consensus/RCLConsensus.cpp | 5 +++-- src/xrpld/consensus/Consensus.h | 4 +++- src/xrpld/{app => }/consensus/ConsensusSpanNames.h | 7 +------ src/xrpld/overlay/detail/PeerImp.cpp | 3 ++- 10 files changed, 20 insertions(+), 22 deletions(-) mode change 100644 => 100755 .github/scripts/levelization/generate.py rename src/xrpld/{app => }/consensus/ConsensusSpanNames.h (97%) diff --git a/.github/scripts/levelization/generate.py b/.github/scripts/levelization/generate.py old mode 100644 new mode 100755 diff --git a/.github/scripts/levelization/results/loops.txt b/.github/scripts/levelization/results/loops.txt index 463a35e822..66906f48c6 100644 --- a/.github/scripts/levelization/results/loops.txt +++ b/.github/scripts/levelization/results/loops.txt @@ -7,9 +7,6 @@ Loop: test.jtx test.unit_test Loop: xrpl.telemetry xrpld.rpc xrpld.rpc > xrpl.telemetry -Loop: xrpld.app xrpld.consensus - xrpld.app > xrpld.consensus - Loop: xrpld.app xrpld.overlay xrpld.app > xrpld.overlay diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index 1d8ed01560..775645a53b 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -236,6 +236,7 @@ xrpl.tx > xrpl.protocol xrpld.app > test.unit_test xrpld.app > xrpl.basics xrpld.app > xrpl.core +xrpld.app > xrpld.consensus xrpld.app > xrpld.core xrpld.app > xrpl.json xrpld.app > xrpl.ledger diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index 9b0ef51db6..5d68278629 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -251,8 +251,8 @@ resource::SemanticConventions::SERVICE_INSTANCE_ID = "xrpl.consensus.proposers_total" = int64 // Total peer positions "xrpl.consensus.agree_count" = int64 // Peers that agree (haveConsensus) "xrpl.consensus.disagree_count" = int64 // Peers that disagree -"xrpl.consensus.threshold_percent" = int64 // Current threshold (50/65/70/95) -"xrpl.consensus.result" = string // "yes", "no", "moved_on" +"xrpl.consensus.threshold_percent" = int64 // Close-time consensus threshold (avCT_CONSENSUS_PCT = 75%) +"xrpl.consensus.result" = string // "yes", "no", "moved_on", "expired" "xrpl.consensus.mode.old" = string // Previous consensus mode "xrpl.consensus.mode.new" = string // New consensus mode ``` diff --git a/OpenTelemetryPlan/06-implementation-phases.md b/OpenTelemetryPlan/06-implementation-phases.md index f78dc172dc..77b5604973 100644 --- a/OpenTelemetryPlan/06-implementation-phases.md +++ b/OpenTelemetryPlan/06-implementation-phases.md @@ -181,11 +181,12 @@ SHAMap tracing are not implemented. | Span Name | Location | Attributes | | --------------------------- | ---------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `consensus.proposal.send` | `RCLConsensus.cpp:177` | `xrpl.consensus.round` | -| `consensus.ledger_close` | `RCLConsensus.cpp:282` | `xrpl.consensus.ledger.seq`, `xrpl.consensus.mode` | -| `consensus.accept` | `RCLConsensus.cpp:395` | `xrpl.consensus.proposers`, `xrpl.consensus.round_time_ms` | -| `consensus.accept.apply` | `RCLConsensus.cpp:521` | `xrpl.consensus.close_time`, `close_time_correct`, `close_resolution_ms`, `state`, `proposing`, `round_time_ms`, `ledger.seq`, `parent_close_time`, `close_time_self`, `close_time_vote_bins`, `resolution_direction` | -| `consensus.validation.send` | `RCLConsensus.cpp:753` | `xrpl.consensus.proposing` | +| `consensus.phase.open` | `Consensus.h:707` | _(none)_ | +| `consensus.proposal.send` | `RCLConsensus.cpp:232` | `xrpl.consensus.round` | +| `consensus.ledger_close` | `RCLConsensus.cpp:341` | `xrpl.consensus.ledger.seq`, `xrpl.consensus.mode` | +| `consensus.accept` | `RCLConsensus.cpp:492` | `xrpl.consensus.proposers`, `xrpl.consensus.round_time_ms`, `xrpl.consensus.quorum` | +| `consensus.accept.apply` | `RCLConsensus.cpp:541` | `xrpl.consensus.close_time`, `close_time_correct`, `close_resolution_ms`, `state`, `proposing`, `round_time_ms`, `ledger.seq`, `parent_close_time`, `close_time_self`, `close_time_vote_bins`, `resolution_direction` | +| `consensus.validation.send` | `RCLConsensus.cpp:900` | `xrpl.consensus.ledger.seq`, `xrpl.consensus.proposing` | ### Exit Criteria @@ -279,7 +280,7 @@ See [Phase4_taskList.md](./Phase4_taskList.md) for full task details. validations) to enable true distributed tracing between nodes. **Status**: Design documented, NOT implemented. Protobuf fields (field 1001) -and `TraceContextPropagator` class exist. Wiring deferred until Phase 4a is +and `TraceContextPropagator` free functions exist. Wiring deferred until Phase 4a is validated in a multi-node environment. **Prerequisites**: Phase 4a complete and validated. diff --git a/OpenTelemetryPlan/Phase4_taskList.md b/OpenTelemetryPlan/Phase4_taskList.md index 9be67807d4..1670e9b57e 100644 --- a/OpenTelemetryPlan/Phase4_taskList.md +++ b/OpenTelemetryPlan/Phase4_taskList.md @@ -903,6 +903,6 @@ share the same trace_id. P2P propagation adds **span-level** linking: ## Prerequisites - Phase 4a (this task list) — establish phase tracing must be in place -- `TraceContextPropagator` class (already exists in +- `TraceContextPropagator` free functions (already exist in `include/xrpl/telemetry/TraceContextPropagator.h`) - Protobuf `TraceContext` message (already exists, field 1001) diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 034b083d04..9d386d2602 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -1,6 +1,5 @@ #include -#include #include #include #include @@ -19,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -495,7 +495,8 @@ RCLConsensus::Adaptor::makeAcceptSpan(Result const& result) span->setAttribute( telemetry::cons_span::attr::roundTimeMs, static_cast(result.roundTime.read().count())); - span->setAttribute(telemetry::cons_span::attr::quorum, static_cast(result.proposers)); + span->setAttribute( + telemetry::cons_span::attr::quorum, static_cast(app_.getValidators().quorum())); return span; } diff --git a/src/xrpld/consensus/Consensus.h b/src/xrpld/consensus/Consensus.h index bbaf1d9999..a32cdd2c0c 100644 --- a/src/xrpld/consensus/Consensus.h +++ b/src/xrpld/consensus/Consensus.h @@ -1,8 +1,8 @@ #pragma once -#include #include #include +#include #include #include @@ -1804,6 +1804,8 @@ Consensus::haveConsensus(std::unique_ptr const& clog stateStr = "yes"; else if (result_->state == ConsensusState::MovedOn) stateStr = "moved_on"; + else if (result_->state == ConsensusState::Expired) + stateStr = "expired"; span.setAttribute(cons_span::attr::result, stateStr); CLOG(clog) << "Consensus has been reached. "; diff --git a/src/xrpld/app/consensus/ConsensusSpanNames.h b/src/xrpld/consensus/ConsensusSpanNames.h similarity index 97% rename from src/xrpld/app/consensus/ConsensusSpanNames.h rename to src/xrpld/consensus/ConsensusSpanNames.h index 9304599e30..868f730860 100644 --- a/src/xrpld/app/consensus/ConsensusSpanNames.h +++ b/src/xrpld/consensus/ConsensusSpanNames.h @@ -31,7 +31,7 @@ * +-- consensus.establish [main thread, child] * | Created: Consensus::startEstablishTracing() * | Ended: Consensus::phaseEstablish() on accept - * | Attrs: converge_percent, tx_count, disputes_count + * | Attrs: converge_percent, establish_count, proposers * | * +-- consensus.update_positions [main thread] * | Created: Consensus::updateOurPositions() @@ -166,9 +166,6 @@ inline constexpr auto resolutionDirection = join(xrplConsensus, makeStr("resolut inline constexpr auto convergePercent = join(xrplConsensus, makeStr("converge_percent")); /// "xrpl.consensus.establish_count" inline constexpr auto establishCount = join(xrplConsensus, makeStr("establish_count")); -/// "xrpl.consensus.proposers_agreed" -inline constexpr auto proposersAgreed = join(xrplConsensus, makeStr("proposers_agreed")); - // Avalanche threshold attributes /// "xrpl.consensus.avalanche_threshold" inline constexpr auto avalancheThreshold = join(xrplConsensus, makeStr("avalanche_threshold")); @@ -189,8 +186,6 @@ inline constexpr auto thresholdPercent = join(xrplConsensus, makeStr("threshold_ inline constexpr auto result = join(xrplConsensus, makeStr("result")); /// "xrpl.consensus.quorum" inline constexpr auto quorum = join(xrplConsensus, makeStr("quorum")); -/// "xrpl.consensus.validation_count" -inline constexpr auto validationCount = join(xrplConsensus, makeStr("validation_count")); // Trace strategy attribute /// "xrpl.consensus.trace_strategy" diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 03c743fc9b..edaae48397 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1,6 +1,5 @@ #include -#include #include #include #include @@ -10,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -1963,6 +1963,7 @@ PeerImp::onMessage(std::shared_ptr const& m) telemetry::seg::consensus, telemetry::cons_span::op::proposalReceive)); span->setAttribute(telemetry::cons_span::attr::trusted, isTrusted); + span->setAttribute(telemetry::cons_span::attr::round, static_cast(set.proposeseq())); std::weak_ptr const weak = shared_from_this(); app_.getJobQueue().addJob(