The Peer Disconnect Rate By Reason panel anchored its LogQL capture on
"\] ", which only matches a reason logged immediately after the [NNN]
peer-id prefix. PeerImp does not log that way: PeerImp::fail emits
"[NNN] <name> failed: <reason>" and the clean teardown emits
"close: Closed". Only ConnectAttempt::fail, which logs the bare reason,
ever matched. The panel's Timeout series was therefore connect-attempt
timeouts only, Ping Timeout was invisible, and PeerImp's own Closed was
uncounted.
Match all three prefixes and separate Ping Timeout from Connect Timeout.
Recorded as LogQL trap 11 in the runbook, alongside the other silent
failures this dashboard exposed.
Also document six overlay observability gaps found while auditing what
ping/pong and gossip traffic is actually tracked. All are pre-existing
and none is fixed here: the code fixes belong in develop-owned overlay
files (TrafficCount, OverlayImpl, PeerImp, PeerfinderManager), not on a
telemetry branch, and one of them needs a public signature change.
- 09 section 6: six known issues, each marked NOT IMPLEMENTED with
file:line evidence -- mtCLUSTER counted as unknown (overhead_cluster_*
always zero, 8 panels flatline), squelch_ignored byte counts always
zero, inbound/outbound byte-basis asymmetry plus a stale Total header
comment, ping/endpoints instrumentation absent, peer span coverage,
and PeerFinder exporting 2 of ~17 available readings.
- 02 section 2.3.2: add a Status column to the span catalog. Of 36
catalogued spans, 16 are live, 15 were never built, and 5 shipped
under different names (consensus.phase.establish became
consensus.establish, ledger.close became consensus.ledger_close,
rpc.request split into rpc.http_request/rpc.ws_message, txq.apply
became txq.apply_direct/txq.accept_tx). The catalog is a design
inventory; 09 section 1.1 remains authoritative for what emits.
- Phase9_taskList: tasks 9.14-9.17 tracking the deferred work, with
exit criteria checked only for what is actually done.
- Glossary: new Ping / pong keepalive term distinguishing ping timeout
from connect timeout. Correct the Cluster and Squelch entries, which
described behaviour the metrics cannot show.
The glossary header pointed at tasks/telemetry_terms.py as its
generator. That file is in no branch and nowhere on disk -- tasks/ is
gitignored one directory up -- so the header now states the file is
hand-maintained and gives the entry format.
Gates: check_otel_naming.py passes all 9 rules (Rule D over 555
dashboard queries, Rule E over the runbook); 19 doc anchors verified;
dashboard JSON valid with a one-line diff. No C++ changes.
Grafana 13 enables the renderAuthJWT feature toggle by default, so the image
renderer silently drops render requests carrying the default token and alert
notifications arrive with no panel image. The runbook's alerting section
covered delivery but not this failure mode.
Adds how the token is wired (one variable feeding both services, so they cannot
drift), how to override it, and the two-command check for when images stop
appearing.
Fixes found while validating the Log-Derived Insights dashboard against live
Loki and Tempo. Every change was verified by executing the query, not by
inspection.
loki.yaml — the ConsensusLedgerHash derived field never matched anything:
- The TraceQL query named `.xrpl.consensus.ledger_id`. The attribute is
`consensus_ledger_id` (ConsensusSpanNames.h); live Tempo lists no dotted or
xrpl-prefixed variant among its span attributes.
- The bare `.` intrinsic scope cannot match a span attribute regardless of
name; TraceQL requires `span.`.
Corrected to `{span.consensus_ledger_id="..."}`, which returns the expected
consensus.round trace for a real ledger hash.
log-derived-insights.json:
- Stat units `suffix: <noun>` rendered raw integers, so large values printed
in full. `si:<noun>` is unsafe because Grafana parses the leading letter as
an SI prefix (`si:fee` rendered "29.0 Mee"). Switched to `short`, the
convention already used by 42 panels; the noun is in each panel title.
- Stat panels showed a single fleet-wide tile labelled "Value #A". Grafana's
Loki backend does not name binary-operation frames from legendFormat, so the
ratio panel could not resolve per-node labels. Replaced it with a plain sum
of duplicate fetches; the ratio remains available as a timeseries. Added an
explicit legendFormat to all eight stat panels.
- The ledger-fetch regex required `good:N dupe:N` together, but
SHAMapAddNode::get() omits any zero counter. That dropped 32% of lines --
disproportionately the efficient fetches -- biasing the ratio upward. Each
field now matches independently.
- state-timeline used `spanNulls: true`, which spans gaps indefinitely and
would render an outage as continuous. Set to the 30-minute threshold.
- Stat panels relaid two-per-row and given a tooltip, per dashboard guidelines.
telemetry-runbook.md:
- LogQL examples used line filters where structured metadata exists.
`|= "ERR"` also matches the literal in a message body: measured 423 ERR
lines plus 4 DBG lines per 6h. Replaced with severity, partition, and
trace_id field filters.
- Documented the new dashboard, its debug-log requirement, and the LogQL traps
these fixes exposed.
- Corrected the dashboard count, which was already stale at eleven against
fifteen on disk, and named the six with no reference section.
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.
Update the catalogue table and prose for the two thresholds corrected by the
7-day backtest, and document why ValidatedLedgerStale must exclude the
1209600s sentinel that getValidatedLedgerAge() returns when no validated
ledger exists — that clause looks redundant and would otherwise be removed
as a simplification.
Also record the tuning lesson: justify thresholds from a 7-day sample, not
24 hours.
Correct the dashboard count (11 -> 15), note which boards lack a reference
section, and update the log-derived-insights panel/row counts. Replace line
matches with structured-metadata filters in the LogQL examples and document
the PeerFinder debug requirement plus which [MIXED] panels work at the
default log level.
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
The two key-job latency panels on node-health declared milliseconds
while querying `job_running_us` / `job_queued_us`, which record
microseconds. Every reading was a thousand times too large: the p95 for
acceptLedger, 241495us, rendered as "241 sec" instead of 241ms.
job-queue.json already read the same metrics as microseconds, so the
two dashboards disagreed by 1000x on identical data.
Also replace the generic `ops`, `cps` and `cpm` units, which Grafana
renders as the literal "ops/s", "counts/s" and "counts/min", with
custom-suffix units naming what each panel counts -- jobs, ledgers,
validations, lookups. The per-minute panels already scale correctly in
their queries; only the noun was missing.
Normalise the micro sign to U+00B5 throughout. Four axis labels used
the visually identical Greek mu, U+03BC, while every unit field used
the micro sign.
Ledger Close Rate plots ledgers closed beside ledger fetches, which one
unit cannot describe; give the fetches series its own unit on a
right-hand axis. Drop two field overrides on NodeStore Read Latency
that restated the panel unit as a custom suffix -- a suffix is appended
verbatim, so it would have suppressed magnitude scaling and left large
values unreadable, the same defect fixed above.
Queries are unchanged apart from the transaction path piechart, which
moves from rate() to increase(): a per-slice "per second" reading is
not a share of a total.
Alongside, widen the Complete Ledger Ranges table to full width, hoist
the stat panels above the fold, and bring the touched panels up to the
tooltip and null-spanning guidelines.
The four job-latency panels on node-health declared milliseconds while
querying `job_running_us` / `job_queued_us`, which record microseconds
(MetricsRegistry records the raw value, and the instrument description
says microseconds). Every reading was therefore a thousand times too
large: the p95 for acceptLedger, 241495us, rendered as "241 sec"
instead of 241ms. job-queue.json already read these same metrics as
microseconds, so the two dashboards disagreed by 1000x on identical
data. Switch node-health to microseconds to match.
Also replace the generic `ops` and `cps` units, which Grafana renders
as the literal "ops/s" and "counts/s", with custom-suffix units naming
what each panel counts -- messages, fetches, calls, mismatches.
Two panels plot more than one quantity on a single axis, which no
single unit can describe. Give each series its own unit through field
overrides: reads per second beside two queue depths on NuDB Read
Pressure, and ledgers beside fetches on Ledger Close Rate, the latter
on a right-hand axis.
State Duration Rate plots a seconds-per-second time share, which can
exceed 1.0 and so is not a percentage; label it as the ratio it is.
The normalised share already exists as its own panel.
Queries are unchanged; the values were already correct.
Alongside, bring the touched panels up to the dashboard guidelines and
hoist the stat panels above the fold.
Grafana renders `unit: "ops"` as the literal string "ops/s", so every
rate panel read as "operations per second" regardless of what it
actually counted. `Ledger Build Rate` showed "0.258 ops/s" where the
value is one ledger every 3.9s -- the number was right, the unit was
meaningless.
Replace the generic units with Grafana custom-suffix units naming the
quantity, following the existing `suffix:/hr` and `si:drops` precedent
in this repo. Nine of these are `stat` panels with no axis, so the unit
string was the only text a reader ever saw.
Also switch the two trusted/untrusted piecharts and the transaction
path piechart from rate() to increase(): a per-slice "per second"
reading is not a share of a total, counts in the window are.
Queries are unchanged apart from those three; the values were already
correct.
Alongside, bring the touched panels up to the dashboard guidelines:
tooltip mode/sort/max-height, 30-minute null spanning, and axis labels
in title case. Hoist the stat panels above the fold on
ledger-operations and rpc-performance.
Panels that a later branch in this chain removes are deliberately left
alone -- fixing them would only add merge conflicts.
Three measurement fixes landed with no doc or dashboard change, leaving text
that is now false and one fix unusable from a dashboard.
Deferrals and timeouts are recorded in TimeoutCounter, a base shared by five
subclasses, so the all-lane pair could show the documented livelock
fingerprint while ledger acquisition was healthy. The runbook procedure and
the reference doc now name acquire_ledger_deferrals and
acquire_ledger_timeouts and say why the all-lane pair misleads; a new panel
plots the ledger-scoped pair as rates on one axis, since the divergence is
the signal. The existing panel is retitled All Lanes and points at it.
Writer mean depth is depthSum over depthSamples, not over insertCount, and
the measured 1.60 came from the biased estimator, so it and the 37% queueing
share derived from it are lower bounds rather than values. The reference
table now marks them as such, and the decision rule is shown to survive the
correction rather than depending on the exact figures.
Completions were never counted for acquisitions satisfied from the local
store, so the run that read zero across 510 seconds had in fact reached
full. Every place that treated a zero as a symptom now says it only means
something on a build that has the fix.
Also corrects the sync-diagnosis label-value count from 13 to 15 and a stale
source line range; the instrument count stays 35, because both new values
multiplex onto the existing nodestore_state gauge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Drops nodestore_read_us and everything added to reach it. read_mean_us already
carries microsecond precision and separated the two sync failure modes cleanly
in live testing -- 8.8 us on a clean store against a 223 us cold-store peak --
so the distribution added no signal that changed a diagnosis.
The cost of getting it was disproportionate. NodeStoreScheduler had no path to
the metrics registry, so its production constructor grew a ServiceRegistry
parameter: a metric addition changing a production signature. That in turn
forced an edit to a pre-existing test, src/test/app/SHAMapStore_test.cpp, whose
only stake in this is that it constructs a scheduler. Worse, the scheduler is
built in Application's member initializer list, long before metricsRegistry_
exists, so the registry could not be captured once and had to be re-resolved on
every fetch -- a lookup on a path that runs millions of times per sync.
The constructor returns to taking JobQueue& alone and SHAMapStore_test.cpp
returns to the single-argument call, leaving that file differing from its
pre-change form only by the NodeStore:: to node_store:: rename it picked up from
develop.
FetchReport::elapsed stays microseconds and onFetch keeps its explicit
duration_cast to milliseconds for addLoadEvents, which takes milliseconds. That
widening was a separate fix and is what makes read latency measurable at all.
kSubMillisecondBoundaries loses its only consumer and regains [[maybe_unused]],
which is the state the commit that introduced it left it in; without the
attribute an unused constant is an error under wextra with werr.
Also removes the ledger-data-sync panel that charted the histogram and the
fetch_type and found template variables, which filtered on labels no metric
emits any more, plus the runbook and reference-doc sections and the two
instrument and view counts that named it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The nudb_bytes label value on the storage_detail gauge named something the
code never measured. It observes Database::getStoreSize(), which returns the
storeSz_ accumulator: the cumulative payload bytes of objects this process
handed to the NodeStore. That is not a NuDB file size. It excludes NuDB's
keys, bucket padding and log, and it resets with the process while the files
on disk do not.
The name caused two concrete errors. It invited sizing the store on disk from
a number that cannot do it, and it invited a write-amplification ratio against
node_written_bytes -- which reads the same accessor at MetricsRegistry.cpp:836,
so that ratio is a constant 1.0 and measures nothing.
The nudb_ prefix was wrong too. storeSz_ is written only by
Database::storeStats(), called from DatabaseNodeImp, DatabaseRotatingImp and
Database itself. No backend code touches it, so the value reads the same on
RocksDB. That distinguishes it from the real nudb_* family
(nudb_writers_in_flight and friends), which come from getWriteStats() and are
absent entirely on a non-NuDB backend.
stored_object_bytes says what the value is and claims nothing about the
filesystem. Docs already described the value correctly; they keep that
explanation and now also record the old name, so a query pinned to it can be
traced. Neither Backend nor Database exposes an on-disk size accessor and none
was added -- no metric reports the store's on-disk size today.
Updates the node-health panel title, description and PromQL, and the four docs
that name the label value.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eleven local phase-10 commits, mostly documentation and dashboard-description
work, plus one real include fix. No conflicts (git merge-tree reported 0).
Reviewed for interaction with this branch:
- b50aa17aee adds <xrpl/consensus/ConsensusSpanNames.h> to RCLConsensus.cpp and
PeerImp.cpp. This is the same misc-include-cleaner finding my local clang-tidy
run reported on those two files, so the merge resolves two of the 74
pre-existing findings rather than adding any.
- 8ade2b43d9 / 7da5ac5992 / d18f713f91 rename the node-health found-ratio and
stored-bytes panels and correct the nudb_bytes and NuDB found-ratio
descriptions, then repoint the runbook at the new names. These are the same
correction phase-10 made earlier to node_reads_hit -- 'found an object', not a
cache hit -- carried through to the panels that render it.
- ledger-sync-health.json is untouched by phase-10, so none of this branch's
panel work is affected.
Verified after merging: naming checker exits 0, its 138 unit tests pass, all 15
dashboards validate, levelization produces no diff, and this branch's own work is
intact (4 touch() call sites, the ledgers_behind guard, 54 panels / 18 bargauges).
The panel was renamed from NuDB Cache Hit Ratio to NuDB Read Found Ratio,
which left the runbook naming a title that no longer exists and carrying a
paragraph saying the rename had not happened yet. Both are corrected, and
the explanation now says why the ratio is a found rate: the counter
increments whenever a fetch returned an object, and a node with
online_delete has no object cache at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Docs-only from phase-10: two commits refining the sync-diagnosis decision rule
and nodestore_state metric semantics.
Reviewed for conflicts with this branch's work and found none. Two points worth
noting because they touch documentation this branch also edits:
- node_reads_hit is re-described from "reads served from cache" to "fetches that
found an object (not a cache hit)". That is the accurate reading of the
counter and does not change any query here.
- The cold-read decision rule is retuned (insert mean threshold ~1.2, and the
read-mean/found-rate split for telling cold-but-held from real misses). It
does not contradict the retirement of nodestore_latency on this branch, which
removed a duplicate gauge rather than any nodestore_state series.
git merge-tree reported 0 conflicts before merging.
Two conflicts, both additive-vs-additive; each resolution keeps both sides.
check_otel_naming.py -- phase-10 taught the L6 label extractor to match the
label MAP first and to resolve a key hoisted into a `k...Label` constant,
scanning headers as well as sources. Our side had added the two-regex
first/subsequent literal scan and the `metric_constants(root)[1]` union that
covers the `namespace label` header style.
Kept phase-10's mechanism whole: METRIC_LABEL_MAP + the `(?:^|\{)` key regex
already subsumes what METRIC_LABEL_NEXT did, since matching inside the map body
makes every pair after the first open with a single `{`. So METRIC_LABEL_NEXT is
dropped as genuinely redundant rather than kept as a duplicate scan, and the
reason it existed is folded into METRIC_LABEL's comment. Re-added our
`metric_constants(root)[1]` union on top: LABEL_CONST_DEF only matches
`k`-prefixed identifiers, so it cannot see MetricNames.h's `label::jobType`
style, and without that union Rule D would reject dashboards querying labels
Rule I forced into constants. The two derivations are complementary and both
are now documented as such.
MetricsRegistry.cpp -- both sides added a new sibling view-registration helper
next to addMicrosecondHistogramView, and both added a registration call in
initExporterAndProvider(). Kept all four helpers
(addHistogramView/Microsecond/RoundDuration/SubMillisecond) and every
registration: phase-10's addSubMillisecondHistogramView + kNodeStoreReadUs
alongside our addRoundDurationHistogramView, sweepMallocTrimUs and the two
millisecond dial/resolve ladders.
phase-10's nodestore_read_us histogram does not duplicate our work. The
nodestore_latency gauge that would have overlapped it was retired in c4e434d520
before this merge, and the surviving nodestore_state gauge is complementary
rather than duplicative: both read the same fetch measurement, but the gauge
publishes only a since-boot mean via scaledMean() and cannot yield a
percentile -- the consequence observeNodeStoreTotals' own docs state plainly --
while the histogram buckets each fetch and can. The histogram also splits by
fetch_type and found, which the gauge cannot. phase-10 registered its
explicit-bucket View, so it does not inherit the SDK default ladder.
Each file keeps its own existing naming style: phase-10's k-prefixed constants
are left as-is, ours stay namespaced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The decision rule for "slow to reach full" keyed on an absolute read-time
threshold and a found-rate threshold that misclassified the very run they
were written to explain: a populated-store run reading 31.8 us at 88.3%
found fell through both cuts and came out as "disk-bound" rather than the
cold-read case it is.
Replace it with two ordered questions -- is the read cost several times a
warm read, and is the write path queueing -- and demote the found rate to a
splitter that only applies once reads are known to be expensive. A high
found rate on its own is the normal state of a populated store, so it can
never be a trigger. The rule now classifies all four reference datasets
correctly, and the runbook shows the rule applied to each so the "confirm
against the reference points" step agrees with the table.
Also in the runbook:
- name the source of the devnet incident figures at the point of use, and
point forward to the caveat from the same paragraph
- state the provenance of the measured columns, and split the incident
figures into their own table marked as not our measurement
- say plainly that the compounding-factor explanation is an unconfirmed
hypothesis
- correct the deferral gate: it fires at the acquisition's own job limit of
5, not at the ledgerData lane cap of 3
- note that no read-max gauge exists, so the tie-break uses max_over_time
of the mean or the read histogram's p99
- split a PromQL block that put two expressions on adjacent lines, which
parses as one invalid expression
In the data-collection reference:
- node_reads_hit counts fetches that found an object, not cache hits
- nudb_bytes is cumulative payload bytes from the same accessor as
node_written_bytes, not on-disk size, so their ratio is a constant 1.0
- write_load and nudb_writers_in_flight are the same atomic on NuDB;
document that and what write_load means on RocksDB
- correct the instrument count to 8 and the view count to 7 after
nodestore_read_us and its view were added
- scope the Phase 9 query examples to one node with the regex form
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
