fix(telemetry): correct five signals that would have misled an operator

Found by reviewing what each metric actually measures, with attention to the
derived and bucketed ones. All five could report healthy while the node was
not, or the reverse.

- The nodestore latency panel took rate() of a mean. The gauge already
  divides duration by count in code, so rating it produced a figure with no
  unit, and Prometheus discards a gauge's decreases, so a heavy back-fill
  read as roughly zero microseconds per operation. The cumulative duration
  totals are now exported alongside the means, and the panel divides the
  rate of the total by the rate of the count, which is the latency over the
  panel's own window rather than a since-boot average that flattens with
  uptime.
- The DNS-resolve and outbound-dial histograms had no explicit buckets, so
  they inherited a ladder that stops at ten seconds while the dial timer is
  fifteen. Every timed-out dial fell in the overflow bucket and p95 read
  exactly ten seconds however bad it got. Both now have a ladder reaching
  thirty seconds with fifteen on its own boundary, so a timeout is
  distinguishable from merely slow.
- The missing-node counts only cleared when a tree completed, so a
  timed-out or failed acquire left its last count latched. Since the gauge
  reports the maximum across everything still in the collection, and
  eviction waits on a grace period plus the sweep interval, a finished node
  reported as stuck for minutes. That inverts the one signal that separates
  stuck from slow. Cleared unconditionally on the terminal path instead.
- A disabled quorum published a sentinel so large that, on a timeseries
  axis shared with the trusted-key count, it flattened the key line to the
  baseline and hid the outage it was meant to mark. The series is now
  omitted and a quorum_disabled flag carries the state.
- Two panel descriptions claimed a one-second export cycle. The reader is
  configured for ten.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Pratik Mankawde
2026-07-27 14:27:28 +01:00
parent c9b90ec527
commit 371f10934e
9 changed files with 93 additions and 22 deletions

View File

