feat: Phase 9: Internal metric gap fill — MetricsRegistry, ~68 new metrics, dashboards - #6513
feat: Phase 9: Internal metric gap fill — MetricsRegistry, ~68 new metrics, dashboards#6513pratikmankawde wants to merge 462 commits into
Conversation
pratikmankawde
added
DistributedTracingAndObservability
3c15d46 to
fe20cb4
Compare
fe20cb4 to
da5da5f
Compare
da5da5f to
4d05bf7
Compare
pratikmankawde
changed the title
pratikmankawde
changed the title
4d05bf7 to
c7468d5
Compare
c7468d5 to
5670dc4
Compare
5670dc4 to
ac3e5b9
Compare
ac3e5b9 to
1b7789a
Compare
…ound A ledger reaches totalAgreements or totalMissed only when its event is classified, which normally happens in reconcile() once the grace period has elapsed. The insert-path bound erased the oldest entry outright, so an event dropped before it was classified was counted as neither an agreement nor a miss and both lifetime totals under-reported. MaxPendingEventsTrimming shows it: 1100 recorded ledgers yielded 1000 agreements. Move the classification into classifyPending() and call it from both places, so a ledger reaches the totals exactly once whether reconcile() or the bound resolves it. Add a test that records past the cap without reconciling and asserts every evicted ledger still has a verdict -- the property the earlier bound test missed, because it only checked the record-time counter. Classifying on eviction fixes the verdict on whichever validations have arrived, so one still in flight can no longer complete or repair it. That only happens while over the bound, which means reconcile() is not running.
Two findings, both fatal under warnings-as-errors: misc-const-correctness on the counter in counter_starts_at_zero, which only ever reads it. Declare it const; the two counters that call add() stay mutable. readability-implicit-bool-conversion on the metrics-registry pointer in setValidLedger's guarded block. Compare against nullptr explicitly.
pendingCount() returns std::size_t and nothing in this file included <cstddef>, so misc-include-cleaner fails the build. The check only reports on a pull request's changed files, so the omission went unseen until an unrelated edit brought this file back into scope.
Every counter recordInsert() touches is declared only with telemetry compiled in, and its one caller sits inside the same guard, but the method itself was declared unconditionally. A telemetry-off build therefore failed to compile it: concurrentWriters, insertCount, insertTotalUs and insertMaxUs are all undeclared there. Guard the method with the state it maintains.
…ase9-metric-gap-fill
…ase9-metric-gap-fill
…etry The round and its two constants are reached only from the write-stats tests, which are inside the telemetry guard. Left outside it, the round is an unused function in a build without telemetry, and -Werror rejects it on both gcc and clang. Moves the guard boundary rather than annotating the round, so the build without telemetry carries no dead code. Mirrors NuDBBackend::recordInsert on the production side, which is guarded for the same reason.
…emetry clang-tidy runs over the whole tree in the build without telemetry, and reported nine findings that no other configuration can see. Eight are headers left included after the only code using them is compiled out: scope.h in the backend, and scope.h, Types.h, WriteStats.h, cstdint, latch, optional and thread in the test. Each moves into a guarded group instead of being deleted, since all are needed when telemetry is on. The ninth is acquireStats_. Its counters hold no state without telemetry, which leaves AcquireStats trivially default constructible, so the member needs an explicit initializer to avoid indeterminate values. Verified per file in both configurations: each finding reproduced before the change and is gone after it, and every file compiles with and without telemetry under -Werror -Wall -Wextra.
Without telemetry the counters hold no state, so AcquireStats is trivially default constructible. MSVC then rejects a const instance left to the compiler-generated constructor (C4269, fatal under -Werror), and clang-tidy reports the same type as an uninitialized member elsewhere. Braces settle both. Fixed at the two owners rather than by giving Counter a written-out constructor: an empty counter has no state to be indeterminate, so this is about what the compilers report, not about wrong values, and making Counter non-trivial would need a modernize-use-equals-default suppression in a header eleven files include. MSVC is not available here, so C4269 itself is unverified locally; both files compile under gcc with and without telemetry at -Werror -Wall -Wextra, and the clang-tidy member-init finding is confirmed gone.
Both sides added to the same guarded include block: this branch's get-object metric names, and the tx-tracing header arriving from phase-3. Kept both inside the one guard rather than taking a side.
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Review of the phase-9 metric gap-fill, done against branch tip 07669a8eda. Read-only; no files changed, no containers touched.
Counts: 0 BLOCKING, 2 SHOULD-FIX · High, 2 SHOULD-FIX · Medium.
GitHub returns no patch for any of the four files these findings sit in (node-health.json, consensus-health.json and the two new alerting YAMLs — the diff is too large), so I cannot anchor inline comments. Each finding below carries its own path:line. Every claim was verified by me at this ref with git show <ref>:<path>, and dashboard claims are asserted per panel, extracted by title.
[SHOULD-FIX · High] docker/telemetry/grafana/dashboards/node-health.json:415 and docker/telemetry/grafana/dashboards/consensus-health.json:1211 — two panels omit the reason!="" filter this PR's own runbook says is required, so the mismatch count reads double.
One handleMismatch() call increments two instruments that are meant to land in the same Prometheus family:
- the legacy beast counter —
src/xrpld/app/ledger/LedgerHistory.cpp:41makeCounter("ledger.history", "mismatch"), incremented unconditionally at:323, carrying noreasonlabel; - the new registry counter —
src/xrpld/telemetry/MetricsRegistry.cpp:358-359CreateUInt64Counter("ledger_history_mismatch_total", …), incremented at:1707asAdd(1, {{"reason", …}}), reached through therecordReasonlambda atLedgerHistory.cpp:330-332on exactly one of six mutually exclusive branches (:340,:362,:370,:387,:405,:412).
docs/telemetry-runbook.md:2424-2435 in this same PR documents the consequence and prescribes the remedy: "Both normalise to the same Prometheus family, so an unfiltered sum() or increase() reports exactly twice the real mismatch count. Aggregate over the labelled series only — sum by (reason) (...), or sum(ledger_history_mismatch_total{reason!=""})."
Neither panel applies it. Extracted by title from each file: "Ledger History Mismatches [$xrpl_network_type]" (stat, node-health :415) and "Ledger History Mismatch Rate by Reason" (timeseries, consensus-health :1211) both select ledger_history_mismatch_total{…} with no reason predicate — I checked each target's expr for the substring reason!= and both are false. The sum by (reason, …) panel has a second symptom: the unlabelled series joins the group as reason="", and because the legend is built with label_replace(…, "series", "$1", "reason", "(.*)"), it plots a seventh, unnamed series whose value is the sum of the other six. Whoever diagnoses a fork sees a phantom "reason" that is really the total.
Fix: add , reason!="" to the selector in both exprs — one token each, and exactly what the runbook already prescribes. Retiring the duplicate producer is the separate code fix the docs already record as pending.
One honest limitation: I could not execute-verify the step that makes the two names collapse into one family, because that happens in the collector's Prometheus exporter (docker/telemetry/otel-collector-config.yaml:231-238, which leaves add_metric_suffixes at its default) and proving it needs a running stack, which I would not disturb. So the family collapse is unverified by execution — but the duplicate incrementing is proven from the code above, the PR's own runbook asserts the collapse outright, and the suggested filter is correct and harmless either way.
[SHOULD-FIX · High] docker/telemetry/grafana/provisioning/alerting/rules.yaml:81 — the LedgerHistoryMismatch alert uses the exact unfiltered form the runbook warns about, and prints the doubled number into the operator's notification.
The query at :81 is sum by (service_instance_id) (increase(ledger_history_mismatch_total{service_name="xrpld"}[15m])), and the description at :70-71 interpolates the result: "recorded {{ $values.B.Value }} ledger history mismatch(es) in the last 15m". So the person reading the alert is told twice the real count.
This also corrects the runbook: docs/telemetry-runbook.md:2434 says "The alert rule is unaffected: it only tests > 0". The threshold is unaffected, but the reported value is not, and the value is what the operator acts on.
Mitigating context, which is why this is not blocking: the rule carries isPaused: true at :61 — and so does every rule in the file (14 isPaused: true occurrences against 13 title: entries), so nothing fires until someone unpauses it. Fix: add , reason!="" here too, and correct that one sentence in the runbook.
[SHOULD-FIX · Medium] docker/telemetry/grafana/provisioning/alerting/contactpoints.yaml:69 and :81 — the Slack titles interpolate .CommonLabels.rulename, a label nothing in this repo sets, so the title renders empty.
.CommonLabels is derived from the labels the alert instances carry. Every one of the 13 rules in rules.yaml sets only severity and category — I grepped every labels: block in the file and the complete key set is severity (3 critical, 10 warning) and category (consensus 3, jobqueue 3, node_state 2, overlay 3, validator 2). And grep -rn rulename docker docs OpenTelemetryPlan .github returns only three lines, all inside this file: the comment at :64 and the two titles. The repo's own grouping label is alertname (policies.yaml groups by it).
That makes the comment at :64-68 self-defeating: it explains that rulename was chosen instead of service_instance_id so the title survives NoData/Error evaluations, but the label it switched to is never populated in any state.
Unverified: whether Grafana injects a rulename label of its own for synthetic DatasourceNoData/DatasourceError alerts — I have no live Grafana here and did not want to disturb the running stack on this box. If it does, the titles work only for those synthetic alerts and are blank on the normal firing path, which is the opposite of the comment's intent. Either way no provisioned label supplies it.
Mitigating context: the webhook URLs are deliberate placeholders (https://hooks.slack.invalid/disabled at :63 and :80) and all rules are paused, so nothing is delivered yet. Fix: {{ if .CommonLabels.rulename }}…{{ else }}{{ .CommonLabels.alertname }}{{ end }}, then fire one rule in a live Grafana and read the delivered title.
[SHOULD-FIX · Medium] src/xrpld/telemetry/MetricMacros.h:377-402 (and the counter and up-down variants that follow at :404 onward) — the three XRPL_METRIC_OBSERVABLE_*_REGISTER macros drop the instrument handle, so a gauge registered through them can never emit.
:385 binds the result of CreateInt64ObservableGauge to the local auto xrpl_inst_, calls AddCallback on it at :386-399, and then the do { … } while (false) ends at :402 — the shared_ptr is the caller's only reference and it dies there. Nothing stores it.
The SDK does not keep the instrument alive for you. In the installed 1.28.0 headers, opentelemetry/sdk/metrics/state/observable_registry.h:44-46 takes a raw ObservableInstrument *, the registry holds std::unordered_map<uintptr_t, std::unique_ptr<ObservableCallbackRecord>> (:65), and CleanupCallback(ObservableInstrument *) exists at :60 precisely so the instrument's own destruction can withdraw its callback. So the callback goes away with the instrument.
This is also why MetricsRegistry itself works: all 19 of its observables are assigned to member fields (jqTransOverflowObservable_, cacheHitRateGauge_, txqGauge_, …) and each has its AddCallback on the very next line, registered eagerly from startAsyncGauges(). The macro is the only path that discards the handle.
Nothing is broken today — grep for XRPL_METRIC_OBSERVABLE_ across src and include, excluding the header itself and the tests, returns no production caller. The first person to use it gets a silently dead metric. Fix: have the macro hand the instrument to the registry (or any process-lifetime store) rather than letting it fall out of scope.
Related, worth a rename rather than a fix: the test at src/tests/libxrpl/telemetry/MetricMacros.cpp:274 is called observable_gauge_register_reports_current_value but its own comment at :290-292 says plainly that it "does NOT assert the observed" value and can only prove registration did not crash. The comment is honest; the name is not. Renaming it would stop it reading as coverage this defect should have failed.
Things I checked that are RIGHT, so nobody "fixes" them:
- This PR fixes a real counter overflow.
include/xrpl/nodestore/Database.h:370and:378declarefetchHitCount_andfetchSz_asstd::atomic<std::uint64_t>; ondevelopthey are stillstd::atomic<std::uint32_t>at:232-233. Commit579d9028e8does the widening and is an ancestor of this branch but not of phase-8. The arithmetic that makes it matter: a 32-bit byte accumulator wraps every 2^32 = 4,294,967,296 bytes, so at a few hundred bytes per read it wraps roughly every 12 million reads — many times a day on a busy node, which is why the derived bytes-per-read ratio could read below 1 byte, a physically impossible value for a SHAMap node. Anyone reviewing against the old u32 fact should note it no longer applies here. - Observable registration is eager and correctly ordered. 19 observable instruments, each with an
AddCallbackon the following line, all created insideregisterAsyncGauges().MetricsRegistry::start()(MetricsRegistry.cpp:200-230) deliberately creates only pushed instruments and documents the rule at:212-219, naming the exact past defect.startTelemetryGauges()is called atApplication.cpp:1575, afteroverlay_is built at:1557-1566and before the[rpc_startup]loop at:1647, with a comment at:1568-1574explaining why that window. Acatch(...)would not save an assert, and it does not need to. - The
[insight]semantics comment is accurate.docker/telemetry/xrpld-telemetry.cfg:111-114states thatserveris the only key that changes behaviour, that no prefix is applied, and thatservice_instance_idis read and discarded with the Prometheus label coming from[telemetry]instead — all of which matches the code. Fees::accountReservetakes two parameters (include/xrpl/protocol/Fees.h:53) and the call atMetricsRegistry.cpp:1408passes two.- This PR adds a dedicated
metrics_endpointconfig key (Application.cpp:1729), which resolves an open thread on the phase-7 PR about the metrics endpoint being derived by a silent string suffix swap.
Existing threads: I re-verified the three open ones and left all three open, because all three are still live at this tip — the renderer port publish at docker/telemetry/docker-compose.yml:190, the two new dependency loops this branch adds to loops.txt, and the validator-health UID appearing under both the Phase 9 and Phase 11 tables in 09-data-collection-reference.md (:1944 and :1957, while its three Phase 11 siblings all carry the xrpld- prefix). Details are in each thread.
|
|
||
| ### Step 2: Start xrpld in standalone mode | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
About the command on the next line: this PR turns xrpld-telemetry.cfg into a Devnet config, so -a --start no longer matches the file's own usage note.
Through phase-8 the cfg's header documented exactly this invocation (./xrpld --conf docker/telemetry/xrpld-telemetry.cfg -a --start). This PR rewrites it into a Devnet config — it gains [network_id], [ips] and [validators_file], and its header now says:
rippled/docker/telemetry/xrpld-telemetry.cfg
Lines 7 to 13 in 07669a8
— no -a, and "Wait for sync (server_state=full)". Test 1 here (and Test 2 Step 2) still pass -a --start.
Standalone itself still works with this file: [validators_file] is skipped for standalone in Config, [port_peer] is stripped in ServerHandler, and [ips] is only read behind a !standalone() gate. The real problem is the store. --start sets StartUpType::Fresh, which writes a genesis chain into the same docker/telemetry/data/nudb and docker/telemetry/data that the Devnet run uses — and the sibling mainnet cfg added in this PR documents avoiding precisely that:
rippled/docker/telemetry/xrpld-telemetry-mainnet.cfg
Lines 96 to 100 in 07669a8
So a standalone Test 1 followed by a Devnet run (or the reverse) opens one NuDB with two different chains in it.
Either fix works, but the two files should agree: give Test 1 its own cfg with a data/standalone prefix, or keep this cfg and have both the cfg header and Test 1 name the same invocation, with a note that -a --start destroys the Devnet store.
|
|
||
| Expected: > 0 results. | ||
|
|
||
| > **Use `service_name`, not `job`.** The collector's `resource/logs` processor |
There was a problem hiding this comment.
The conclusion of this note is right, but the mechanism it gives is wrong — the base config no longer upserts job at all.
Four specifics:
-
resource/logssets one attribute, not two. Inotel-collector-config.yamlit isservice.name: xrpld / upsertand nothing else.grep -n 'key: job'over both collector configs hits onlyotel-collector-config.grafanacloud.yaml— thejobupsert exists solely on the cloud config, which is why a{job=...}selector is worth warning about there and not here. Thejobkey was removed from the base config on phase-8 (d4282cc36e), i.e. before this note was written. -
The cited comment says the opposite of what the note attributes to it. The block above the attribute does not claim
jobis there for operators to paste; it records the negative result:{job="xrpld"}matched 0 streams,jobarrives as structured metadata, promoting it would mean mounting a Loki config, "Select onservice_name." -
otel-collector-config.yaml:57-70points at the wrong thing. Those lines are the filelogregex_parseroperator.resource/logsstarts at line 67 and its attribute runs to 86. -
docker-compose.yml:75likewise. Line 75 is a comment inside the collector's volume block; thelokiservice is at line 112 and itscommand: -config.file=/etc/loki/local-config.yamlat 116. (The claim itself — no Loki config override is mounted — is correct.)
Everything downstream of the mechanism holds: service_name is the promoted label, job cannot be a stream selector, and {job="xrpld"} returning zero with no error is a genuine trap. Suggest rewriting the first two sentences to say that only the Grafana Cloud config stamps job, and that Loki does not promote it to a stream label in either case, then keeping the rest and dropping the two line references (or pinning them with permalinks, since they have already drifted once).
| | **Transaction** (cont.): `tx.receive` | `trace_transactions` | A **peer** relays a transaction. Never appears in standalone — submit on one node of the cluster and look on another. | T2 | | ||
| | **Transaction** (cont.): `tx.apply` | `trace_transactions` | Ledger close with a non-empty transaction set: submit, then `ledger_accept` (T1) or wait for consensus (T2). | T1 / T2 | | ||
| | **TxQ** (6): `txq.enqueue`, `txq.apply_direct`, `txq.batch_clear`, `txq.accept`, `txq.accept_tx`, `txq.cleanup` | `trace_transactions` | `txq.enqueue`/`apply_direct` on every submission; `txq.accept`/`accept_tx`/`cleanup` on every ledger close. To force real queueing, submit faster than ledgers close or with a fee below the required fee level. | T1 | | ||
| | **Consensus** (13): `consensus.round`, `.phase.open`, `.establish`, `.update_positions`, `.check`, `.proposal.send`, `.ledger_close`, `.accept`, `.accept.apply`, `.validation.send`, `.mode_change`, `.proposal.receive`, `.validation.receive` | `trace_consensus=1` | Requires real consensus — **standalone emits none of these**. Bring up T2 and wait for nodes to reach `proposing`; one `consensus.round` per close. `.mode_change` needs an actual mode transition (stop/start a node). | T2 | |
There was a problem hiding this comment.
"standalone emits none of these" is not right — five consensus families do fire on a standalone ledger_accept.
ledger_accept is handled by NetworkOPsImp::acceptLedger, which asserts standalone_ and then drives a round by hand:
rippled/src/xrpld/app/misc/NetworkOPs.cpp
Lines 4648 to 4649 in 07669a8
beginConsensus→startRound→startRoundTracing→consensus.round, andConsensus::startRoundInternal→consensus.phase.open.consensus_.simulate→closeLedger({})→Adaptor::onClose→consensus.ledger_close; thenonForceAccept→makeAcceptSpan→consensus.accept→doAccept→consensus.accept.apply.
simulate sets phase_ = ConsensusPhase::Accepted directly and never enters phaseEstablish, so the families that really are T2-only are .establish, .update_positions, .check, .proposal.send, .proposal.receive, .validation.receive and .mode_change.
Two other places in this PR carry the same claim and need the same correction:
- the note below, "
consensus.*andpeer.*cannot be produced in standalone mode" — thepeer.*half is correct, theconsensus.*half is not; - the Test 1 "Expected spans (standalone mode)" table row
| consensus.* | No | Consensus disabled standalone |(inherited from the phase-6 PR, commented there too).
Suggested split of this row, since the two halves have different triggers and different Test columns:
| | **Consensus** (13): `consensus.round`, `.phase.open`, `.establish`, `.update_positions`, `.check`, `.proposal.send`, `.ledger_close`, `.accept`, `.accept.apply`, `.validation.send`, `.mode_change`, `.proposal.receive`, `.validation.receive` | `trace_consensus=1` | Requires real consensus — **standalone emits none of these**. Bring up T2 and wait for nodes to reach `proposing`; one `consensus.round` per close. `.mode_change` needs an actual mode transition (stop/start a node). | T2 | | |
| | **Consensus** (13, 5 here): `consensus.round`, `.phase.open`, `.ledger_close`, `.accept`, `.accept.apply` | `trace_consensus=1` | `ledger_accept` in standalone drives a simulated round via `NetworkOPsImp::acceptLedger`, so these five fire without peers; also one per close in T2. | T1 / T2 | | |
| | **Consensus** (cont.): `.establish`, `.update_positions`, `.check`, `.proposal.send`, `.proposal.receive`, `.validation.receive`, `.mode_change` | `trace_consensus=1` | Requires real consensus — `simulate` jumps straight to `Accepted`, so standalone emits none of these. Bring up T2 and wait for `proposing`. `.mode_change` needs an actual mode transition (stop/start a node). | T2 | |
|
|
||
| - **Traces**: Explore → hosted Tempo datasource → search `{resource.service.name="xrpld"}` | ||
| - **Metrics**: Explore → hosted Prometheus/Mimir → query `span_calls_total` | ||
| - **Logs**: Explore → hosted Loki → query `{service_name="xrpld"}` (requires `warning`+ file logging). **Not `{job="xrpld"}`** — see the note under Test 3 Step 3. |
There was a problem hiding this comment.
"requires warning+ file logging" is not a real precondition, and points the wrong way.
Nothing in the pipeline filters on severity. grep -n 'severity_number\|min_severity\|severity_text' returns zero hits in both otel-collector-config.yaml and otel-collector-config.grafanacloud.yaml; the filelog receiver just tails /var/log/xrpld/*/debug.log with start_at: beginning, and its regex_parser only captures severity into an attribute. Whatever is in the file is ingested.
The process default is already above warning anyway — Main.cpp sets Severity thresh = Severity::Info before any config is read, and only --quiet/--verbose move it. And warning is not a shared convention across the shipped configs: xrpld-telemetry.cfg sets debug, xrpld-telemetry-mainnet.cfg sets warning.
More importantly the requirement is backwards for what this bullet is about. Trace-correlated log lines only exist where a line is emitted inside an active span, and the dependably correlated one is the consensus accept pair, which logs at info. At warning and above that pair is suppressed and correlation becomes incidental — which is why later branches in this chain move integration-test.sh from warning to info and document info as the minimum. So warning is the one level that makes this bullet's check unreliable.
| - **Logs**: Explore → hosted Loki → query `{service_name="xrpld"}` (requires `warning`+ file logging). **Not `{job="xrpld"}`** — see the note under Test 3 Step 3. | |
| - **Logs**: Explore → hosted Loki → query `{service_name="xrpld"}` (requires xrpld to be writing `debug.log` under the mounted log root; use `log_level info` or lower — at `warning` the consensus accept pair that reliably carries trace context is suppressed). **Not `{job="xrpld"}`** — see the note under Test 3 Step 3. |
| cmake --build . --target xrpld | ||
| ``` | ||
|
|
||
| Conan also writes a `conan-release` preset, so `cmake --preset conan-release -Dtelemetry=ON` works too. There is no preset named `default`. |
There was a problem hiding this comment.
Minor: both halves of this sentence are true, but the preset is unusable at the working directory this section establishes.
Line 13 says "From a build directory (.build/)", and the two commands above are written for that cwd. From .build/ the preset cannot be resolved:
$ cd .build && cmake --list-presets
CMake Error: Could not read presets from <repo>/.build:
File not found: <repo>/.build/CMakePresets.json
Presets resolve only from the repo root, via the gitignored CMakeUserPresets.json that includes .build/build/generators/CMakePresets.json. From the root, cmake --list-presets does list exactly one preset and no default, so the naming claim is correct.
Second mismatch: the preset's binaryDir is .build/build/Release, so cmake --preset conan-release does not produce .build/xrpld as line 23 promises — only the hand-written cmake line above does.
Worth either dropping the sentence or qualifying it, e.g. "From the repo root, cmake --preset conan-release -Dtelemetry=ON also works; note it builds into .build/build/Release, not .build/. There is no preset named default."
| # otel-collector health | ||
| curl -sf http://localhost:13133/ && echo "collector ready" | ||
| # otel-collector readiness: any HTTP response on the OTLP/HTTP port means the | ||
| # receiver is listening. Do NOT use `curl -sf` here — a GET of / returns 404, |
There was a problem hiding this comment.
This readiness check and its rationale are added twice in this PR, and the two copies already differ.
Both are new here — neither exists on phase-6 or phase-8. Copy 1 is this block; copy 2 is § Troubleshooting → "No traces in Tempo" step 3:
3. Check that otel-collector port 4318 is accessible (`-f` would fail on the
receiver's 404 for `GET /`, so test for any HTTP status instead):
curl -so /dev/null -w '%{http_code}\n' http://localhost:4318/
Same insight, stated twice in prose, and the commands have already diverged: this one tests != "000" and echoes collector ready, the other just prints the code with a trailing newline and asserts nothing. Only one of them is a check. Suggest keeping the assertion form here and reducing the troubleshooting entry to a pointer back to Step 1, or vice versa.
Related, and the reason it is worth fixing rather than tolerating: this file now carries two inventories of what fires in standalone — the Test 1 "Expected spans (standalone mode)" table inherited from phase-6, and the "Span → How to Trigger" table plus its notes added by this PR. The note below already cross-references the older table ("see 'Expected spans (standalone mode)' above"), so the single-source intent is there; only the per-span data is restated. That restatement is exactly why the standalone consensus.* error I flagged separately has to be corrected in three places across two PRs instead of one. Whichever table is authoritative, the other should carry the toggle/trigger columns only and defer for the expectations.
These comments dated themselves against this branch or against an earlier revision of the same change, neither of which survives a squash merge. - 'as of this branch' in the pricing-case doc and the pass-through static_assert becomes a statement about what the constants currently produce. - 'Extracted from processGetObjectByHash()' and 'Split from start()' describe edits internal to this change; both now say why the method stands alone. - Recording.h: the mock-abstract mismatch is a standing consequence of a member set that differs between builds, not something that 'has previously' happened. - MetricsRegistry.cpp: describe the loops.txt entry as recording two cycles rather than as what ordering.txt 'previously had'. - InboundLedger.h: the acquire span is the only signal for back-fill cost; it did not 'previously emit' nothing. - check_bucket_parity.py: replace the eleven-phase drift story with the reason the check exists, and point the failure message at HistogramBuckets.h and the collector config instead of OpenTelemetryPlan/, which does not reach develop. Comments and one error message only, no behaviour change.
…g the table The rule's rationale said its three points were 'learned from a dataset that an earlier version of this table got wrong'. That earlier table exists only in this branch's intermediate commits, which a squash merge does not publish, so the sentence points at nothing a reader can check. The measured figures behind each point are unchanged. Documentation only.
setNodeId() and Counter::add() silenced an unused parameter with a (void) cast in their telemetry-disabled arm. The attribute is this codebase's idiom: it appears 105 times across 40 files on develop. Parameter names stay, because the enabled arm uses them and the @PARAM lines name them. totalAgreementsEver() and totalMissedEver() are pure accessors and gain [[nodiscard]]; no caller discards either.
xrpl::to_string(ClosedInterval) renders a one-ledger range as a bare sequence number with no dash. The gauge's parser required a dash, so it dropped that range: a node holding a single complete ledger published no series at all, and because the index counter only advanced on emitted segments, every later range's index label shifted down by one. parseLedgerRange() now treats a dashless segment as a range of one ledger. It is inline in the header because MetricsRegistry.cpp is not compiled into the unit-test binary, so an out-of-line definition could not be tested. It uses std::from_chars rather than std::stoll: stoll threw out of the whole callback, losing every remaining range that collection cycle, where from_chars costs only the segment it cannot read. Six tests cover both emitted shapes, the refused shapes, the sequence limits, and a round trip through the real producer. The same file's unused-parameter casts in the telemetry-disabled stubs move to [[maybe_unused]]; eight more cast the member enabled_, which isEnabled() reads outside every #ifdef, so they suppressed nothing and are deleted.
Eighteen conflict regions across nine files. Resolved by asking, per region, which side is the better final state rather than by taking a branch wholesale. Telemetry.cpp keeps phase-9's two resource builders. phase-8 offered a single makeResource() with no node identity; phase-9 splits it into makeTracerResource() and makeMetricsResource() because the metrics provider is built in the constructor, before setNodeId() runs, so xrpl.node.id can only be stamped unconditionally on the tracer side. Collapsing them would have dropped that attribute, which is what keeps per-node traces from folding into one identity. Telemetry.h and the config test compose both sides: phase-9's nodeId member and its assertion, plus the renamed endpoint. xrpld-telemetry.cfg keeps phase-9's devnet identity and its metrics_endpoint, renames the traces key, and drops exporter=otlp_http. Nothing reads an `exporter` key on any branch in the chain: 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. The cfg line was the last carrier. The docs keep phase-9's versions, which are both fuller and more accurate: the incoming runbook listed the consensus strategy values as "random" where the code compares against "attribute". OTelCollector.cpp had five comment-only regions in a file phase-7 owns, so those take the upstream side. MetricsRegistry.h's usage example named a member that no longer exists and the wrong arity; it now matches the real three-argument call and says where the endpoint comes from.
Telemetry.cpp conflicted. Phase-9 rewrote the metrics pipeline into makeTracerResource()/makeMetricsResource()/initMetrics() further down the class, so its side of the region is empty and phase-8's private helper block does not apply. Resolved to phase-9's structure; phase-8's own hunks outside the region (the deleted kTracesPath/kMetricsPath, the verbatim traces URL, the two-endpoint startup log) merged in. Phase-9's initMetrics() still derives the metrics URL by suffix-swap. That is fixed in the next commit, not here.
initMetrics() held its own copy of the suffix-swap: take tracesEndpoint, replace a trailing /v1/traces with /v1/metrics. Any other URL shape sent metrics to the traces path. metrics_endpoint is now a config key of its own, so use it as given. Application::startTelemetry() still reads the key itself for MetricsRegistry, which builds its own exporter. Telemetry exposes no accessor for the Setup it parsed, and re-parsing would re-run the mTLS validation and cert-file checks at a later point in startup, so the second read stays. The comment above it no longer claims the URL is derived from the traces one. Also corrects 05-configuration-reference.md: the sample configs carry metrics_endpoint where exporter used to sit, not a comment.
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.
…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.
…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.
3 participants
High Level Overview of Change
Fills the metric gaps: values that existed only in
get_counts/server_info/ TxQ / PerfLog now export as time series through a centralMetricsRegistry. Adds five dashboards, thirteen provisioned alert rules, a Grafana Cloud export path, and aledger.acquirespan.No need to review tasklist files.
Context of Change
Phases 1-8 built the pipelines; the signals operators actually page on were still only reachable by polling RPC.
MetricsRegistry(src/xrpld/telemetry/MetricsRegistry.{h,cpp}) owns theMeterProviderand a 10sPeriodicExportingMetricReader, and uses a hybrid approach: synchronous instruments where there is a call site to record at (PerfLog, nodestore I/O), async observable gauges where the value is an existing atomic to poll.write_load,read_queue, backend write statsserver_infoparityserver_info,build_info,complete_ledgers,db_metrics— parity with the externalpush_metrics.pyinripplex-ansible, so that script can be retiredledger.acquirespan +AcquireStats— makes inbound-ledger acquisition observable (reason, peer count, timeouts, outcome); this is the path that shows up as syncing↔full flapping.Dashboards — five new (fee-market, job-queue, peer-quality, validator-health, log-derived-insights) and eleven updated. Every panel has real units, and the perf-iac template variables are added throughout;
$nodefilters onservice_instance_id.Alerting — thirteen rules provisioned from
docker/telemetry/grafana/provisioning/alerting/, loaded on container start, each groupedby (service_instance_id)so a node alerts on itself:LedgerHistoryMismatch,LedgerCloseStalled,ValidatedLedgerStale,ValidationsMissed,ValidationsNotChecked,JobQueueTxOverflow,JobQueueLatencyHigh,NodeStoreIOLatencyHigh,NodeStateFlapping,NodeNotFull,ManifestJobQueueConvoy,ManifestFloodInbound,PeerResourceDisconnects. Thresholds and their measured baselines are documented in the runbook's Alerting section.Grafana Cloud —
docker-compose.grafanacloud.yaml+ an alloy config layer export to Cloud alongside the local stack; credentials stay in gitignored.envfiles.Docs / CI — runbook Alerting section,
docs/telemetry-glossary.md,09-data-collection-reference.md; the otel-naming check is extended (dashboard and doc layers) with its pytest suite grown to match.API Impact
libxrplchange (nodestoreWriteStats,JobQueue/PerfLogcounters,ServiceRegistry)Test Plan
MetricsRegistrystarts and stops with the Application; async callbacks fire on the 10s interval../xrpl_tests --gtest_filter='*MetricsRegistry*:*MetricMacros*:*AcquireStats*:*NuDBFactory*:*Backend*:*Database*'