feat: OTel integration in XRPLD - #7770
Conversation
pratikmankawde
changed the title
pratikmankawde
added
DraftRunCI
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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. |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
/ai-review |
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Full review of the OTel integration. Overall quality is high: span cardinality is mostly bounded by design, lock ordering and shutdown sequencing are reasoned about explicitly, and tests assert exact values on both positive and negative paths.
Findings are posted inline, grouped by severity:
- High (1): raw WS
commandreaches a spanmetrics dimension (unbounded Prometheus cardinality) — ServerHandler.cpp - Medium (5): dual metric pipelines with divergent resource attributes; per-tx span events truncating at the SDK limit; stale CI path filters; telemetry default-ON vs. contradicting comment; internal planning docs in the PR
- Low (6): doc contradictions,
detachCallbacks()guarantee overstated, inconsistent span activation on peer receive paths,repairWindowEntrylinear scan, unguardedfindByType, inconsistent hash-size policy
Verified as sound (no action needed): JobQueue collect() snapshot pattern avoiding lock-order inversion; PerfLog signature change propagated to all implementations and tests; rpcEnd early-return fix; RPC method/job handler label bounding; Application shutdown ordering (detachCallbacks → services → registry stop → telemetry stop); secrets hygiene in docker/telemetry; GTest suites.
Recommended pre-merge priority: fix the WS command bounding first (blocking), then decide the telemetry build default, align the two metric pipelines' resources, fix the CI filters, and drop the planning docs.
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Follow-up to my earlier review: an in-depth security and performance impact pass focused on the peer/client attack surface, data leakage, and hot-path costs. New findings are inline (2 security, 2 performance). Two items have no single code anchor, so they live here:
Benchmark evidence should gate the merge (Medium). All overhead claims in this PR are design-level: the disabled-at-runtime path is verified allocation-free (one atomic load + one virtual call per span site), and enabled-mode volume/queue sizing looks bounded on paper. But the PR ships docker/telemetry/workload/benchmark.sh precisely to prove this. Suggest attaching its output to the PR before merge: (a) develop vs. compiled-in-but-disabled (expect noise-level delta), and (b) enabled under mainnet-shaped load (expect low single-digit % CPU).
Runbook note on peer trace parentage (Low). trace_context from peers is advisory and unauthenticated by design (correctly outside the signed payload, strictly shape-validated, memory-safe). Since the overlay is permissionless, any peer can seed arbitrary trace IDs that telemetry-enabled nodes adopt for their receive spans — forgeable parentage and Tempo trace pollution. TLS doesn't change this (the authenticated peer is the author). Worth an explicit runbook warning that cross-node trace parentage is untrusted data, and possibly a future option to honor it only from cluster/trusted peers.
Verified safe during this pass (no action): full span-attribute inventory contains no RPC bodies/secrets/seeds; log correlation is one-directional (trace IDs into logs only); zero new listening sockets (telemetry is outbound push only); peer-supplied trace context parsing is memory-safe with double validation; span/metric queues are bounded with drop-on-overflow; exporter supports one-way and mutual TLS via config; opentelemetry-cpp/1.26.0 pinned via Conan; CI workflows use workflow_dispatch/same-repo push only (no pull_request_target on self-hosted runners); metric callbacks read atomics/cache snapshots without heavyweight locks; ValidationTracker worst-case memory ~8 MB.
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
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. |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
90cb71d to
d3ff791
Compare
NetworkOPs.cpp include block: develop replaced DeliveredAmount.h, MPTokenIssuanceID.h and NFTSyntheticSerializer.h with the single rpc/detail/SyntheticFields.h. Kept that and this branch's two telemetry includes.
Two conflicts, both additions at the same spot: NetworkOPs.cpp include block kept develop's rpc/detail/SyntheticFields.h alongside this branch's telemetry/MetricsRegistry.h; .cspell.config.yaml kept both new words.
kMetricExportInterval and kMetricExportTimeout were declared but the reader options set the same values as literals, so both constants were unused. constexpr at namespace scope has internal linkage, so clang reports them under -Wunused-const-variable, which -Dwerr=ON makes fatal: it failed the compile on ubuntu-clang-release-amd64 and macos-arm64-release, and clang-tidy as well. Using them removes two magic numbers and keeps the comment that explains why the interval is 1 s.
… range tests clang-tidy reported six bugprone-unchecked-optional-access errors: the dataflow analysis does not treat ASSERT_TRUE(x.has_value()) as establishing the precondition, because the assertion's early return is hidden inside a macro. Three sites now guard with a plain if + FAIL(), the form already used elsewhere in this file and one the analysis does model. The fourth site compares the whole optional instead. That covers both "was it parsed" and "are the bounds right" in one exact assertion, and because it is EXPECT rather than ASSERT every row of the table is now checked -- previously one bad row returned from the test and hid the other five. parseLedgerRange uses std::errc, which comes from <system_error>; the header included only <charconv>, which misc-include-cleaner flagged.
…ase10-workload-validation
…ser names The commented [telemetry] block in cfg/xrpld-example.cfg documented 8 of the 14 keys the parser accepts. Add the six that were missing - service_instance_id, use_tls, tls_ca_cert, batch_size, batch_delay_ms and max_queue_size - each with the unit and default read from the parser, and rename the documented endpoint key to traces_endpoint so it matches what makeTelemetrySetup reads. use_tls is documented for what it does rather than what its name suggests: it gates whether tls_ca_cert reaches the exporter as a CA bundle, while the scheme of traces_endpoint is what selects TLS. The path is not opened during parsing, so an unreadable file surfaces as an export failure at runtime. 05-configuration-reference.md named three symbols that do not exist: setup_Telemetry, make_Telemetry and Section::value_or. Correct them to makeTelemetrySetup, makeTelemetry and Section::valueOr.
…ers on The Consensus Health template-variable table documented $node as resolving via exported_instance. That dashboard defines $node as label_values(target_info, service_instance_id) and its panels filter on service_instance_id; exported_instance appears in it zero times. exported_instance is a real label, but it belongs to the StatsD boards shipped alongside, where Prometheus renames a scraped instance label that collides with the target's own. Documenting it against an OTel dashboard pointed readers at the wrong pipeline's label.
…dence keys Addresses review findings on the native-metrics work. StatsDCollector::onTimer drained the send buffer inside the polling_ gate. That gate holds back hook handlers until the application's services are built, but sendBuffers() is socket I/O. StatsDEventImpl derives only from EventImpl, so it never enters metrics_ and posts straight to the buffer; its |ms timings piled up before onCollectionReady and were dropped after onCollectionStopping. The drain now runs every tick, and outside metricsLock_, so onCollectionStopping no longer waits on a UDP flush. TelemetryImpl's constructor left meterProvider_ set when initMetrics() threw. initMetrics publishes globally as its last step, so a throw left getMeter() callers holding a provider nothing else could reach. Reset it in the catch. ~ApplicationImp caught only std::exception around telemetry shutdown while the callees reach third-party SDK code, so a foreign exception would have terminated the process. Added a logging catch-all. ValidationTracker's hard trim evicted by unordered_map bucket order. It now evicts oldest-first, so the entry dropped under pressure is the one least likely to still reconcile. The GetMeter test restored the global meter provider only on the success path, and ASSERT_TRUE early-returns past it. Uses xrpl::ScopeExit instead. The hook debounce window is a named constant rather than a bare 500 in a comparison, and the metric export cadence becomes operator-configurable through metric_export_interval_ms and metric_export_timeout_ms. Both are range-checked: the SDK warns and silently substitutes its own 60s/30s defaults when the timeout is not below the interval, so an unchecked value would slow export rather than speed it up. Parsing uses a signed representation because lexical_cast<uint32_t> accepts a leading minus and wraps it. Naming corrections: CollectorManager documented exported_instance, which no OTel dashboard uses; node-health queried job_count where the exported name is jobq_job_count; network-traffic and overlay-traffic-detail referenced an undeclared DS_PROMETHEUS variable; the counter table omitted the _total suffix the Prometheus exporter appends; the plan docs and task list carried an xrpld_ prefix formatName never applies; and OTelCollector::New()'s contract promised its instanceId, serviceName and networkType arguments were read, contradicting the definition that marks them unused.
…icsRegistry The native-metrics pipeline built its own OTLP/HTTP exporter and set only the URL, so an operator who enabled TLS got mutual TLS on the trace exporter and a plaintext-configured exporter for metrics. cfg/xrpld-example.cfg promises TLS for "the OTLP exporter connection" with no carve-out, and two exporters exist. The exporter now reads the same four [telemetry] TLS keys the trace exporter does. Its resource was also thinner than the trace resource: service.name was hardcoded, and service.version, xrpl.network.id and xrpl.network.type were absent. Because the collector promotes resource attributes to labels, an operator setting service_name split their fleet - spans carried the configured name while every XRPL_METRIC_ series still said xrpld, blanking native-metric panels in every dashboard that filters on it. All four now come from config, and xrpl.network.type is derived inside from network_id through the shared networkTypeFromId so a caller cannot supply a mismatched pair. start() and initExporterAndProvider() take a StartOptions aggregate rather than growing to eleven positional parameters, seven of them same-typed strings where a swap would compile silently and stamp the wrong label - the defect class this change exists to remove. It is constructed at one production site, so replacing it with Telemetry::Setup once that is exposed stays a single-site edit. service.version, service.instance.id and xrpl.node.id are stamped only when non-empty, keeping this pipeline's existing behaviour of omitting an attribute rather than writing it blank. Also corrects three plan-doc references that cited line numbers rather than symbols; the reader line moved and the numbers differ per branch.
…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.
phase-6 corrected the Consensus Health template-variable table to name service_instance_id, the label that dashboard actually filters on. This branch removes that table entirely - the section is restructured around a Prometheus-label reference and a pointer to the runbook - so the corrected row has nothing to land in. Resolved by keeping the restructured section; phase-6's fix remains correct for phase-6, where the table still exists. The [telemetry] cfg block merged without conflict: the composed 14-key block from upstream and this branch's metrics_endpoint, metric_export_interval_ms and metric_export_timeout_ms entries coexist, 17 keys with one entry each.
…orrelation Nine files conflicted. Resolutions, and why: Telemetry.cpp - upstream carried its own initMetrics(), makeResource() and makeMetricExporter(); this branch already has an initMetrics() that builds the exporter inline and uses makeMetricsResource(), which stamps xrpl.node.id only when it is already known. Keeping both would have defined initMetrics() twice. Kept this branch's, then pointed its reader at setup_.metricExportInterval and setup_.metricExportTimeout: upstream turned those constants into [telemetry] keys and removed the old ones, so the previous spelling no longer resolves. OTelCollector.h - kept upstream's parameter docs. This branch's text promised instanceId, serviceName and networkType become resource attributes; the constructor marks all three [[maybe_unused]] and the .cpp already says they are not read. CollectorManager.cpp - kept upstream's comment for the same reason. node-health.json - kept this branch's 60 panels. Upstream's only change to the file was job_count to jobq_job_count, which this branch already had. cfg/xrpld-example.cfg - composed. Kept this branch's warning that service_instance_id must be set explicitly for the metrics pipeline, took upstream's traces_endpoint rename, and removed a duplicate metrics_endpoint entry along with the claim that metrics derive from the traces URL by rewriting the signal path. Nothing derives it; both metric exporters read metrics_endpoint. 17 keys, one entry each. 05-configuration-reference.md - both sides misdescribed the parser. Kept this branch's fuller text, corrected the endpoint default to traces_endpoint, and replaced the "resolve their URL differently" table with what the code does now. 09-data-collection-reference.md, 06-implementation-phases.md, Phase7_taskList.md - kept this branch's versions, which drop a metric that was never implemented, correct the state encoding to 0-6, and rename nudb_bytes to stored_object_bytes. Re-applied upstream's rpc_requests_total fix, which taking this side had reverted.
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
…json The close-time span attributes name their unit and epoch: close_time_ripple_epoch_s, parent_close_time_ripple_epoch_s and close_time_self_ripple_epoch_s. The harness inventory still required the unsuffixed keys, so the attribute checks for consensus.accept.apply and ledger.build failed on every validation run while the spans themselves were correct. Rename the four required_attributes entries to the keys the code emits.
2 participants
High Level Overview of Change
Full OpenTelemetry integration for
xrpld: distributed traces, native OTel metrics, and log-trace correlation, with a companion observability stack (Collector, Tempo, Prometheus, Loki, Grafana) and a workload harness that proves the end-to-end signal path.Grafana dashboards
No need to review tasklist files.
Context of Change
xrpldhad StatsD counters and free-form logs — no traces, no structured attributes, and no way to tie a slow RPC to the consensus round it landed in.TraceContextprotobuf message; consensus rounds share a deterministic trace ID so all nodes in a round land in one trace.OTelCollectorimplementsbeast::insight::Collectoron the OTel Metrics SDK, replacing the StatsD hop;MetricsRegistryadds the values that previously existed only inget_counts/server_info/ TxQ / PerfLog, reaching parity with the externalpush_metrics.py.trace_id/span_idon log lines, ingested into Loki 3.x via OTLP, with two-way Grafana links to Tempo.by (service_instance_id), a Grafana Cloud export path, an operator runbook and a glossary.Delivered as ten reviewed phases (see the chain above); plan docs live under
OpenTelemetryPlan/. Reviewing the phase PRs in order is far easier than reading this diff.Enablement — the build option
telemetryis currently ON so CI keeps compiling this code, but a stock node emits nothing: the runtime[telemetry] enabledkey defaults to0, and metrics need[insight] server=otel.API Impact
libxrplchange — adds telemetry primitives (SpanGuard/ScopedSpanGuard,*SpanNames.h,Redaction,OTelCollector) and instrumentation-only changes to nodestore,JobQueueandPerfLog; no existing symbol changed.TraceContextmessage, field 1001 reserved onTMTransaction/TMProposeSet/TMValidation. Peers that never set it are unaffected.No wire-format or JSON-RPC schema change.
Test Plan
docker/telemetry/workload/run-full-validation.shbrings up 5 validators plus the full stack, drives RPC/transaction load, and asserts every expected span, attribute and metric emits.