The microsecond ladder's first edge was 100us, which sat ABOVE the mass of
every instrument using it. Measured on devnet: 99.3% of job_queued_us
samples, 92.5% of job_running_us and 90.4% of getobject_lookup_us fell in
that first bucket. histogram_quantile then interpolated inside bucket 0 and
returned `quantile / fraction_in_bucket_0 x first_edge` -- p75/p95/p99 of
job_queued_us read 75.52/95.66/99.69us against a prediction of
75.53/95.67/99.70. Three-decimal agreement: those panels were reporting
arithmetic on the bucket edge, not latency.
The fix was already half-written. kSubMillisecondBoundaries had been parked
in MetricsRegistry.cpp as [[maybe_unused]] with a comment noting exactly this
problem for nodestore reads. Its edges are now folded into kMicrosecondBuckets
rather than deleted, so the parked intent is carried forward: 1..1000us
resolution where the mass is, upper edges unchanged so multi-second stalls
stay measurable.
Also moves the GetObject count and charge ladders into HistogramBuckets.h, so
all five ladders have one owner and one set of invariant tests (29 now).
Adds check_bucket_parity.py, wired into the existing OTel naming workflow.
The C++ millisecond ladder and the collector's spanmetrics ladder are
specified to agree over their shared range; they were identical when shipped,
then the collector side alone was extended and nothing noticed for eleven
phases. The check asserts containment rather than equality, because jobs
outlive spans -- jobq_updatepaths averages ~60s, which no span approaches, so
demanding equality would force a ceiling that censors it. Verified it rejects
a missing collector edge, a bogus in-range edge, and a return to the 5s
ceiling.
ledger-data-sync's "Job Queue Wait p95 By Type" moves off the beast
jobq_*_q_milliseconds pair onto job_queued_us filtered by job_type. Those
beast metrics are ms-quantised at the source (Event rounds up to a whole
millisecond), so 94-100% of their samples sat in the first bucket and no
ladder change could fix them. Note the label values are camelCase
(job_type="ledgerData"), not the lowercase metric-name fragments.
Both histogram-fed alert thresholds re-validated and left unchanged, with the
measured basis recorded so neither gets tuned against the old artefact: only
0.0022% of job_queued_us samples exceed the 1s threshold, and every edge
bracketing the 1000ms ios_latency threshold survived the ladder change.
Docs: the rpc_size "known issue -- tracked separately" notes in the runbook
and 09-data-collection-reference are now resolved notes, the stale 10-edge
span_duration bucket list is corrected to the collector's real 20, and the
runbook gains a "Reading A Histogram Percentile" section covering both
saturation traps and the expected discontinuity after a ladder change.
Nine conflicts, resolved as follows.
src/xrpld/app/ledger/detail/InboundLedger.cpp -- kept this branch's version.
phase10 sets the span's outcome/timeouts/peer_count attributes inline at each
exit; this branch replaced that with the idempotent finalizeAcquireSpan(), called
on all four exits (init, done, give-up, destructor). Taking phase10's blocks
would have set the outcome twice against a helper documented as not overwriting
what the real exit recorded. phase10's comment explains why peer_count must not
be read in a destructor; the helper solves that structurally by taking
std::optional<std::size_t> and being passed std::nullopt from there.
src/xrpld/telemetry/MetricsRegistry.cpp -- kept metric::ledgerEconomy over
phase10's "ledger_economy" literal. This branch added the naming check that
requires constants for converted families, so the literal would regress it. Took
phase10's comment cleanup.
src/xrpld/telemetry/MetricsRegistry.h -- kept registerRotationStateGauge(), which
only exists here, and took phase10's removal of the stale task-number comment.
validate_telemetry.py -- combined both. phase10 replaced serial metric polling
with a concurrent fan-out on one shared deadline, because 58 metrics x 45 s of
additive timeout overran the CI budget; that is kept. Its target list filters on
SKIPPED_METRIC_GROUPS rather than the two literals it hardcoded, so the
sync_diagnostics group stays owned by assert_sync_diagnostics_metrics() instead
of being polled and reported twice. Both SYNC_DIAGNOSTICS_GROUP and
METRIC_POLL_CONCURRENCY are needed and both are kept.
check_otel_naming.py -- both sides extend the rule docstring. Took phase10's
fuller Rule E text (doc discovery, allow-dotted markers) and re-appended rules
I/J/K/L, which exist only here.
expected_metrics.json -- the two sides add disjoint sibling groups, so both are
kept: sync_diagnostics alongside node_health_gauges, overlay_reduce_relay,
overlay_overflow, validation_lifetime_counters and not_asserted. Both dashboard
uids are kept, giving 16 asserted uids against 16 dashboards on disk.
expected_spans.json -- kept this branch's span set, a superset that adds the
acquire phase spans, ledger.serve, txset.acquire and peer.dial, and expands
ledger.acquire's required attributes. Took phase10's description, which documents
what the totals mean, and its note on how the RPC wildcard span is created.
total_span_types and total_unique_attributes are recomputed for the union: 48 and
74, since each side's figure counted only its own spans.
Docs: took phase10's more accurate wording on what the dashboard check actually
covers, and corrected the dashboard count from 15 to 16 where the merge made it
stale.
Verified: no conflict markers remain, both JSON contracts parse, both Python
files compile, asserted dashboard uids match the dashboards on disk exactly, and
the OTel naming check reports all layers consistent.
check-otel-naming fails Rule D on this branch: ledger-data-sync.json
aggregates by xrpl_work_item, which nothing in-tree emits because
perf-iac's alloy pipeline stamps it, so it cannot be derived from
*SpanNames.h. The sibling perf-iac identities xrpl_branch and
xrpl_node_role are already allowlisted; this one was missed when the
by() clauses were reintroduced.
Add it to EXTERNAL_INFRA_LABELS alongside them. Verified by removing
the entry again, which restores the failure.
The integration-test sections, alert provisioning headers and a naming-check
test comment were indexed by rollout phase and task numbers defined only in
planning documents outside the shipped tree. Each now names what it covers.
The reference docs had drifted from the code in ways that break the reader
rather than merely misinform: PromQL examples that return no data, a rollback
flag that is a no-op, a sampling knob that does not exist, and two span parents
that moved. Code is treated as the truth throughout; where the code is the
defective side, the doc now records it as a known issue instead of describing
the bug as intent.
Renames the docs missed: histogram names gain the exporter's unit suffix
(ios_latency_milliseconds_bucket and four siblings), ledger_history_mismatch
gains _total, the StatsD-era quantile label gives way to le buckets,
rpc.request becomes rpc.http_request, traces_spanmetrics_calls_total becomes
span_calls_total, and the nine dotted xrpl.* span attributes are recorded as
renamed rather than left as live keys.
Re-parenting: consensus.update_positions and consensus.check are children of
consensus.establish, not of consensus.round.
Units and labels: state_accounting_*_duration is microseconds, not seconds;
cache_metrics label values are case-sensitive; object_count carries demangled
C++ type names. Nodestore read and write latency stays microseconds -- the
nanosecond accumulator change did not move the exported unit.
Adds what shipped but was undocumented: the ledger.acquire span, seven
consensus.round events, twelve span attributes, node_writes_duration_us, the
7-day validation-agreement window, the TxQ admission and reduce-relay metric
families, metrics_endpoint, and the phase-10 validation workflow.
Corrects claims that never held: 10% head sampling (it is fixed at 100%),
configurable redaction (it is unconditional), -DXRPL_ENABLE_TELEMETRY=OFF
(the flag is -Dtelemetry=OFF, default ON), FindOpenTelemetry.cmake and the
xrpl_telemetry target (neither exists), Promtail and a StatsD exporter in the
pipeline (neither exists), and Loki stream selection on job= (only
service_name is a stream label).
Phase 9 is marked complete, its provisioned alerting is attributed to the
branch that shipped it, and Phase 11 stays at zero except the one prerequisite
its code closes. Counts are reconciled repo-wide: 41 emitted span families,
15 dashboards on disk with 14 asserted, 13 alert rules in 5 groups.
Hardens the gate that let this drift through: Rule E of the naming check now
covers the reference docs, its allow-dotted marker is key-scoped and warns on
stale or empty use, a missing checked file is reported instead of silently
skipped, the test suite runs in CI, and doc paths trigger the check.
C++ and CMake changes are comment-only: three MetricsRegistry instrument names,
eight OTelCollector claims of a metric-name prefix that formatName never adds,
and the telemetry option's inverted default.
* upstream/release/3.3.x: (41 commits)
chore: Bump version to 3.3.0
chore: Bump version to 3.3.0-rc7
fix: Increase manifest protocol message size cap and fix manifests relay
fix: Cap untrusted manifests per message and drop oversized ones
chore: Bump version to 3.2.1
chore: Bump version to 3.2.1-rc1
fix: Cap untrusted manifests per message and drop oversized ones
fix: Reject oversized validator manifest before decoding
fix: Reduce untrusted manifest cache cap to 100
fix: Bound untrusted manifest cache
chore: Bump version to 3.3.0-rc6
feat: Package validator-keys inside rippled
chore: Bump version to 3.3.0-rc5
fix: Switch SponsorshipSet to use a delta for sfFeeAmount
fix: Re-revert "fix: Set request size limits and differential pricing for get-object-by-hash calls"
chore: Bump version to 3.3.0-rc4
fix: Revert "fix: Set request size limits and differential pricing for get-object-by-hash calls"
chore: Bump version to 3.3.0-rc3
fix: Reduce untrusted manifest cache cap to 100
fix: Revert "fix: Reject oversized SHAMap nodes in gotStaleData and fetch-pack path"
...
Rule D validated every dashboard label against L1 (*SpanNames.h) and L6
(MetricsRegistry) labels. LogQL labels have a third provenance neither
layer can resolve: they are minted by the collector's regex_parser named
captures (partition, severity) or by an in-query `| regexp` stage
(action, pk, state, mode, phase, jobname, ip, pubkey). Checking them
against L1/L6 reported ten violations for labels correct by
construction.
Make the rule datasource-aware instead of allowlisting a filename. The
dashboard JSON is parsed so each query can be attributed to its
datasource, and queries on a log datasource are skipped. The exemption is
per query, not per file, so a dashboard mixing Prometheus and Loki panels
still has its Prometheus panels validated.
Parsing the JSON also fixed a blind spot: label filters are stored with
backslash-escaped quotes (`label=~\"$v\"`), which the previous raw-text
regex could never match, so only the `sum by (...)` form was ever
checked. With the strings unescaped, 555 queries are now validated where
far fewer were before. That surfaced three legitimate label sources the
rule did not model, each fixed at its source rather than allowlisted:
- deployment_environment / xrpl_network_type: resource attributes the
collector promotes onto metric datapoints. Derived from the config's
resource_metrics_key_attributes, so a new key is picked up
automatically, in both dotted and underscore forms.
- resource.service.instance.id: strips to a dotted service-identity
key, which builtins only held in underscore form.
- name: the TraceQL span-name intrinsic, alongside duration and kind.
A file that does not parse falls back to the raw-text scan, which checks
every query rather than skipping it; JSON validity is already enforced by
the prettier pre-commit hook.
Adds 10 tests: the exemption, per-query scoping in a mixed dashboard,
target-inherits-panel datasource, no sideways inheritance leak, nested
row panels, TraceQL intrinsics, the malformed-JSON fallback, and the
collector-promotion helper.
These changes were developed on the phase-10 branch but belong to content this
branch and its upstreams introduced. Carrying them on phase-10 made its PR diff
report churn in files phase-10 does not own, and left each PR claiming a scope
that did not match its contents.
Moved here from phase-10 (identical content, no functional change):
- Dashboards: all 14 existing boards plus the new log-derived-insights board.
- Docs: telemetry-runbook.md (minus the workload/benchmark sections, which
describe phase-10 tooling) and the new telemetry-glossary.md.
- Grafana Cloud + Alloy export path: collector config, compose override, the
two .env examples and alloy/config.alloy.
- Local stack: otel-collector-config.yaml gains sub-millisecond and
second-scale spanmetrics buckets, pins unit=ms, and promotes
close_time_correct; integration-test.sh and TESTING.md follow.
- Node configs: exported_instance -> service_instance_id in comments; the
mainnet sample now logs at warning to bound log volume.
- Metrics code: Telemetry.cpp builds the metrics pipeline in the constructor
via initMetrics() so the global MeterProvider is published before any
subsystem creates a beast::insight instrument, and the histogram view keeps
each instrument's own name instead of collapsing them under one series.
MetricsRegistry gains a last_close_time gauge and skips negative job-queue
durations. OTelCollector drops an unused accessor.
- Naming CI: xrpl_work_item joins EXTERNAL_INFRA_LABELS and Rule E accepts the
dotted perf-iac resource-attribute form. This must travel with the
dashboards and runbook that reference those labels, or the rules fail.
- Doxygen input glob no longer recurses dot-directories.
Sections describing phase-10 tooling stay on phase-10 and keep their
"Future Enhancement" / "Planned, not yet implemented" markers here; phase-10
removes those markers when it lands the tooling.
Four findings from a review pass over the PR.
The "Spans & traces" row was empty. Moving the row header down to clear
the back-fill panels was only half the change -- the seven span-derived
panels stayed at their old y, one unit below the native panels, so every
pair overlapped and Grafana parented all fifteen to "Back-fill &
persistence". The panels now sit below the row header, which restores
the split the runbook already describes: eight native panels answer "how
much", seven span-derived ones answer "which". Both rows stay expanded,
so the docs no longer call them collapsed.
metric_constants() excises each namespaced block before the flat
prefix pass. The flat pass classifies by identifier prefix and is meant
for headers that name the role in the identifier because they have no
`namespace metric`/`label`/`lval`; it was running over the whole header,
so a `kLabel`-prefixed constant written inside `namespace metric` landed
in both buckets and an instrument name became a valid label key for Rule
D. Nothing in the tree does that today, which is why it went unnoticed,
and why the guard is a test rather than a fix for an observed failure.
The `site` label now keeps a non-default port and drops userinfo residue
from the host. Omitting the port unconditionally merged two local sites
that differ only by port; printing it unconditionally would have renamed
the existing `https://vl.ripple.com` series. Comparing against the
scheme default distinguishes a configured port from the one the Resource
constructor fills in. parseUrl's host group also permits '@', so a
malformed URI with two of them leaves part of the userinfo in `domain`.
Adversarial validation of the previous commit found one of its two code fixes
was diagnosed wrongly and the other incomplete. Both are corrected here, along
with the layers the first pass missed.
1. The new dial outcome was named for the wrong condition. It was added as
`duplicate` on the belief that PeerFinder had already granted a slot for the
address. It has not: `Logic::onConnected` contains exactly ONE false-returning
path and it is the self-connect check, which logs "Logic dropping as self
connect" (include/xrpl/peerfinder/detail/Logic.h). The duplicate check lives
in `newOutboundSlot`, evaluated before a ConnectAttempt exists, so a real
duplicate can never reach this branch.
That mattered beyond the name: the previous commit told operators the outcome
was benign churn to ignore, when it actually reports a local misconfiguration
-- this node has its own address in [ips_fixed] or behind its advertised
endpoint, and every dial to it is wasted. Renamed to `self_connection`,
reusing the slug `handshake_negotiation_fail_total` already publishes for the
same fault so it reads identically on both signals, and every description
corrected to say so. The fail() string now reads "Self connection" too.
The first pass also missed three enforcement and contract sites: the
ConnectAttempt.h Doxygen state machine (which still mapped the slot branch
onto tls_fail), the LedgerSpanNames unit test (which pinned exactly five
values over a std::array<..., 5> and so left the new member untested), and the
span-derived twin panel plus two reference docs that still published the old
five-value domain.
2. The credential-free site label was incomplete twice over.
- It appended the port, and `Resource::Resource` DEFAULTS that to 443/https
and 80/http when the config omits one. The label would have become
`https://vl.ripple.com:443/` where Grafana Cloud currently holds
`https://vl.ripple.com`, silently renaming the series for every deployment
already scraping this metric. Verified against live label values before and
after; the port is now omitted.
- parseUrl's path group is `(/.*)?`, greedy to end of string, so a query or
fragment lands inside `path`. A list URL authenticated by `?token=...` would
have leaked exactly as userinfo did. The path is now truncated at the first
'?' or '#'.
Also updated the MetricNames.h usage example, which still taught the raw-URI
pattern to the next author, and the 09-doc row that described the label as the
configured URI.
3. Rule J hardening from the same review: `classify_instrument_kind` returns an
`other` sentinel for a non-factory macro, and storing it in the kind set could
render a future conflict as "created as counter and other". The sentinel is
now skipped, keeping it doing what it already did -- matching no shape rule.
Added a second regression test whose input the pre-fix code reported as CLEAN
(gauge-then-histogram on a `_us` name), so the guard is proven by a 0-vs-1
difference and not only by a changed message. Both new tests were run against
a reconstructed last-wins implementation and both fail against it.
Documented the conflict class in the Rule J rows of the checker README and
CONTRIBUTING, which previously described only the suffix conventions.
Verified: naming checker exits 0 with Rule J passing all 40 real names; 140
checker tests pass; 15 dashboards validate; both workload JSON files parse;
clang-tidy over the full compile database reports no finding on any changed line
of ConnectAttempt.cpp or ValidatorSite.cpp; pre-commit passes.
Not verified: not compiled. The label change adds string truncation and the
outcome rename touches a constexpr used across three translation units, so CI's
build remains the first real check on both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four defects from the automated review on PR #7875, each verified against the
current tree before fixing (one further comment, the row-63 dashboard overlap,
was already fixed by an earlier commit and needed nothing).
1. Rule J could not detect an instrument-kind mismatch. instrument_kinds() wrote
`kinds[wire] = ...`, so a wire name created through two different factories
kept only the kind visited last and whichever emit site the file walk reached
last silently decided the verdict. It now collects a set per name and reports
the conflict itself -- one name exporting two instruments is the defect, and
no suffix can be correct for both. Added a regression test that builds a name
as both a counter and an observable gauge and asserts the message names both.
2. A duplicate connection was reported as `tls_fail`. The TLS handshake had in
fact succeeded; PeerFinder simply already held a slot for that address, which
is ordinary churn on a healthy node. Conflating the two made a rising
`tls_fail` unreadable -- it could mean unreachable peers or merely a busy
PeerFinder, and those need opposite responses. Added a distinct `duplicate`
outcome and carried the widened vocabulary through every place that
enumerates it: the panel description, both filter descriptions, the runbook
branch table, the runbook outcome list and the expected_spans note. The
`dial_outcome` template variable is a label_values() query, so it picks the
new value up on its own.
3. ConnectAttempt::onShutdown had no `operation_aborted` guard, unlike the five
other handlers in the same file. A clean teardown was therefore counted as
`upgrade_fail`, inflating that outcome on any node shutting down with dials in
flight.
4. ValidatorSite used the raw configured URI as a Prometheus label.
[validator_list_sites] accepts credentials in the URI and ParsedUrl keeps them
in username/password, so a configured `https://user:pass@host` would have
copied the secret into a metric label and on into the collector, Prometheus
and every dashboard. The label is now rebuilt from scheme, host, port and
path -- everything needed to tell one site apart, and nothing more.
Verified: naming checker exits 0 with Rule J still passing all 40 real
instrument names; its unit tests now number 139 and all pass; 15 dashboards
validate; both workload JSON files parse; clang-tidy over the full compile
database reports no finding on either changed .cpp; pre-commit passes.
Not verified: not compiled. Item 4 introduces string concatenation and item 2 a
new constexpr, so CI's build is the first real check on both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Brings in the phase-10 revert of the nodestore read-latency histogram plus the
nudb_bytes -> stored_object_bytes rename.
Conflicts in MetricsRegistry.{h,cpp} resolved keeping both intents:
- MetricsRegistry.cpp: dropped everything that existed only to serve the
reverted nodestore_read_us histogram -- the addSubMillisecondHistogramView()
helper, its call site, the kSubMillisecondBoundaries array and the
NodeStoreMetricNames.h include. Kept every view this branch registers
(consensus round duration, sweep_malloc_trim_us, dns_resolve_latency_ms,
overlay_dial_latency_ms) and the shared addHistogramView() base helper.
Took the rename at the storage_detail observe() call site.
- MetricsRegistry.h: took phase-10's move of the four nodestore_state observe
helpers and their ObserveFn sink from private to public, while keeping this
branch's enriched Doxygen on observeNodeStoreTotals().
Also corrected the registered-view count in the 09 reference doc: neither side's
arithmetic survives the merge, since this branch adds four views phase-10 never
saw and the revert removes one. Ten views are registered now, not six or seven.
The 22 metric label values on the nodestore_state gauge were asserted
nowhere, and the depthSum assertions could not tell the real
fetch_add(depth) from a fetch_add(1) that would silently zero every
derived queueing time.
Make the four observe* helpers and their ObserveFn sink public rather
than private, so the seam the header already claimed is actually
reachable. All four are static and read only their arguments, so this
exposes no object state; a friend declaration would have granted access
to every private member instead. The assertions live in the existing
Beast nodestore suite because src/test compiles into xrpld, which
contains MetricsRegistry.cpp, while xrpl_tests deliberately does not
when telemetry is enabled.
Each helper now has its exact emitted label set asserted, so a typo in
any literal fails instead of silently producing a disjoint series, and
each derived mean is asserted ABSENT on a fresh store -- a refactor to
value_or(0) would draw a believable flat zero on a latency axis and
otherwise pass everything.
Replace the concurrent write-stats test with one that forces genuine
overlap through a latch. NuDB holds one global mutex for the whole
insert and doInsert reads the depth before entering it, so a blocked
thread records a depth of at least 2; asserting depthSum strictly
exceeds insertCount therefore cannot be satisfied by a constant 1. The
old bounds admitted that bug at their floor.
Also: cover the std::nullopt branch on the two backends that exist in
every build, bound the store-duration accumulator by the wall clock,
drop four assertions that cannot fail, and correct two comments that
claimed coverage the tests do not have -- the duplicate-key test is not
the throwing path, because nudb reports key_exists without throwing,
and no test drives NodeStoreScheduler::onFetch.
