feat: Phase 7: Native OTel metrics migration — OTelCollector, OTLP pipeline, dashboard rename - #6493
feat: Phase 7: Native OTel metrics migration — OTelCollector, OTLP pipeline, dashboard rename#6493pratikmankawde wants to merge 281 commits into
Conversation
5913274 to
1bb5087
Compare
1bb5087 to
322348d
Compare
322348d to
125c808
Compare
pratikmankawde
added
DraftRunCI
125c808 to
451d19e
Compare
451d19e to
320f907
Compare
320f907 to
d693dcb
Compare
d693dcb to
4e554c0
Compare
4e554c0 to
3f5f367
Compare
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
| check_statsd_metric "rippled_LedgerMaster_Published_Ledger_Age" | ||
| check_statsd_metric "rippled_jobq_job_count" | ||
| # Node health gauges (ObservableGauge — no _total suffix) | ||
| check_otel_metric "rippled_LedgerMaster_Validated_Ledger_Age" |
There was a problem hiding this comment.
Test queries use old metric format. OTelCollector outputs lowercase, unprefixed names. Update check_otel_metric calls:
| check_otel_metric "rippled_LedgerMaster_Validated_Ledger_Age" | |
| check_otel_metric "ledgermaster_validated_ledger_age" |
ValidationTracker carries no telemetry guards and neither does its test, so both are compiled whatever the telemetry setting. Its implementation and the src/ include path it needs were added only inside if(telemetry), so a build with telemetry off could not reach the header and left fifteen of its symbols unresolved at link time on every platform. Hoist both out of the guard. The else() branch already does this for MetricsRegistry, so the case was anticipated there and missed here.
NullTelemetry overrides the other OTel virtuals behind the telemetry guard so it stays concrete in both configurations, but getMeter was added to the base without a matching override. That leaves the class abstract in a telemetry-on build, which compiles today only because its sole instantiation sits behind #ifndef. Anyone constructing one with telemetry on gets an abstract-class error pointing at the base, not at the missing override. Return a NoopMeter from a function-local static, mirroring getTracer.
formatName() never reads prefix, so setting it here does nothing and the exported names are bare and lowercase. Leaving it invites queries written against xrpld_jobq_job_count, which match no series. The StatsD examples keep it, because that path does apply it to the name.
OTelCollector routes every instrument name through a static formatName() that only lowercases the name and maps '.' and space to '_'. The sole read of prefix_ is the startup log line at OTelCollector.cpp:802, and all four instrument factories go through formatName(), so no prefix can ever reach an exported name. StatsDCollector does prepend it, so the StatsD example keeps the key and now states why. Covers the three server=otel blocks in the 09 reference and the config integration-test.sh generates. This branch introduces OTelCollector, so it is where the inert examples first appear; phase-6's examples are all server=statsd and stay as they are.
The class documented a name format it does not produce. Eleven comments said the [insight] prefix is prepended, and gave examples like "xrpld_rpc_size" and "xrpld_LedgerMaster_Validated_Ledger_Age" that also kept the original casing. formatName() lowercases the raw instrument name and maps dots and spaces to underscores, and applies no prefix. All four instrument factories go through it, so "RPC.Size" exports as "rpc_size". prefix_ is written in the constructor and read in one place, the startup log line, so nothing it holds can reach an exported name. The service is identified by the service.name resource attribute. Comments only, in both the header and the implementation. The ASCII diagram still lists prefix_ as a member, which it is.
| "query": "label_values(xrpl_work_item)", | ||
| "datasource": { | ||
| "type": "prometheus", | ||
| "uid": "${DS_PROMETHEUS}" |
There was a problem hiding this comment.
Lines 639, 659, 679: datasource uid uses undefined ${DS_PROMETHEUS} instead of literal prometheus — variable population will fail.
| "uid": "${DS_PROMETHEUS}" | |
| "uid": "prometheus" |
| "uid": "${DS_PROMETHEUS}" | |
| "uid": "prometheus" |
| "query": "label_values(xrpl_branch)", | ||
| "datasource": { | ||
| "type": "prometheus", | ||
| "uid": "${DS_PROMETHEUS}" |
There was a problem hiding this comment.
[high] xrpl_branch variable uses unresolved datasource uid "${DS_PROMETHEUS}"
Same copy-paste issue as the xrpl_work_item variable above: xrpl_branch's datasource uid is set to "${DS_PROMETHEUS}" instead of the literal "prometheus" uid used elsewhere in this dashboard. With no matching input/variable defined, this variable's label_values(xrpl_branch) query will fail to resolve a datasource and the dropdown will not populate.
Suggestion: Use the literal uid "prometheus" for consistency with the rest of the dashboard.
| "uid": "${DS_PROMETHEUS}" | |
| "uid": "prometheus" |
| "query": "label_values(xrpl_node_role)", | ||
| "datasource": { | ||
| "type": "prometheus", | ||
| "uid": "${DS_PROMETHEUS}" |
There was a problem hiding this comment.
[high] xrpl_node_role variable uses unresolved datasource uid "${DS_PROMETHEUS}"
Same pattern repeats a third time: xrpl_node_role's datasource uid is "${DS_PROMETHEUS}" rather than the literal "prometheus" uid used by service_name, deployment_environment, xrpl_network_type, and node. This will break population of the xrpl_node_role dropdown for the same reason as the other two instances.
Suggestion: Use the literal uid "prometheus" for consistency with the rest of the dashboard.
| "uid": "${DS_PROMETHEUS}" | |
| "uid": "prometheus" |
| } | ||
|
|
||
| /** | ||
| * Override the service instance id, for callers that learn it late. |
There was a problem hiding this comment.
Service.instance.id set after construction bypasses metrics resource — traces and metrics will report different resource identities.
| * Override the service instance id, for callers that learn it late. | |
| // Move MeterProvider construction to start() (after setServiceInstanceId) | |
| // to ensure metrics resource picks up the final instance ID |
| "query": "label_values(xrpl_work_item)", | ||
| "datasource": { | ||
| "type": "prometheus", | ||
| "uid": "${DS_PROMETHEUS}" |
There was a problem hiding this comment.
Same datasource uid issue: xrpl_work_item, xrpl_branch, xrpl_node_role use undefined ${DS_PROMETHEUS} instead of prometheus.
| "uid": "${DS_PROMETHEUS}" | |
| "uid": "prometheus" |
| "uid": "${DS_PROMETHEUS}" | |
| "uid": "prometheus" |
| | `peer_finder_active_outbound_peers` | PeerfinderManager.cpp | Active outbound peer connections | 10–21 | | ||
| | `overlay_peer_disconnects` | OverlayImpl.cpp | Cumulative peer disconnection count | Low growth | | ||
| | `overlay_peer_disconnects_charges` | OverlayImpl.cpp | Disconnects due to resource limit charges | Low growth (subset of above) | | ||
| | `job_count` | JobQueue.cpp | Current job queue depth | 0–100 (healthy) | |
There was a problem hiding this comment.
Metric name mismatch in table: lists job_count but should be jobq_job_count to match section 2.5 and configuration examples.
| | `job_count` | JobQueue.cpp | Current job queue depth | 0–100 (healthy) | | |
| `jobq_job_count` |
| | `job_count` | JobQueue.cpp | Current job queue depth | 0–100 (healthy) | | |
| | `jobq_job_count` | JobQueue.cpp | Current job queue depth | 0–100 (healthy) | |
| // Hard trim if still over limit. The loop above already removed every | ||
| // reconciled entry older than the late-repair window, so here we drop | ||
| // any remaining reconciled entry as a last resort. | ||
| if (pending_.size() > kMaxPendingEvents) |
There was a problem hiding this comment.
Hash_map iteration is unordered — trim may evict recent reconciled entries instead of oldest ones, losing recoverable validation data.
| if (pending_.size() > kMaxPendingEvents) | |
| std::deque<uint256> reconciled_order_; // track order so eviction removes oldest first |
| { | ||
| initMetrics(); | ||
| } | ||
| catch (std::exception const& e) |
There was a problem hiding this comment.
On init failure, meterProvider_ isn't reset — partial provider remains accessible via getMeter() while global provider stays noop, causing divergence.
| catch (std::exception const& e) | |
| meterProvider_.reset(); // in catch block to ensure both paths fall back to noop |
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Review of the phase-7 native-metrics migration, done against branch tip 0eccf294bc. Read-only; no files changed.
Counts: 2 BLOCKING, 3 SHOULD-FIX inline, plus 4 SHOULD-FIX / NIT items below that could not be anchored inline.
Every claim here was re-verified at this branch rather than taken from an earlier review; dashboard claims were checked per panel (extracted by title) rather than by file-wide grep, and each asserted absence names the grep that returns nothing. Separately, I triaged the 31 open threads on this PR: 10 are now answered and resolved, 21 stay open because they are still live.
Findings that could not be anchored inline (09-data-collection-reference.md's patch is too large for the API to return, and cfg/xrpld-example.cfg is not in the diff):
- [SHOULD-FIX · Medium]
OpenTelemetryPlan/09-data-collection-reference.md:529— "Thebeast::insight::Collectorinterface and all metric names are preserved — only the wire protocol changed" is not what this PR does: it renames every beast metric from prefixed-and-case-preserved to bare lowercase. The diff for this very file shows- | xrpld_jobq_job_count …replaced by+ | job_count …, and line 71 of the same file states the opposite ("their names carry noxrpld_prefix"). An operator who reads :529 will keep dashboards and alerts on the old names. - [SHOULD-FIX · Medium]
OpenTelemetryPlan/09-data-collection-reference.md:830— the$noderow maps toexported_instance; the real label isservice_instance_id(297 target matches across the ten boards, 0 forexported_instance). Detail in the inline comment onCollectorManager.cpp:57. - [SHOULD-FIX · Low]
OpenTelemetryPlan/09-data-collection-reference.md:701and:704— both link to#2-statsd-metrics-beastinsight, an anchor this PR removed when it renamed the heading at:525from "## 2. StatsD Metrics (beast::insight)" to "## 2. System Metrics (beast::insight — OTel native)". Two dead links. Already fixed on phase-9/phase-10 (0 occurrences there). - [SHOULD-FIX · Low]
cfg/xrpld-example.cfg:1270— the shipped example config still tells users "Currently the only choice isstatsd" for[insight] server. This PR addsotelas a second choice, so the user-facing documentation of the option becomes wrong here. The file is untouched by this PR and the sentence is still wrong at the tip of the chain, so nobody downstream has picked it up either. - [NIT · Low] Three renames in this PR change output outside telemetry and are not mentioned in the PR body:
src/xrpld/overlay/detail/TrafficCount.h:285(getobject_Fetch Pack_gettogetobject_Fetch_Pack_get, surfaced as thecategoryfield of thetrafficPropertyStream set),src/xrpld/app/main/Application.cpp:399(Cached SLEstoCached_SLEs) andsrc/xrpld/shamap/NodeFamily.cpp:30,38. On the OTel path they are no-ops because the name formatter already maps spaces to underscores; on the StatsD path they change the metric names, and the PropertyStream one is user-visible either way. One line in the PR body would cover it.
One resolved thread that is still live, worth reopening rather than re-reporting: the thread on node-health.json:592 about legendFormat using {{quantile}}. At this tip the four job panels still read e.g. "Accept Ledger [{{quantile}}]" while the query is histogram_quantile($quantile, …). histogram_quantile output carries no quantile label, so the legend renders as Accept Ledger []. The sibling complaints in that thread (missing quantile template variable, unit: ms vs microseconds) were genuinely fixed — the variable now exists as a custom var and the four panels declare unit: µs — so only the legend part remains.
Not raised again because an open thread already covers it: the rippled_* metric names in docker/telemetry/integration-test.sh:594-609; job_count vs jobq_job_count in 09-data-collection-reference.md:588; the undefined ${DS_PROMETHEUS} in network-traffic.json and overlay-traffic-detail.json; the mixed datasource in ledger-data-sync.json; ValidationTracker having no production caller, its post-eviction double count, its unenforceable kMaxPendingEvents, its eviction order and its nine O(n) scans under the mutex; the partially-built MeterProvider surviving the init catch in Telemetry.cpp; the silent metrics-endpoint suffix swap; and the MeterProvider leak on assertion failure in GetMeter.cpp.
| "datasource": { | ||
| "type": "prometheus" | ||
| }, | ||
| "expr": "job_count{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}", |
There was a problem hiding this comment.
[BLOCKING · High] — The "Job Queue Depth" panel queries job_count, but the exported name is jobq_job_count, so this panel is permanently empty.
The gauge is created through the jobq group, and the group prefixes the name before the collector ever sees it:
src/xrpld/app/main/Application.cpp:386passescollectorManager_->group("jobq")into theJobQueue.src/libxrpl/core/detail/JobQueue.cpp:39then callscollector_->makeGauge("job_count")on that group.src/libxrpl/beast/insight/Groups.cpp:80-83forwards it asmakeName(name), andmakeNameisname_ + "." + name(Groups.cpp:42-46), givingjobq.job_count.src/libxrpl/beast/insight/OTelCollector.cpp:1001-1020maps.to_and lowercases, so the series isjobq_job_count.
The group segment is not optional on either transport, so the bare name matches nothing. The same PR's own reference doc has the matching defect (OpenTelemetryPlan/09-data-collection-reference.md:588), which is already tracked in a separate open thread.
Fixed downstream: node-health.json on pratik/otel-phase9-metric-gap-fill queries jobq_job_count (1 occurrence, 0 bare). This PR still needs the same one-word fix to stand alone.
| "datasource": { | ||
| "type": "prometheus" | ||
| }, | ||
| "expr": "histogram_quantile($quantile, sum by (le, service_instance_id) (rate(job_running_us_bucket{job_type=\"acceptLedger\", service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))", |
There was a problem hiding this comment.
[BLOCKING · High] — Four new panels (24 targets) query job_running_us_bucket / job_queued_us_bucket, which nothing on this branch emits, so they render empty for anyone who opens this dashboard.
Asserted absence, with the command: grep -rn 'job_running_us\|job_queued_us' src include cmake docker/telemetry/otel-collector-config.yaml returns no output at this branch. The names first appear on pratik/otel-phase9-metric-gap-fill, in src/xrpld/telemetry/MetricsRegistry.cpp:115-116 (kJobQueuedDurationUs / kJobRunningDurationUs) — a file that does not exist here (grep -rln MetricsRegistry src include cmake returns nothing).
Affected panels, extracted by title from this file rather than by a file-wide grep: "Key Jobs Execution Time" (11 targets, job_running_us_bucket), "Key Jobs Dequeue Wait Time" (11 targets, job_queued_us_bucket), "All Jobs Execution Time (Detail)" and "All Jobs Dequeue Wait (Detail)" (1 each).
Two ways to make this PR self-consistent: move these four panels to the PR that introduces the metrics, or keep them and say in the panel descriptions that they are inert until then. Shipping a dashboard where four of its panels can never draw is the part worth avoiding — an operator cannot tell "no data" from "node is idle".
|
|
||
| // Read service_instance_id, same key as the [telemetry] | ||
| // section uses, so multi-node deployments can distinguish | ||
| // metric sources via the exported_instance Prometheus label. |
There was a problem hiding this comment.
[SHOULD-FIX · Medium] — This comment promises an effect that does not exist, next to a value the callee throws away. An operator setting [insight] service_instance_id will get nothing and no warning.
Two independent problems:
- The value is discarded.
src/libxrpl/beast/insight/OTelCollector.cpp:794-800reads: "instanceId/serviceName/networkType are retained on the New() signature for back-compat but no longer used here: the telemetry module owns the resource attributes for the shared metrics pipeline", followed by(void)instanceId;at:797. Soservice_instance_id,service_nameand the network type read at:58-64here have no effect on the OTel path; onlyserver=selects the collector. - The
exported_instancelabel does not exist.grep -rn exported_instance src include docker docs OpenTelemetryPlanreturns exactly two hits: this comment, andOpenTelemetryPlan/09-data-collection-reference.md:830, which tells operators$nodemaps toexported_instance. Every dashboard actually filters onservice_instance_id: extracting each panel's target exprs and counting(\w+)=~"$node"givesservice_instance_id297 times across the ten boards indocker/telemetry/grafana/dashboards/, andexported_instance0 times.
Fix: say plainly that these three keys are inert on the OTel path and that resource attributes come from [telemetry], and correct the $node row in 09-data-collection-reference.md:830 to service_instance_id.
Both sites are already fixed on pratik/otel-phase9-metric-gap-fill (0 occurrences in either file there); phase-8 still carries both, so the fix is needed here to keep this PR self-contained.
| m.doProcess(); | ||
|
|
||
| sendBuffers(); | ||
| sendBuffers(); |
There was a problem hiding this comment.
[SHOULD-FIX · Medium] — Moving sendBuffers() inside the polling_ guard means buffered samples are never sent outside the polling window: metrics recorded during startup pile up unsent, and anything recorded after shutdown begins is lost. This affects the server=statsd path, which this PR keeps as a supported fallback.
Why the guard matters here: Event, Counter, Gauge and Meter do not reach the wire through the doProcess() loop above — they call impl_->postBuffer(...) directly (:554, :594, :642, :718), which appends to data_ at :345. grep -n data_ src/libxrpl/beast/insight/StatsDCollector.cpp gives only 218 (the declaration), 345 (the only append) and 394-405 (inside sendBuffers), so sendBuffers() is the single drain.
polling_ starts false (:227) and is set true only in onCollectionReady() (:267-270), then back to false in onCollectionStopping() (:272-280). So every sample recorded before the app calls onCollectionReady() sits in data_ and grows, and every sample after onCollectionStopping() is dropped. Before this PR the flush was unconditional — on pratik/otel-phase6-statsd the same function has sendBuffers(); outside any guard (line 445 there).
Fix is one line: leave sendBuffers() outside the if. The reason the guard was added, per polling_'s own docstring at :222-226, only concerns the doProcess() loop, not the drain.
Still unchanged on pratik/otel-phase9-metric-gap-fill, so the same fix is wanted there after merge-forward.
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Second half of my review of this PR — the dashboard, docs and config side. I posted the C++/build half earlier today; splitting it was not deliberate, the two review lanes simply finished at different times, and I did not want to hold verified blockers back. Nothing here repeats a comment from the first review.
Counts in this review: 2 BLOCKING, 3 SHOULD-FIX. (Running total for the PR: 4 BLOCKING, 6 SHOULD-FIX inline, plus 4 lower-severity items I listed in the first review's summary.)
All evidence was re-derived directly from pratik/otel-phase7-native-metrics at 0eccf294bc with git show <ref>:<path>, and I re-verified all 130 line citations behind both reviews mechanically against that ref (one pass, zero mismatches) after the local checkout was rearranged mid-review. Dashboard claims are asserted per panel, extracted by title.
Theme worth reading the five comments together for: this PR makes a good, deliberate decision — bare lowercase metric names, with service.name identifying the node instead of a xrpld_ prefix — and the C++ carries it through cleanly. What has not caught up is everything that describes the decision. The phase task list still specifies the old scheme (Phase7_taskList.md:152), the phase plan still lists 64 old names (06-implementation-phases.md), the configuration reference still tells operators to set the now-inert prefix key (05-configuration-reference.md:256), and the integration test asserts the old names (already in an open thread). One sweep over the docs closes all of it, and the task list is the highest-value one because it is the spec a future reader follows.
The two BLOCKING items are independent of that theme: seven panels in the new ledger-data-sync.json query metrics that only exist two PRs later, and transaction-overview.json loses a panel that nothing in the chain restores.
Verified and deliberately NOT raised, so nobody re-litigates them:
xrpld_in dashboards: 0 occurrences across all ten boards. The fivestatsd-*boards that queried prefixed names are deleted by this PR, and the collector config here has no StatsD receiver (receivers: [otlp]/[otlp, spanmetrics]), so no dashboard is orphaned.exported_instancein dashboards: 0 occurrences; all ten filter onservice_instance_id=~"$node"(297 target matches). The stale references are in the docs and one code comment, which I raised in the first review.allValue: ".*"on template variables does NOT break a single-node local run — an absent PromQL label behaves as the empty string, solabel=~".*"matches series that lack the label. Any review comment claiming otherwise is wrong.- The "millisecond ladder saturates at 5000 / RPC size is censored" concern is fixed by this PR:
src/xrpld/rpc/detail/ServerHandler.cpp:191now passesUnit::Bytes, andsrc/libxrpl/telemetry/Telemetry.cpp:437-444registers one view per unit so a byte count no longer inherits a latency ladder. - The runbook's dashboard UID references: this PR repairs three that were dangling at phase-6.
| "type": "prometheus", | ||
| "uid": "${DS_PROMETHEUS}" | ||
| }, | ||
| "expr": "label_replace(label_join(label_replace(state_tracking{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\", xrpl_work_item=~\"$xrpl_work_item\", xrpl_branch=~\"$xrpl_branch\", xrpl_node_role=~\"$xrpl_node_role\", metric=\"state_value\"}, \"series\", \"Server State\", \"\", \"\"), \"xrpl_ident\", \", \", \"service_instance_id\", \"xrpl_branch\", \"xrpl_work_item\"), \"xrpl_ident\", \"[$1]\", \"xrpl_ident\", \"(?:, )*(.*[^, ])(?:, )*\")" |
There was a problem hiding this comment.
[BLOCKING · High] — Seven panels in this new dashboard query five metric families that nothing on this branch emits, so the whole "Sync Diagnostics" half of the board is blank.
Extracted per panel from this file (not by file-wide grep), the affected titles and the series each needs:
- "Sync State" →
state_tracking - "Time Spent Per State" →
server_info - "Ledger Close Rate" →
ledgers_closed_total - "NuDB Read Latency", "NuDB Read Found Ratio", "NuDB Read Pressure" →
nodestore_state - "Load Factor & Peers" →
load_factor_metrics
Asserted absence, with the command: grep -rn '<name>' src include --include=*.cpp --include=*.h returns nothing for state_tracking, ledgers_closed_total, nodestore_state and load_factor_metrics at this branch, and the only server_info hits are the RPC handler registration (src/xrpld/rpc/detail/Handler.cpp:283) and an include path — no instrument.
All five are created by MetricsRegistry, which arrives two PRs later: on pratik/otel-phase9-metric-gap-fill they are src/xrpld/telemetry/MetricsRegistry.cpp:351 (ledgers_closed_total), :766 (load_factor_metrics), :939 (nodestore_state), :978 (server_info) and :1447 (state_tracking). That file does not exist here — grep -rln MetricsRegistry src include cmake returns nothing.
The rest of this dashboard is fine: the overlay/getobject panels above query real beast::insight series, and the job-queue panels correctly use the jobq_<jobtype>_milliseconds_bucket form.
Suggested fix: move the seven Sync Diagnostics panels to the PR that adds the metrics, or keep them and say in each panel description that they stay empty until then. As shipped, an operator cannot distinguish "no data" from "node is idle".
|
|
||
| - In `OTelCollector.cpp`, construct OTel instrument names to match existing Prometheus metric names: | ||
| - beast::insight `make_gauge("LedgerMaster", "Validated_Ledger_Age")` → OTel instrument name: `xrpld_LedgerMaster_Validated_Ledger_Age` | ||
| - The prefix + group + name concatenation must produce the same string as `StatsDCollector`'s format |
There was a problem hiding this comment.
[SHOULD-FIX · High] — Task 7.5 specifies the exact opposite of what the code does, and it is the reason the integration test in this same PR asserts names that cannot exist.
This line and its neighbours instruct: make_gauge("LedgerMaster", "Validated_Ledger_Age") should become the OTel instrument xrpld_LedgerMaster_Validated_Ledger_Age (:151), "The prefix + group + name concatenation must produce the same string as StatsDCollector's format" (:152), separators should match the StatsD convention (:153), and the integration test should assert xrpld_LedgerMaster_Validated_Ledger_Age, xrpld_Peer_Finder_Active_Inbound_Peers and xrpld_rpc_requests (:156-158).
The shipped code deliberately does not do that. src/libxrpl/beast/insight/OTelCollector.cpp:1001-1020 only lowercases and maps ./space to _, and its contract says so at :561-563: "The [insight] prefix is not applied; the service.name resource attribute identifies the service." The dashboards agree — grep -o 'xrpld_' over each of the ten boards in docker/telemetry/grafana/dashboards/ returns 0.
Why this matters beyond the doc being wrong: docker/telemetry/integration-test.sh:594-609 asserts exactly the three names this task list dictates, which is why that script cannot pass (already raised in a separate open thread). Fixing the task list is what stops the same mistake being made again — this file is the spec a reader will follow.
Suggested fix: rewrite Task 7.5 to state the actual rule (bare, lowercase, group segment retained, _total added by the Prometheus exporter) and update its three sample queries. Note this text was never corrected downstream either — the same lines are still on pratik/otel-phase9-metric-gap-fill.
|
|
||
| **What to do**: | ||
|
|
||
| - Ensure `xrpld_Overlay_Peer_Disconnects_Charges` appears in the StatsD-to-Prometheus metric name mapping |
There was a problem hiding this comment.
[SHOULD-FIX · Medium] — This document carries 64 xrpld_-prefixed metric names that this branch's collector can never emit, so the phase plan and the code now disagree about every system metric.
Count, with the command: grep -o 'xrpld_[a-zA-Z_]*' OpenTelemetryPlan/06-implementation-phases.md | wc -l gives 64 at this branch and 0 on pratik/otel-phase9-metric-gap-fill, which is where the sweep was eventually done. The names cannot exist here because src/libxrpl/beast/insight/OTelCollector.cpp:1001-1020 adds no prefix (contract at :561-563), and the ten dashboards in this PR query bare lowercase names throughout (grep -o 'xrpld_' per file returns 0 for all ten).
Two other things in this file worth folding into the same pass:
:485uses<br/>inside agraph LRnode label. Flowchart labels are markdown strings and need a real newline;<br/>is only valid insequenceDiagram,classDiagram,gantt,gitGraphandquadrantChart.:1262says thestate_valueencoding is "0-7" and:1476bounds itin [0, 7], while the table at:1269-1278defines exactly seven values 0 through 6 and the prose at:1267says "extends this to 0-6". 7 is not a legal value. (This one is already tracked in an open thread whose original file was folded into this document.)
Suggested fix: run the same de-prefixing sweep here that phase-9 later applied, and correct the two items above while in the file.
A comment that describes an earlier state of its own branch documents something no reader can look up, because the branch is squash-merged and the state it contrasts against never reaches the merged history. Four passages in HistogramBuckets.h and one in Telemetry.cpp did exactly that, and the OTelCollector and Unit.h wording implied a transition rather than a fact. - HistogramBuckets.h: the ladders now explain the invariant they enforce, instead of recounting where the edges used to live and how they drifted. - Telemetry.cpp: one view per unit means a byte count is bucketed on the byte ladder, stated directly rather than as something it stopped inheriting. - OTelCollector.cpp: the collector is described by what it is, a thin adapter over the shared pipeline, rather than as a shim that gave up an exporter. - Unit.h: the StatsD path is out of service as a present fact, and the contract its wire format would break is an external protocol one. Comment text only. No declaration, signature or emitted value changes.
The class docs for the OTel insight bridge described how the code got to its current shape rather than what it does. Those comparisons resolve against a revision that the squash merge does not publish, so they read as confidently wrong once merged. - OTelCollector: state that it is selected by [insight] server=otel as an alternative to StatsDCollector. It replaced nothing; CollectorManager still selects StatsDCollector for server=statsd. - Unit: give the reason an Event needs an explicit unit, and the consequence of omitting one, without narrating what the two backends previously assumed. - OTelEventImpl: say that HistogramBuckets.h is the single owner of the bucket edges and why a copy goes stale, instead of quoting a superseded edge list. - HistogramBuckets: drop 'just as 5 s censors them today'. The millisecond ladder tops at 120000, so nothing is censored at 5 s. Comments only, no behaviour change.
…the collector Harness and docs: - integration-test.sh queried traces_span_metrics_* for spanmetrics, but this branch sets the connector namespace to "span", so those two checks matched nothing and failed. The dashboards and runbook had moved; the script had not. - The same script queried eight native metric names with a product prefix and capitals that formatName() cannot produce: it lowercases, maps '.' and ' ' to '_', and prepends nothing. Corrected against the runbook tables. - TESTING.md carried the same stale spanmetrics names and a jq example reading a Prometheus label that does not exist. - The runbook now records where each part of a derived metric name comes from, since only the namespace is ours to choose. Collector: - OTelCounterImpl::increment silently dropped a negative amount. An OTel counter takes unsigned deltas, so assert and let a release build under-count rather than wrap. - OTelGaugeImpl::increment computed current + amount in int64, which is undefined on overflow, and the clamp ran afterwards so it could not help. Check the headroom first. set() now clamps rather than casting a uint64 above INT64_MAX to a negative, which is what made underflow reachable. - The meter scope was two bare literals. They are constants now, and Telemetry.cpp static_asserts them equal to kMeterName and kMeterVersion: beast cannot include the telemetry header, so a build failure is the only way to catch the copies drifting. - formatName uses views::transform and ranges::to, as Backend.cpp already does. - Unused constructor parameters take [[maybe_unused]] instead of (void) casts. - The destructor logged "shutting down" and "stopped" with nothing between. initMetrics was 79 lines doing four jobs. The exporter and the histogram views are separate functions now, addUnitView is a member rather than a lambda capturing this, and the export interval and timeout are named. It also derived the metrics URL from the traces URL by suffix swap, which sent metrics to the traces path whenever the configured URL had any other shape; both URLs now come from one rule that handles a bare host, a trailing slash and either signal path.
… accessors nodiscard Nine std::count_if calls over window1h_ and window7d_ took an iterator pair; std::ranges::count_if takes the container. One hand-rolled erase-while-iterating loop becomes a single std::erase_if: pending_ is a hash_map, which is a std::unordered_map alias, so the C++20 overload applies. The hard-trim loop below it is left as a loop on purpose. It re-tests pending_.size() every step to stop as soon as the cap is met, which erase_if cannot express. The accessors return values a caller must use: 13 in ValidationTracker.h, the two Unit.h mappers, the two HistogramBuckets.h helpers, getMeter() and networkTypeFromId(). No caller in the chain discards any of them.
Three conflicts, all composed rather than resolved by taking a side: - TelemetryConfig.cpp: phase-6 kept networkTypeFromId file-local with [[nodiscard]]; phase-7 had relocated it to public scope for Application.cpp. Kept phase-7's relocation, so one definition remains. The [[nodiscard]] survives on the declaration in Telemetry.h. - Telemetry.cpp x2: phase-7 added getMeter overrides, phase-6 added [[nodiscard]] to the startSpan below them. Kept both, and put [[nodiscard]] on getMeter too. - TESTING.md: phase-7 had the right metric name (span_calls_total, which the spanmetrics namespace produces) but the wrong label. Its xrpl.rpc.command appears nowhere else in the branch; the attribute is bare `command`, which is what the dashboards query. Took phase-7's metric with the correct label. Both signalEndpoint call sites follow the renamed member. signalEndpoint itself is left in place: removing it and adding metrics_endpoint is a design change, not part of propagating a rename.
One [telemetry] key served both OTLP signals, and the metrics URL was derived from it by suffix-swap: strip a trailing slash, strip a known signal path if present, append the wanted one. Anything not ending /v1/traces therefore posted metrics to the traces path, and the OTLP version was pinned in code where an operator could not reach it. Adds metrics_endpoint alongside traces_endpoint. Both are full URLs used verbatim, so traces and metrics can go to different collectors, or to one whose OTLP paths are not the defaults. signalEndpoint(), kTracesPath and kMetricsPath are gone; nothing derives an endpoint from another. The startup log names both URLs, since with two independent endpoints there was otherwise no way to see where metrics were going. Also drops exporter=otlp_http from the shipped config and the test fixture. No branch in the chain reads an `exporter` key: it was a real Setup member in the first phase-1b implementation, removed when only OTLP/HTTP was wired up, and already deleted from TESTING.md once on the same grounds.
…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.
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.
|
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. |
| - "8889:8889" # Prometheus metrics (spanmetrics + OTLP) | ||
| # StatsD UDP port removed — beast::insight now uses native OTLP. | ||
| # Uncomment if using server=statsd fallback: | ||
| # - "8125:8125/udp" |
There was a problem hiding this comment.
Fallback StatsD line exposes unauthenticated receiver on all interfaces if uncommented. Restore loopback binding:
| # - "8125:8125/udp" | |
| # - "127.0.0.1:8125:8125/udp" |
| telemetry::makeTelemetrySetup( | ||
| config_->section("telemetry"), | ||
| "", // Updated later via setServiceInstanceId() | ||
| nodePublicKey.value_or(""), |
There was a problem hiding this comment.
Metrics and traces report different instance.id with 3-arg makeApplication — breaks service correlation. Ensure all callers pass resolved key:
| nodePublicKey.value_or(""), | |
| // Resolve nodeIdentity before Telemetry construction so both metrics and traces | |
| // receive the same instance.id, or ensure all makeApplication call sites pass | |
| // the resolved node public key instead of std::nullopt |
|
|
||
| EXPECT_EQ(tracker_.totalAgreements(), 3u); | ||
| EXPECT_EQ(tracker_.totalMissed(), 2u); | ||
| EXPECT_EQ(tracker_.agreements1h(), 3u); |
There was a problem hiding this comment.
After reconciling these three agreements and two misses, all three windows should report 3 agreements, 2 misses and 60%, but the assertions check only the 1h window. Elsewhere in the suite, the 24h getters are checked only on an empty tracker, and none of the 7d getters are called; removing either longer window's insertion or late-repair update would therefore leave the existing assertions satisfied. Extend this case to assert both longer windows, check their miss-to-agreement transition in LateRepair, and include the 7d getters in EmptyWindowReturnsZero. These scenarios already perform the necessary reconciliation, so the extra assertions require no additional sleeps.
4 participants
High Level Overview of Change
Replaces the StatsD hop with native OTel metrics:
OTelCollectorimplementsbeast::insight::Collectoron the Metrics SDK and exports over OTLP/HTTP to the same endpoint as traces. The StatsD-derived dashboards lose theirstatsd-prefix.No need to review tasklist files.
Context of Change
Phase 6 routed the existing insight metrics through a StatsD receiver. That worked but meant UDP, protocol translation, and a second ingestion path.
OTelCollector(xrpl/beast/insight/OTelCollector.h, ~1.1k LOC) removes it by mapping insight types onto SDK instruments behind the unchangedCollectorinterface:Counter<int64_t>ObservableGauge<int64_t>(async callback)Histogram<double>, 1-5000 ms bucketsCounter<uint64_t>CollectorManagergains aserver=otelbranch;server=statsdstill works. Guarded byXRPL_ENABLE_TELEMETRY, falling back toNullCollector. Metric names are preserved, so existing panels and alerts keep resolving.ValidationTracker— new (src/xrpld/telemetry/ValidationTracker.*): tracks per-round validation agreement so validator health can be measured rather than inferred.Dashboards — the five
statsd-*boards are renamed tonode-health,network-traffic,rpc-pathfinding,ledger-data-sync,overlay-traffic-detail(the metrics no longer come from StatsD, so the prefix was misleading), and the remaining boards are updated for the OTLP label set. Panel units audited; perf-iac template variables ($xrpl_work_item,$xrpl_branch,$xrpl_node_role) added — these labels come from perf-iac's own alloy pipeline and default to All, so local runs render unchanged.API Impact
libxrplchange (newbeast::insightcollector implementation; interface unchanged)Test Plan
telemetry=TrueandFalse; the latter falls back toNullCollector.[insight] server=otel→ metrics arrive in Prometheus via OTLP;server=statsdunchanged../xrpl_tests --gtest_filter='*GetMeter*:*ValidationTracker*'integration-test.shpasses.