@@ -225,6 +225,16 @@ private:
void
refreshMissingNodeCounts() noexcept;
/**
* Zero the missing-node counts because this acquire has finished.
*
* Unconditional, so it also clears after a timeout or failure, where the
* have-tree flags are never set and the flag-guarded refresh would leave a
* stale non-zero count visible to the gauge.
*/
void
clearMissingNodeCounts() noexcept;
/**
* Fold one processed batch into the acquire totals and emit its telemetry.
*

View File

@@ -224,6 +224,20 @@ InboundLedger::refreshMissingNodeCounts() noexcept
missingTxNodes_.store(0, std::memory_order_relaxed);
}
void
InboundLedger::clearMissingNodeCounts() noexcept
{
// Unconditional, unlike refreshMissingNodeCounts(): this runs when the
// acquire is over, however it ended. A timed-out or failed acquire never
// sets the have-tree flags, so the flag-guarded refresh above would leave
// its last sweep count latched. The gauge maxes over every acquire still in
// the collection, and eviction waits on a one-minute grace plus the sweep
// interval, so a latched count would report a finished node as stuck for
// minutes -- inverting the one signal that separates stuck from slow.
missingStateNodes_.store(0, std::memory_order_relaxed);
missingTxNodes_.store(0, std::memory_order_relaxed);
}
std::size_t
InboundLedger::getPeerCount() const
{
@@ -741,6 +755,10 @@ InboundLedger::done()
signaled_ = true;
touch();
// The acquire is over on every path through here, so the missing-node
// counts must stop being reported. See clearMissingNodeCounts().
clearMissingNodeCounts();
// Keep the span active as the ambient context across the outcome log so
// that line carries the span's trace_id. The activation is non-owning;
// acquireSpan_ still owns the span. It pops at the end of this block, while

View File

@@ -475,6 +475,11 @@ inline constexpr char parseError[] = "parse_error";
namespace unl_quorum {
inline constexpr char trustedKeys[] = "trusted_keys";
inline constexpr char quorum[] = "quorum";
// 1 while the validator list has disabled quorum, 0 otherwise. Carries the
// state that used to be encoded by publishing a sentinel value on `quorum`
// itself: a number that large plotted on a shared axis flattens the
// trusted-key line to the baseline, hiding the very failure it marked.
inline constexpr char quorumDisabled[] = "quorum_disabled";
} // namespace unl_quorum
/**
@@ -595,6 +600,13 @@ inline constexpr char writeCount[] = "write_count";
inline constexpr char readCount[] = "read_count";
inline constexpr char writeMeanUs[] = "write_mean_us";
inline constexpr char readMeanUs[] = "read_mean_us";
// Cumulative microsecond totals. The means above are convenient to read at a
// glance but cannot be rated: they are already a ratio, and a gauge of a ratio
// has no meaningful derivative. Dividing the rate of these totals by the rate of
// the matching count yields the latency over the panel's own window, which is
// what a dashboard actually wants.
inline constexpr char writeDurationUs[] = "write_duration_us";
inline constexpr char readDurationUs[] = "read_duration_us";
} // namespace nodestore_latency
/**

View File

@@ -328,6 +328,22 @@ MetricsRegistry::initExporterAndProvider(std::string const& endpoint, std::strin
// comes from the shared constant both sites use.
addMicrosecondHistogramView(*views, kGetObjectLookupUs);
// Millisecond dial/resolve latencies. Both exceed the SDK default ceiling
// of 10,000: the dial timer is 15 s, so without an explicit ladder every
// timed-out dial lands in the overflow bucket and p95 reads exactly 10 s
// however bad it gets. The 15 s boundary sits on its own so a timeout is
// distinguishable from merely slow.
addHistogramView(
*views,
metric::dnsResolveLatencyMs,
{1.0, 5.0, 10.0, 25.0, 50.0, 100.0, 250.0, 500.0, 1'000.0, 2'500.0, 5'000.0, 10'000.0,
15'000.0, 20'000.0, 30'000.0});
addHistogramView(
*views,
metric::overlayDialLatencyMs,
{1.0, 5.0, 10.0, 25.0, 50.0, 100.0, 250.0, 500.0, 1'000.0, 2'500.0, 5'000.0, 10'000.0,
15'000.0, 20'000.0, 30'000.0});
// The remaining two GetObject histograms are not durations, so the
// microsecond ladder above does not fit them. Both still need explicit
// boundaries: the SDK default stops at 10,000 and both ranges exceed it.
@@ -1662,11 +1678,16 @@ MetricsRegistry::registerUnlQuorumGauge()
// instead: headroom then goes strongly negative, which is the
// truthful signal.
auto const quorum = validators.quorum();
observe(
lval::unl_quorum::quorum,
quorum == std::numeric_limits<std::size_t>::max()
? std::numeric_limits<int64_t>::max()
: static_cast<int64_t>(quorum));
// A disabled quorum omits the series rather than publishing a
// sentinel. Both consumers of this gauge are timeseries panels
// sharing one axis with trusted_keys, so a huge value would
// flatten the key line to the baseline and hide the outage it
// was meant to signal. The boolean below carries the state, and
// a missing quorum line is itself the visible anomaly.
bool const quorumDisabled = quorum == std::numeric_limits<std::size_t>::max();
if (!quorumDisabled)
observe(lval::unl_quorum::quorum, static_cast<int64_t>(quorum));
observe(lval::unl_quorum::quorumDisabled, quorumDisabled ? 1 : 0);
}
catch (...) // NOLINT(bugprone-empty-catch)
{
@@ -2149,15 +2170,22 @@ MetricsRegistry::registerNodeStoreLatencyGauge()
// is missing: a reported 0 us would claim writes are
// instantaneous, which is worse than no reading at all.
//
// The numerator guard is load-bearing, not defensive.
// Database::store() is pure virtual, so only the store paths
// that call recordStoreDuration() contribute. Today that is
// Database::importInternal (the [import_db] path). The two
// concrete runtime databases -- DatabaseNodeImp::store and
// DatabaseRotatingImp::store -- do not call it yet, so on an
// ordinary node write_count climbs while the duration total
// stays 0. Omitting the mean makes that a visible data gap
// instead of a false "writes take 0 us" line on the panel.
// The numerator guard covers the pre-first-write window only.
// Both concrete databases time their backend write, so the
// total advances on any ordinary node; before the first write
// it is still 0, and omitting the mean then is better than
// publishing a false "writes take 0 us".
// The cumulative totals are observed unconditionally, so a
// panel can divide rate(duration) by rate(count) and read the
// latency over its own window rather than a since-boot average
// that flattens as uptime grows.
observe(
lval::nodestore_latency::writeDurationUs,
static_cast<int64_t>(storeDurationUs));
observe(
lval::nodestore_latency::readDurationUs,
static_cast<int64_t>(fetchDurationUs));
if (storeCount > 0 && storeDurationUs > 0)
{
observe(