ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-22 14:50:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(telemetry): correct the dial-outcome diagnosis and harden the site label

Browse Source 
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>
This commit is contained in:
Pratik Mankawde
2026-07-28 17:40:52 +01:00
parent f649670ef7
commit b9497d05da
14 changed files with 181 additions and 123 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
src/tests/libxrpl/telemetry/LedgerSpanNames.cpp
View File

@@ -584,7 +584,7 @@ TEST(LedgerSpanNames, peer_dial_attribute_keys_are_bare_underscore)
TEST(LedgerSpanNames, peer_dial_outcome_values_match_the_counter_label_set)
{
// These five ARE the values ConnectAttempt::reportOutcome passes to the
// These six ARE the values ConnectAttempt::reportOutcome passes to the
// overlay_connect_total counter -- the span and the counter read the same
// constants from the same funnel, which is what stops them drifting apart.
// Pinned literally because the Bootstrap-row dial panel and the runbook
@@ -594,6 +594,10 @@ TEST(LedgerSpanNames, peer_dial_outcome_values_match_the_counter_label_set)
EXPECT_EQ(std::string_view(peer_span::val::tlsFail), "tls_fail");
EXPECT_EQ(std::string_view(peer_span::val::upgradeFail), "upgrade_fail");
EXPECT_EQ(std::string_view(peer_span::val::timeout), "timeout");
// Reuses the slug handshake_negotiation_fail_total already publishes for the
// same fault, so one misconfiguration reads identically on both signals.
EXPECT_EQ(std::string_view(peer_span::val::selfConnection), "self_connection");
}
TEST(LedgerSpanNames, peer_dial_outcome_values_are_mutually_distinct)
@@ -601,10 +605,11 @@ TEST(LedgerSpanNames, peer_dial_outcome_values_are_mutually_distinct)
// The dial panel splits by this attribute, so two outcomes sharing a value
// would merge two different failure stages into one line -- and the stage
// is the whole diagnostic content of the dial signal.
std::array<std::string_view, 5> const values{
std::array<std::string_view, 6> const values{
peer_span::val::connected,
peer_span::val::tcpFail,
peer_span::val::tlsFail,
peer_span::val::selfConnection,
peer_span::val::upgradeFail,
peer_span::val::timeout};
for (std::size_t i = 0; i < values.size(); ++i)

26
src/xrpld/app/misc/detail/ValidatorSite.cpp
View File

@@ -395,16 +395,26 @@ ValidatorSite::reportFetchOutcome(
// the time series stable when a site redirects.
//
// The label is rebuilt from the parsed parts rather than using the raw
// configured URI: [validator_list_sites] accepts credentials in the URI,
// and ParsedUrl keeps them in username/password. Emitting the raw string
// would copy them into a metric label, from which they would reach the
// collector, Prometheus and every dashboard. Scheme, host, port and path
// are all a reader needs to tell one site from another.
// configured URI. [validator_list_sites] accepts userinfo in the URI and
// ParsedUrl retains it in username/password even though the fetch itself
// never sends it, so the label was the one place a configured
// `https://user:pass@host` could surface -- and from there it would reach
// the collector, Prometheus and every dashboard.
//
// Scheme, host and path only, and the path truncated at the first '?' or
// '#'. Two reasons:
// - The port is omitted because the Resource constructor defaults it to
// 443/https and 80/http when the config omits one. Including it would
// rewrite the existing `https://vl.ripple.com` series as
// `https://vl.ripple.com:443/` and break continuity for every deployment
// already scraping this metric.
// - parseUrl's path group is `(/.*)?`, which is greedy to end of string, so
// a query or fragment lands inside `path`. A list URL authenticated by
// `?token=...` would otherwise leak through the label the same way
// userinfo would.
auto const& url = sites_[siteIdx].loadedResource->pUrl;
std::string siteLabel = url.scheme + "://" + url.domain;
if (url.port)
siteLabel += ":" + std::to_string(*url.port);
siteLabel += url.path;
siteLabel += url.path.substr(0, url.path.find_first_of("?#"));
XRPL_METRIC_COUNTER_INC_LABELED(
app_,

12
src/xrpld/overlay/detail/ConnectAttempt.cpp
View File

@@ -354,11 +354,13 @@ ConnectAttempt::onHandshake(error_code ec)
if (!overlay_.peerFinder().onConnected(
slot_, beast::IPAddressConversion::fromAsio(localEndpoint)))
{
// Not a TLS failure: the handshake succeeded and PeerFinder simply
// already holds a slot for this address. Reporting it as tls_fail
// conflated ordinary dial churn with peers we cannot speak to.
reportOutcome(telemetry::peer_span::val::duplicate);
fail("Duplicate connection");
// Not a TLS failure: the handshake succeeded and PeerFinder then
// recognised the remote address as our own. Logic::onConnected has
// exactly one false-returning path and it is the self-connect check
// ("Logic dropping as self connect"), so this branch means we dialled
// ourselves -- a local misconfiguration, not an unreachable peer.
reportOutcome(telemetry::peer_span::val::selfConnection);
fail("Self connection");
return;
}

19
src/xrpld/overlay/detail/ConnectAttempt.h
View File

@@ -158,18 +158,25 @@ private:
* |
* +-- onTimer ................................. "timeout"
* +-- onConnect (connect / local_endpoint) . "tcp_fail"
* +-- onHandshake (TLS / slot / shared value) "tls_fail"
* +-- onHandshake
* | +-- TLS handshake / shared value ....... "tls_fail"
* | +-- PeerFinder rejects our own address . "self_connection"
* +-- onWrite / onRead / onShutdown ............. "upgrade_fail"
* +-- processResponse
* +-- bad status / protocol / activate ... "upgrade_fail"
* +-- PeerImp created + addActive ........ "connected"
*
* The slot branch is drawn separately from the TLS one because
* `Logic::onConnected` fails for exactly one reason -- the remote address is
* ours -- and that is a local misconfiguration rather than an unreachable
* peer.
*
* @param outcome One of the `peer_span::val` dial-outcome constants:
* `connected`, `tcpFail`, `tlsFail`, `upgradeFail`, `timeout`. Taken
* as a string_view over a compile-time constant, so no allocation
* happens on the caller side. The constants are the single source
* for both the counter label and the span attribute, so the two
* cannot drift apart.
* `connected`, `tcpFail`, `tlsFail`, `selfConnection`, `upgradeFail`,
* `timeout`. Taken as a string_view over a compile-time constant, so
* no allocation happens on the caller side. The constants are the
* single source for both the counter label and the span attribute, so
* the two cannot drift apart.
*
* @note Per-connection path: one dial per outbound peer, so this is not
* a hot loop.

33
src/xrpld/overlay/detail/PeerSpanNames.h
View File

@@ -85,26 +85,29 @@ namespace val {
* cannot drift apart: the dial state machine names its outcome once and both
* signals receive that same value.
*
* - connected: the peer was activated and added to the overlay.
* - tcp_fail: the TCP connect or local-endpoint read failed.
* - tls_fail: the TLS handshake or the shared-value exchange failed.
* - duplicate: TLS succeeded but PeerFinder already holds a slot for this
* address, so the attempt was redundant rather than faulty.
* - upgrade_fail: TLS succeeded but the HTTP upgrade, protocol negotiation
* or activation was rejected.
* - timeout: the attempt never reached any terminal state in time.
* - connected: the peer was activated and added to the overlay.
* - tcp_fail: the TCP connect or local-endpoint read failed.
* - tls_fail: the TLS handshake, or the shared-value read taken before
* the HTTP upgrade, failed.
* - self_connection: TLS succeeded and then PeerFinder recognised the remote
* address as one of our own, so we had dialled ourselves.
* - upgrade_fail: TLS succeeded but the HTTP upgrade, protocol negotiation
* or activation was rejected.
* - timeout: the attempt never reached any terminal state in time.
*
* `duplicate` is separate from `tls_fail` on purpose. Dialling an address we
* are already connected to is normal churn on a healthy node, while a TLS
* failure means the peer could not be spoken to at all. Reporting both as
* `tls_fail` made a rising TLS-failure count unreadable: it could equally mean
* broken peers or merely a busy PeerFinder, and the two need opposite
* responses.
* `self_connection` is separate from `tls_fail` because it is a local
* misconfiguration, not an unreachable peer: the node has its own address in
* `[ips_fixed]` or behind its advertised endpoint, and every dial to it is
* wasted. Counting it as a TLS failure made a rising `tls_fail` unreadable --
* broken peers and a self-dial loop need completely different responses. The
* slug matches `handshake_fail::selfConnection` on
* `handshake_negotiation_fail_total`, so the same fault reads the same way
* whichever signal surfaces it.
*/
inline constexpr auto connected = makeStr("connected");
inline constexpr auto tcpFail = makeStr("tcp_fail");
inline constexpr auto tlsFail = makeStr("tls_fail");
inline constexpr auto duplicate = makeStr("duplicate");
inline constexpr auto selfConnection = makeStr("self_connection");
inline constexpr auto upgradeFail = makeStr("upgrade_fail");
inline constexpr auto timeout = makeStr("timeout");
} // namespace val

10
src/xrpld/telemetry/MetricNames.h
View File

@@ -88,12 +88,16 @@
*
* Example usage -- edge case: a value that must NOT be a constant. The site
* URI is runtime data, so only the KEY is named here; declaring the value
* would imply a bounded set that does not exist:
* would imply a bounded set that does not exist. Note the value is built from
* the PARSED url, not the configured string: the config accepts userinfo and a
* query, and either would otherwise ride the label into Prometheus.
* @code
* auto const& url = sites_[siteIdx].loadedResource->pUrl;
* std::string site = url.scheme + "://" + url.domain;
* site += url.path.substr(0, url.path.find_first_of("?#"));
* XRPL_METRIC_COUNTER_INC_LABELED(
* app_, metric::unlFetchTotal, "...",
* {{label::site, std::string(sites_[siteIdx].loadedResource->uri)},
* {label::outcome, std::string(outcome)}});
* {{label::site, site}, {label::outcome, std::string(outcome)}});
* @endcode
*
* @note Header-only and dependency-free: nothing here includes an OTel or an