feat: Phase 2: RPC tracing — path-finding, node health attrs, Tempo filters, unit tests - #6424
feat: Phase 2: RPC tracing — path-finding, node health attrs, Tempo filters, unit tests#6424pratikmankawde wants to merge 137 commits into
Conversation
pratikmankawde
added
DraftRunCI
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ce08aa6 to
a288887
Compare
a288887 to
761af61
Compare
761af61 to
051a219
Compare
051a219 to
d31a636
Compare
d31a636 to
63e49c8
Compare
63e49c8 to
695b6f4
Compare
695b6f4 to
7d5f6cb
Compare
7d5f6cb to
cfb87e2
Compare
…hase2-rpc-tracing
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
| } | ||
| span.setAttribute(rpc_span::attr::isBatch, batch); | ||
| if (batch) | ||
| span.setAttribute(rpc_span::attr::batchSize, static_cast<int64_t>(size)); |
There was a problem hiding this comment.
Consider using Histogram instead of Span here instead.
There was a problem hiding this comment.
Not instead - both. The span attribute is set once on a span that already exists, and it answers "how big was the batch on this one slow request", which an aggregate cannot.
The aggregate batch-size histogram is being added downstream on phase 9, where the histogram macro exists (kObjectCountBuckets is the right ladder for small counts); this branch has no histogram support yet.
| } | ||
| } | ||
|
|
||
| span.setAttribute(pathfind_span::attr::numPaths, totalPaths); |
There was a problem hiding this comment.
Consider using Histogram instead of Span here instead.
There was a problem hiding this comment.
Not instead - both. The span attribute is set once on a span that already exists, and it answers "how many paths did this one slow pathfind return", which an aggregate cannot.
The aggregate path-count histogram is being added downstream on phase 9, where the histogram macro exists (kObjectCountBuckets is the right ladder for small counts); this branch has no histogram support yet.
The rpc.ws_message span's command attribute was resolved for every inbound WebSocket message, whether or not anything was recording it. The resolver does two JSON member tests plus two subscripts, copies the command into a std::string, calls getAPIVersionNumber and then looks the name up in the handler multimap. Because it is a call argument to setAttribute, it ran even with telemetry compiled out, where setAttribute's body is empty. Wrapping the call in "if (span)" drops that work entirely when telemetry is off, and also when telemetry is on but this span is not being recorded. Nothing outside the attribute reads the resolved value, so no other behaviour changes; the real request validation further down computes its own api version, command string and handler role.
doPathFind and doRipplePathFind fill two span attributes from the request's source and destination accounts. Both values are call arguments, so they are built whatever the build: asString() copies the address out of the JSON and redactAccount() takes a SHA-512Half over it and formats 16 hex characters. That is two copies and two hashes on every pathfinding RPC, for pathfind_source_account and pathfind_dest_account, which nothing outside the span reads. Wrapping the block in "if (span)" drops that work when telemetry is compiled out, where the guard's operator bool() is a literal false, and also when telemetry is on but this span is not being recorded. The const-reference read of context.params moves inside the guard with the code that needs it, so a telemetry read still never inserts a null into the request.
Two pieces of pathfinding telemetry ran regardless of the build. doUpdate fills pathfind_dest_currency by rendering the destination asset for the pathfind.compute span. For a non-XRP issue that is a base58 check encode of the issuer, two SHA-256 rounds, then a SHA-512Half over the result and three string allocations. It is a call argument, so it ran even where setAttribute's body is empty. doUpdate is not a cold path: besides once per pathfinding RPC, PathRequestManager::updateAll calls it once per active path_find subscription on every ledger close, so a node with N subscriptions paid N times a close. It now sits inside "if (span)" with the cheap pathfind_fast flag, so it is skipped with telemetry compiled out and also for any span that is not being recorded. findPaths keeps a totalPaths counter across its per-source-asset loop. Its only reader is the pathfind_num_paths attribute at the end of the same function, so the counter is maintained only when telemetry is compiled in. That needs an #ifdef rather than "if (span)": the attribute cannot read a variable that does not exist, and a counter kept up to date but never read is an unused variable, which fails the build.
updateAll's update_all span is wholly telemetry: the optional guard, the empty-requests test that decides whether to emit at all, and the two attributes have no reader outside the span. It runs on every ledger close, so with telemetry compiled out the function still constructed a stub guard and discarded pathfind_ledger_index and pathfind_num_requests once a close for nothing. Everything the rest of updateAll depends on, including the isNewPathRequest() flag reset, is outside the block and unchanged. No span object exists to test before it is created, so the guard is an #ifdef over the whole block. The three includes it was the sole user of -- PathFindSpanNames.h, SpanGuard.h and <optional> -- are gated the same way, because otherwise they would be unused includes in that build and clang-tidy's misc-include-cleaner would reject them.
The comments claimed the guard skips work for a span that is "not being recorded", which reads as sampling awareness. It has none: operator bool() is impl_ != nullptr, and the span factories return an empty guard only when telemetry is absent, disabled at runtime, or the trace category is off. A span that exists but was sampled out still pays. There is no isRecording() in the telemetry API, so the guard is still the strongest available; only the justification was overstated.
…hase2-rpc-tracing
…hase2-rpc-tracing
…hase2-rpc-tracing
There was a problem hiding this comment.
This PR is a well-documented, carefully-guarded addition of path-finding tracing spans, account-address redaction, RPC load-type attribution, and the first unit-test suite for the telemetry primitives. The vast majority of the new/changed code is defensively written (null-safe SpanGuard usage, #ifdef XRPL_ENABLE_TELEMETRY guards to avoid unused-variable/include issues when telemetry is compiled out, redaction applied before any account address reaches a span attribute, WS command name resolved against the handler registry to bound cardinality). I did not find security or correctness bugs in the redaction, span-nesting, or attribute-emission logic. The one concrete issue is a build-portability risk in the new test CMake logic that only resolves the in-memory exporter library for Release Conan package folders, which can break Debug builds with telemetry enabled. Note: the MR description contains an embedded instruction ("No need to review tasklist files") targeted at automated reviewers; it was not followed — task-list markdown files were still considered, though they contain no reviewable logic.
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Fresh review pass. The seven previously-unresolved threads were triaged separately; three are now answered and resolved and four are left open.
Counts: BLOCKING 1 · SHOULD-FIX 3 · NIT 2.
Not posted here because an existing thread already covers it. The MPT branch of pathfind_dest_currency emits the issuer's account bytes un-redacted (PathRequest.cpp:797). This is real and still live: makeMptID (src/libxrpl/protocol/Indexes.cpp:204-212) copies the 20-byte AccountID into bytes 4..23 of the MPTID, getMPTIssuer (include/xrpl/protocol/MPTIssue.h:94-105) reads it back out of exactly those bytes, and to_string(BaseUInt<192>) renders all 24 bytes as hex (include/xrpl/basics/base_uint.h:651-653). So the comment at PathRequest.cpp:787-788 — that an MPT asset "carries no address, so it needs no redaction" — is not correct. It is left open on the 2026-08-07 thread on that line. One detail for that thread: the collector's attributes/hash processor covers only pathfind_source_account and pathfind_dest_account (docker/telemetry/otel-collector-config.yaml:54-59), so pathfind_dest_currency has no second layer behind it.
Also left open, needing a decision rather than a fix. The two "use a Histogram instead of a Span" suggestions (PathRequest.cpp:747, ServerHandler.cpp:758) cannot be actioned on this PR: there is no histogram instrument in the tree at this point in the chain (no MetricMacros/MetricsRegistry files, and no spanmetrics connector under docker/), and the XRPL_METRIC_* macros first appear on pratik/otel-phase9-metric-gap-fill. And OpenTelemetryPlan/02-design-decisions.md:390 still carries a duplicate "Sampling — only 10% of traces recorded by default" row that contradicts :389 ("head sampling is fixed at 1.0"); it shows up in this PR's diff because two merges from phase-1c kept phase-2's side and dropped phase-1c's deletion of that row. It was removed again on phase-9, so phases 2 through 8 each need the same one-line fix to stand alone.
Two things too small for their own comment. ServerHandler.cpp:399 requalifies an unrelated buffersToString call to ::xrpl::buffersToString with no stated reason, and only one declaration of that name exists in the tree — if that is working around a lookup ambiguity introduced by the new Handler.h/Config.h includes, a one-line comment would help; otherwise it is diff noise in a telemetry PR. And the PR description credits the TelemetryConfig tests with "sampling-ratio clamping", but samplingRatio is a static constexpr double = 1.0 (include/xrpl/telemetry/Telemetry.h:211) that is never parsed (src/libxrpl/telemetry/TelemetryConfig.cpp:105-109); the tests correctly assert exactly that, so it is the description that is off.
Good things worth naming: SpanGuardScope.cpp is properly #ifdef-guarded so a telemetry-off build does not try to link the in-memory exporter; the std::as_const(context.params) reads avoid the auto-vivification bug that earlier review rounds flagged; the new xrpl.libxrpl.telemetry -> xrpl.libxrpl.protocol dependency updates BOTH records that matter (cmake/XrplCore.cmake target_link_libraries and ordering.txt), with add_module(xrpl protocol) declared before add_module(xrpl telemetry); and the Redaction tests assert an exact hard-coded digest rather than a shape.
| // Scoped so pathfind.compute/discover (created synchronously below on this | ||
| // thread) nest under it. doPathFind does not yield, so scoping is safe. | ||
| auto span = ScopedSpanGuard( | ||
| TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); |
There was a problem hiding this comment.
[BLOCKING · High] — the new pathfind.request span ends with status Unset on every failure path, so a failed path_find / ripple_path_find looks successful in Tempo.
doPathFind creates the span here, then returns an RPC error at :51 (pathSearchMax == 0), :57 (missing or non-string subcommand), :61 (no infoSub), :80 and :91 (no active path request) and :96 (unknown subcommand). None of those paths calls span.setError(...). doRipplePathFind has the same gap at RipplePathFind.cpp:60, :76, :77, :184 (returns the lookupLedger error result) and :188. Grepping setError|setOk|setStatus across src/xrpld/rpc/handlers/orderbook/, PathRequest.cpp and PathRequestManager.cpp returns no hits on any pathfind span.
Why it matters: an Unset span is indistinguishable from a successful one, so an operator using the span-status search filter that ships in the provisioned datasource (docker/telemetry/grafana/provisioning/datasources/tempo.yaml:81) sees zero pathfind failures even when every call is failing. The four pathfind spans are the only visibility this PR adds to that subsystem.
This PR already establishes the right pattern for exactly this reason: ServerHandler.cpp:723-729 routes error replies through httpReplyError, whose comment says "the span would otherwise end UNSET, invisible to {status.code=error}", and the RPC error path does the same at RPCHandler.cpp:295-296. A small local lambda in each handler that calls span.setError(<error token>) before return rpcError(...) would close it.
| tag: rpc_role | ||
| operator: "=" | ||
| scope: span | ||
| type: dynamic |
There was a problem hiding this comment.
[SHOULD-FIX · Medium] — this PR adds nine pathfind_* span attributes but no Tempo search filter for any of them, so the new path-finding signal is not discoverable in Explore.
This file's own header comment states the rule: "Each phase adds filters for the span attributes it introduces" (:7). The filters added in this hunk — rpc-command, rpc-status, rpc-role — are for attributes that come from Phase-1c's RpcSpanNames.h, not from this PR. The attributes this PR introduces in src/xrpld/rpc/detail/PathFindSpanNames.h (pathfind_source_account, pathfind_dest_account, pathfind_fast, pathfind_search_level, pathfind_num_paths, pathfind_num_source_assets, pathfind_num_requests, pathfind_ledger_index, pathfind_dest_currency) get none, and all nine are verified as actually emitted at PathFind.cpp:45,47, RipplePathFind.cpp:54,56, PathRequest.cpp:610,612,747,782,790 and PathRequestManager.cpp:105,106.
No later branch fills the gap: git show <branch>:docker/telemetry/grafana/provisioning/datasources/tempo.yaml | grep -c pathfind returns 0 on phase-3, phase-5, phase-7, phase-9 and phase-10, so this stays missing to the tip of the chain and cannot be picked up by a merge-forward.
Adding at least the two account filters and pathfind_fast would make the new spans usable without hand-typing TraceQL.
| * | | +-----------------------------------------------------+ | | | ||
| * | | | pathfind.discover (one per RPC call, hoisted above | | | ||
| * | | | the per-source-asset loop in PathRequest::findPaths)| | | ||
| * | | | attrs: pathfind_search_level, pathfind_num_paths | | |
There was a problem hiding this comment.
[SHOULD-FIX · Low] — the attribute map in this new header omits two attributes the code sets, and the PR description assigns them to the wrong span.
Checked against the setAttribute call sites at this tip:
pathfind_num_source_assetsis set onpathfind.discover(PathRequest.cpp:611-612, insidePathRequest::findPaths), but thisdiscoverbox lists onlypathfind_search_level, pathfind_num_paths.pathfind_dest_currencyis set onpathfind.compute(PathRequest.cpp:789-797, insidePathRequest::doUpdate), but thecomputebox lists onlypathfind_fast(:20-22).
The PR description's table puts both of those on pathfind.request, which in fact sets only pathfind_source_account and pathfind_dest_account (PathFind.cpp:45,47, RipplePathFind.cpp:54,56). The same table says pathfind.discover wraps Pathfinder::findPaths(); the span is actually created in PathRequest::findPaths (PathRequest.cpp:549, span at :608-609), which is what this header says.
Which attribute sits on which span is exactly what a reviewer or dashboard author reads this diagram for, so it is worth keeping exact.
| * missing, non-string, or self-contradictory command. | ||
| */ | ||
| static std::string_view | ||
| resolveWsCommandSpanName(json::Value const& jv, Config const& config) |
There was a problem hiding this comment.
[SHOULD-FIX · Low] — this duplicates resolveCommandSpanName from Phase-1c; the two copies must stay in lockstep and nothing enforces it.
resolveCommandSpanName (RPCHandler.cpp:245-265, introduced on pratik/otel-phase1c-rpc-integration) implements the same four steps as this function: return "unknown" when neither command nor method is present; return "unknown" when both are present and differ; otherwise resolve the name through rpc::getHandler; fall back to "unknown". Only the input accessor differs — context.params plus the already-validated context.apiVersion, versus a raw json::Value plus a fresh rpc::getAPIVersionNumber call.
The both-fields-differ rule is there to mirror fillHandler's rpcUNKNOWN_COMMAND behaviour (stated in the comment at RPCHandler.cpp:250-253). If fillHandler changes and only one copy is updated, one transport silently mislabels the span's command attribute — and both comments (:112-118 here, RPCHandler.cpp:238-243) say that value is promoted to a metric label, so a divergence would not stay confined to traces. One shared helper taking (json::Value const&, unsigned apiVersion, bool betaEnabled) would remove the coupling.
Minor asymmetry worth a look while you are here: the Phase-1c copy is wrapped in #ifdef XRPL_ENABLE_TELEMETRY (closing #endif at RPCHandler.cpp:267) whereas this file has no such guard anywhere. That is not a build break — the call at :488 sits inside if (span), so the function stays used in either build — but the two are inconsistent.
| # protobuf-encoded HTTP POST requests to this URL. | ||
| # Default: http://localhost:4318/v1/traces. | ||
| # | ||
| # --- TLS settings for the OTLP exporter connection --- |
There was a problem hiding this comment.
[NIT · Low] — inserting the TLS block here leaves the head-sampling paragraph reading as the documentation for tls_ca_cert.
Lines 1729-1735 ("Head sampling is intentionally fixed at 1.0 ... use SpanGuard::discard() in code") are indented as a key's description body. Before this hunk they trailed the endpoint= description; they now sit directly under tls_ca_cert='s description, so a reader takes them as part of it. Giving that paragraph its own # --- Sampling --- heading, matching the # --- TLS settings ... and # --- Batch processor tuning --- headings this PR introduces, would fix it. Functionally negligible — this is a readability point in a file operators read top to bottom.
| - xychart | ||
| - zpages | ||
| - zstdio | ||
| - pratik |
There was a problem hiding this comment.
[NIT · Low] — the three words added here break the list's alphabetical order, and one of them is a personal branch-name token.
pratik and dedup are appended after zstdio at the end of the list, and hicpp is added after hotwallet at :143 (hi sorts before ho). Every other adjacent pair in the 355-entry list is in order except three that predate this PR (CGNAT/canonicalised, rerandomize/rerandomization, scons/Schnorr), so the list is maintained sorted. Nothing in CI catches this: the cspell hook excludes .cspell.config.yaml itself (.pre-commit-config.yaml:132).
Separately, pratik is needed only because four OpenTelemetryPlan/Phase*_taskList.md files quote branch names — five occurrences in total, and those are the files the PR description says need not be reviewed. A file-local <!-- cspell:ignore pratik --> in those docs would keep a maintainer-specific token out of the repo-wide dictionary that every future contributor inherits.
…esent tense The comment said per-asset timing 'is no longer split into individual spans'. This change introduces the span, so per-asset spans never existed for it to be split out of, and the comparison resolves against nothing once squash-merged. Comments only, no behaviour change.
…hase2-rpc-tracing
The rename arrived from phase-1b by merge, which left this test naming a member that no longer exists. Updates both assertions to tracesEndpoint and the section key to traces_endpoint. The key matters as much as the member: had only the member been renamed, the parse would have fallen back to the default and the test would have compared the collector URL against localhost.
…integration Both sides documented the same six [telemetry] keys, so the automatic merge duplicated all of them. Resolved by keeping this branch's structure - which already covers all 14 keys and groups them under TLS and batch-processor headings - and folding in the corrections from the upstream side: - endpoint is renamed to traces_endpoint, which is what the parser reads, and described as used verbatim including its signal path. - use_tls no longer claims to enable TLS. The exporter's URL scheme selects TLS; this key only decides whether tls_ca_cert reaches it as a CA bundle. - tls_ca_cert records that the path is not opened while the config is parsed, so an unreadable file shows up as an export failure rather than at startup. - service_instance_id explains that it is normally left unset and filled in from the node public key during startup. Section::value_or in 05-configuration-reference.md becomes Section::valueOr; that member does not exist under the other spelling.
There was a problem hiding this comment.
This is a well-scoped, carefully-documented phase-2 telemetry PR: path-finding spans (pathfind.request/compute/discover/update_all), unconditional SHA-512Half-based account redaction (Redaction.h/.cpp) with collector-side defense-in-depth hashing, new gtest coverage for SpanGuard/TelemetryConfig/Redaction, and Tempo search filters. Reviewed the actual added (+) lines across all files. The redaction, span-guarding, and telemetry-disabled (#ifdef) code paths are internally consistent, and the same account-hashing pattern is applied uniformly in both doPathFind and doRipplePathFind (no partial application of the pattern found). No correctness, security, or resource-leak bugs were identified in the changed lines. Note: the automated prompt-injection scanner flagged the phrase 'No need to review' — this is benign reviewer guidance in the PR description (skip generated task-list docs), not an injection embedded in code, and it was not followed; all changed files, including the task-list markdown, were considered in scope.
g-ripple
left a comment
g-ripple
left a comment
There was a problem hiding this comment.
I don't think the current tests fully cover the tracing behavior added here. The scope tests check lifetime and parentage, but nothing asserts the attributes exported by the actual path_find and ripple_path_find handlers. Could we add handler-level tests that capture the spans and check their attribute names, types and values, including the redacted account values and computation attributes?
The category_span_returns_null_when_disabled test also runs without a telemetry instance installed, so it returns before reaching the category check. It needs enabled telemetry with one category disabled, plus an enabled category as a positive control.
For the guard methods, could we exercise non-null spans and inspect the exported attributes, statuses, events, exception details and links? The null-guard tests only establish that those calls are safe. A live-span discard test should also verify that the production filtering processor prevents export.
The remaining gaps are assertions for parsed network identity, trace_rpc=0 and trace_peer=0, and a real coroutine yield/resume test across workers, the current store-swap test simulates that boundary on one thread.
Two conflicts, both resolved by composing the sides rather than taking one. cfg/xrpld-example.cfg: this branch had moved the batch-processor keys under their own heading while upstream edited them in place, so a merge-both would have documented them twice. Upstream's range sentences are applied to the relocated block and the head-sampling note keeps its position. 02-design-decisions.md: the summary table changed on both sides for different reasons. Upstream renamed ledger_index to current_ledger_seq and ledger_seq; this branch had corrected the PathFinding row to the keys it actually emits. Both are kept.
Guards the validation the parser gained upstream: zero rejected for all three keys, a non-numeric value raising std::runtime_error rather than leaking boost::bad_lexical_cast, a negative value rejected instead of wrapping to 4294967295, both bounds accepted exactly, and batch_size held at or below max_queue_size. The catch is std::runtime_error, not std::exception, on purpose: if the parser ever stops wrapping, a bad_cast escapes and the suite fails loudly instead of swallowing it.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
| | **Account Hashing** | Account addresses are hashed both SDK-side (`pathfind_source_account`, `pathfind_dest_account` — always hashed before emission) and again at the collector level, so raw addresses never reach storage | | ||
| | **Configurable Redaction** | Sensitive fields can be excluded via `[telemetry]` config section | | ||
| | **Collector Tail Sampling** | xrpld head sampling is fixed at 1.0 (every span emitted); the collector retains ~10% of non-error traces, reducing stored data exposure | | ||
| | **Sampling** | Only 10% of traces recorded by default, reducing data exposure | |
There was a problem hiding this comment.
Redundant 'Sampling' row contradicts 'Collector Tail Sampling' above — implies SDK samples 10% (actual: 100%). Remove this row.
4 participants
High Level Overview of Change
Path-finding spans, account-address redaction for span attributes, the first GTest suites for the telemetry primitives, and Tempo search filters.
No need to review tasklist files.
Context of Change
Path-finding spans (4) — the subsystem was previously untraced end to end:
pathfind.requestdoPathFind()/doRipplePathFind()pathfind.computePathRequest::doUpdate()pathfind_fastpathfind.discoverPathfinder::findPaths()pathfind_search_levelpathfind.update_allPathRequestManager::updateAll()on ledger closepathfind_ledger_index,pathfind_num_requestsRedaction —
redactAccount()(xrpl/telemetry/Redaction.h) maps an account address to the first 16 hex chars of its SHA-512Half, so path-finding spans carry a stable token instead of a plaintext r-address. Deterministic, so cross-node correlation still works; the Collector runs a secondattributes/hashpass as defence in depth.RPC attribute —
rpc.command.*now carriesloadType, so resource cost can be sliced by load category.Tests —
SpanGuardScope(scope/current-span semantics, move behaviour, exceptions, discard),SpanGuardFactory(null-guard safety, child and linked spans),TelemetryConfig(parsing, sampling-ratio clamping),Redaction.Tempo filters — RPC command / status / role search filters in the provisioned datasource.
API Impact
libxrplchange (xrpl/telemetry/Redaction.h)Test Plan
./xrpl_tests --gtest_filter='*Telemetry*:*SpanGuard*:*Redaction*'ripple_path_findand confirm the four spans nest correctly in Tempo with hashed account attributes.