Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics

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>
This commit is contained in:
Pratik Mankawde
2026-07-28 14:09:05 +01:00
15 changed files with 1845 additions and 50 deletions

View File

@@ -0,0 +1,199 @@
#pragma once
// cspell:ignore ISTOGRAM
// The all-caps macro name XRPL_METRIC_HISTOGRAM_RECORD trips cspell's
// compound-word splitter, which emits the subword "ISTOGRAM"; ignore it here.
/**
* Metric name, description, label key and label values for the per-fetch
* NodeStore read-latency histogram.
*
* The name is referenced from two translation units in two different
* levelization modules, which is why these constants live in a header rather
* than in either unit's unnamed namespace:
*
* NodeStoreMetricNames.h
* |
* +--> NodeStoreScheduler.cpp (xrpld.app -- the record site, via
* | XRPL_METRIC_HISTOGRAM_RECORD_LABELED)
* |
* +--> MetricsRegistry.cpp (xrpld.telemetry -- registers the
* explicit sub-millisecond bucket
* boundaries for the same name)
*
* A copy-pasted literal would let the two drift, and a drifted name silently
* drops the bucket override: the histogram would fall back to the SDK default
* boundaries, whose lowest edge is far above the single-digit-microsecond
* range a warm read occupies, so every warm read would land in bucket 0 and
* the distribution would read as flat. This mirrors the reason
* GetObjectMetricNames.h exists for the `getobject_*` family.
*
* Placed under `include/xrpl/telemetry/` for the same levelization reason as
* GetObjectMetricNames.h: `xrpld.app > xrpl.telemetry` and
* `xrpld.telemetry > xrpl.telemetry` are both existing edges (see
* `.github/scripts/levelization/results/ordering.txt`), so `include/xrpl/` is
* the one level both consumers can already reach. Both sites include this file
* as `<xrpl/telemetry/NodeStoreMetricNames.h>`.
*
* Example usage -- registering the bucket view (MetricsRegistry.cpp):
* @code
* addHistogramView(
* *views,
* kNodeStoreReadUs,
* {kSubMillisecondBoundaries.begin(), kSubMillisecondBoundaries.end()});
* @endcode
*
* Example usage -- edge case: the record site labels one instrument with two
* independent dimensions, which is why the keys and all four values are
* constants rather than literals (a misspelling on either side would create a
* second, silently disjoint series):
* @code
* XRPL_METRIC_HISTOGRAM_RECORD_LABELED(
* app, kNodeStoreReadUs, kNodeStoreReadUsDesc, elapsed.count(),
* {{kFetchTypeLabel, std::string(kFetchTypeAsync)},
* {kFetchFoundLabel, std::string(kFetchFoundTrue)}});
* @endcode
*
* @note These are `constexpr char[]`, not `constexpr std::string_view`. The
* OTel C++ API takes `nostd::string_view`, which on this build is OTel's own
* type; it converts from `char const*` and from `std::string` but has no
* converting constructor from `std::string_view`, so a `string_view` constant
* would not compile at the call sites. Same reasoning as
* GetObjectMetricNames.h.
*
* @note Header-only constants with no runtime state, so there is nothing to
* synchronize -- safe to include from any thread context.
*/
namespace xrpl::telemetry {
// ===== Metric name and description ==========================================
/**
* Per-fetch NodeStore backend read latency, in microseconds.
*
* Referenced twice: at the record site in NodeStoreScheduler.cpp, and by the
* sub-millisecond `addHistogramView()` call in MetricsRegistry.cpp. Both must
* use this one constant.
*/
inline constexpr char kNodeStoreReadUs[] = "nodestore_read_us";
/**
* Description for kNodeStoreReadUs.
*/
inline constexpr char kNodeStoreReadUsDesc[] = "NodeStore backend fetch latency in microseconds";
// ===== Label keys ===========================================================
/**
* Label key separating an async (read-ahead) fetch from a synchronous one.
*
* The two mean different things: a slow async read delays prefetch, while a
* slow synchronous read blocks a caller outright. Merged into one series they
* cannot be told apart.
*/
inline constexpr char kFetchTypeLabel[] = "fetch_type";
/**
* Label key recording whether the fetch found the object.
*
* A miss and a hit have different cost profiles -- a miss can require reading
* every backend -- so mixing them would blur the distribution that matters.
*/
inline constexpr char kFetchFoundLabel[] = "found";
// ===== Label values =========================================================
/** @{ */
/**
* kFetchTypeLabel values, one per node_store::FetchType enumerator.
*/
inline constexpr char kFetchTypeAsync[] = "async";
inline constexpr char kFetchTypeSync[] = "sync";
/** @} */
/** @{ */
/**
* kFetchFoundLabel values. Spelled out rather than emitted as a bool
* AttributeValue so the exported label text is stable and matches the
* string-valued convention every other label in this codebase follows.
*/
inline constexpr char kFetchFoundTrue[] = "true";
inline constexpr char kFetchFoundFalse[] = "false";
/** @} */
// ===== Record-site helpers ==================================================
/**
* Map a fetch's async-ness to its kFetchTypeLabel value.
*
* Takes a bool rather than a node_store::FetchType because this header sits in
* `xrpl.telemetry`, which has no levelization edge to `xrpl.nodestore`. The
* caller does the one-line enum comparison; this function owns the mapping so
* the two label spellings live in exactly one place and are unit-testable.
*
* @param isAsync True for node_store::FetchType::Async.
* @return kFetchTypeAsync when @p isAsync, else kFetchTypeSync.
*
* @note Pure and reentrant: holds no state and performs no I/O.
*
* Example:
* @code
* fetchTypeLabelValue(true); // "async"
* fetchTypeLabelValue(false); // "sync"
* @endcode
*/
[[nodiscard]] constexpr char const*
fetchTypeLabelValue(bool isAsync) noexcept
{
return isAsync ? kFetchTypeAsync : kFetchTypeSync;
}
/**
* Map a fetch's hit/miss outcome to its kFetchFoundLabel value.
*
* @param wasFound node_store::FetchReport::wasFound.
* @return kFetchFoundTrue when @p wasFound, else kFetchFoundFalse.
*
* @note Pure and reentrant: holds no state and performs no I/O.
*
* Example:
* @code
* fetchFoundLabelValue(true); // "true"
* fetchFoundLabelValue(false); // "false"
* @endcode
*/
[[nodiscard]] constexpr char const*
fetchFoundLabelValue(bool wasFound) noexcept
{
return wasFound ? kFetchFoundTrue : kFetchFoundFalse;
}
/**
* Whether an elapsed microsecond count may be handed to the histogram.
*
* The OTel SDK rejects a negative histogram value and logs a warning on every
* such call, so a clock anomaly on a per-fetch path would turn into a log
* flood. Filtering here drops the bad sample instead.
*
* Zero is recordable: a fetch served from a warm page cache can genuinely
* round to 0 us, and suppressing that would make the fastest reads invisible.
*
* @param elapsedUs Measured fetch duration in microseconds.
* @return True when @p elapsedUs is zero or positive.
*
* @note Pure and reentrant: holds no state and performs no I/O.
*
* Example:
* @code
* shouldRecordFetchLatency(0); // true -- genuinely instant read
* shouldRecordFetchLatency(-1); // false -- clock anomaly, skip
* @endcode
*/
[[nodiscard]] constexpr bool
shouldRecordFetchLatency(long long elapsedUs) noexcept
{
return elapsedUs >= 0;
}
} // namespace xrpl::telemetry