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

fix(telemetry): park the span panels in their own row, tighten the site label

Browse Source 
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`.
This commit is contained in:
Pratik Mankawde
2026-07-28 18:36:22 +01:00
parent 85f45922c4
commit 35e0c0795d
6 changed files with 733 additions and 677 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
.github/scripts/otel-naming/check_otel_naming.py vendored
View File

@@ -966,6 +966,7 @@ def metric_constants(root: Path) -> Tuple[Set[str], Set[str], Set[str]]:
values: Set[str] = set()
for h in find_metricname_headers(root):
text = strip_comments(read_source(h))
flat = text
for ns, bucket in (
("metric", names),
("label", keys),
@@ -974,9 +975,16 @@ def metric_constants(root: Path) -> Tuple[Set[str], Set[str], Set[str]]:
for block in namespace_spans(text, ns):
for m in METRIC_CONST_DEF.finditer(block):
bucket.add(m.group(2))
# Remove the block before the flat pass below, so the
# enclosing namespace stays the only thing that decides a
# namespaced constant's bucket. Without this, a `kLabel`-style
# identifier written inside `namespace metric` lands in both
# `names` and `keys`, and an instrument name silently becomes
# an allowed label key for Rule D.
flat = flat.replace(block, "", 1)
# Flat style: classify by identifier prefix. Only constants outside the
# namespaced blocks reach here, so a namespaced header is unaffected.
for m in METRIC_CONST_DEF.finditer(text):
for m in METRIC_CONST_DEF.finditer(flat):
ident, value = m.group(1), m.group(2)
if ident.startswith("kLabel"):
keys.add(value)

31
.github/scripts/otel-naming/test_check_otel_naming.py vendored
View File

@@ -1041,6 +1041,37 @@ class MetricConstantExtraction(unittest.TestCase):
finally:
shutil.rmtree(d)
def test_namespaced_constant_ignores_its_flat_prefix(self):
# A `kLabel`-prefixed identifier written inside `namespace metric` must
# be an instrument name only. The flat prefix pass exists for headers
# that have no such namespace, so it must not also claim this one:
# letting it through would make an instrument name a valid label key,
# and Rule D would then accept `label_peer_total` as a label.
names, keys, vals = self._run(
_metric_header(
metric_body=_mc("kLabelPeerTotal", "label_peer_total"),
label_body=_mc("outcome", "outcome"),
)
)
self.assertEqual(names, {"label_peer_total"})
self.assertEqual(keys, {"outcome"})
self.assertEqual(vals, set())
def test_flat_prefix_still_read_when_namespaces_absent(self):
# The counterpart to the test above: with no `namespace metric`/`label`
# block to excise, the flat pass must still classify by prefix.
names, keys, vals = self._run(
"#pragma once\n"
"namespace xrpl::telemetry {\n"
+ _mc("kLabelOutcome", "outcome")
+ "\n"
+ _mc("kResultResolved", "resolved")
+ "\n}\n"
)
self.assertEqual(names, set())
self.assertEqual(keys, {"outcome"})
self.assertEqual(vals, {"resolved"})
class RuleIMetricLiterals(unittest.TestCase):
"""Rule I: literal instrument names / label keys at a metric emit site are

11
OpenTelemetryPlan/09-data-collection-reference.md
View File

@@ -1762,12 +1762,13 @@ top-to-bottom walks the same path a sync does:
5. `Job queue` — does arrived work ever get a worker thread?
6. `Quorum & publish` — does a held ledger ever validate, and reach clients?
7. `Terminal blockers & serving` — will the node stop validating for good?
8. `Back-fill & persistence` (collapsed) — is an existing database the bottleneck?
9. `Spans & traces` (collapsed) — _which_ fetch, peer or object, not how many?
8. `Back-fill & persistence` — is an existing database the bottleneck?
9. `Spans & traces` — _which_ fetch, peer or object, not how many?
Rows 8 and 9 are collapsed by default because they answer conditional questions:
row 8 only applies to a node with existing history, and row 9 is span-derived, so
it inherits trace sampling and the `trace_ledger` / `trace_peer` flags.
Rows 8 and 9 answer conditional questions: row 8 only applies to a node with
existing history, and row 9 is span-derived, so it inherits trace sampling and
the `trace_ledger` / `trace_peer` flags. They are still expanded by default --
a row an operator has to remember to open is a row they read too late.
Operator flow: [telemetry-runbook.md](../docs/telemetry-runbook.md)
"Diagnosing slow/stuck fresh sync". Terms:

1296
docker/telemetry/grafana/dashboards/ledger-sync-health.json
View File

File diff suppressed because it is too large Load Diff

30
docs/telemetry-runbook.md
View File

@@ -2704,17 +2704,17 @@ diagnosed from the **Ledger Sync Health** dashboard (uid `ledger-sync-health`).
Its nine rows are ordered the way a fresh node progresses, so reading the board
top-to-bottom walks the same path a sync does:
| # | Row | Question it answers |
| --- | ----------------------------------- | ---------------------------------------------------- |
| 1 | Bootstrap (Domain 0) | Can the node reach peers and form a quorum at all? |
| 2 | Peer supply | Does any peer hold what this node needs? |
| 3 | Sync state | Is the node advancing through the mode machine? |
| 4 | Ledger acquire & SHAMap fetch | Is ledger data arriving and being applied? |
| 5 | Job queue | Does arrived work ever get a worker thread? |
| 6 | Quorum & publish | Does a held ledger ever validate, and reach clients? |
| 7 | Terminal blockers & serving | Will the node stop validating for good? |
| 8 | Back-fill & persistence (collapsed) | Is an existing database the bottleneck? |
| 9 | Spans & traces (collapsed) | _Which_ fetch, peer or object — not how many? |
| # | Row | Question it answers |
| --- | ----------------------------- | ---------------------------------------------------- |
| 1 | Bootstrap (Domain 0) | Can the node reach peers and form a quorum at all? |
| 2 | Peer supply | Does any peer hold what this node needs? |
| 3 | Sync state | Is the node advancing through the mode machine? |
| 4 | Ledger acquire & SHAMap fetch | Is ledger data arriving and being applied? |
| 5 | Job queue | Does arrived work ever get a worker thread? |
| 6 | Quorum & publish | Does a held ledger ever validate, and reach clients? |
| 7 | Terminal blockers & serving | Will the node stop validating for good? |
| 8 | Back-fill & persistence | Is an existing database the bottleneck? |
| 9 | Spans & traces | _Which_ fetch, peer or object — not how many? |
**Start from the symptom, not from row 1.** Match what you observe to a branch
below; each branch names the panels that discriminate it, what healthy and
@@ -2844,7 +2844,7 @@ and 18.
The specific symptom: a node with history starts and is slower than the same node
was when empty. Back-fill is **write**-bound, so no read-side panel shows it.
Expand the collapsed **Back-fill & persistence** row.
Read the **Back-fill & persistence** row.
| Look at | Healthy | Unhealthy | Conclude |
| ----------------------------------------------------------- | ------------------------------------------------------------------------- | --------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
@@ -3315,7 +3315,7 @@ panel it reads.
database starts and syncs slower than a fresh one"**. Back-fill is
write-bound, so no read-side panel can show it; check this step whenever a
node with existing history is the slow one. Both panels live in the
collapsed **Back-fill & persistence** row — expand it.
**Back-fill & persistence** row.
Panel _NodeStore Write vs Read Latency (us/op)_ (`nodestore_state`,
`metric=node_writes_duration_us` / `node_reads_duration_us` rated against
their counts) with _NodeStore Operation Rate (writes vs reads)_
@@ -3351,7 +3351,7 @@ panel it reads.
Only relevant when `[ledger_replay]` is enabled. Panels _Replay Fallback to
Full Acquire (by stage)_ (`ledger_replay_fallback_total`) and _Replay
Outcomes (by terminal state)_ (`ledger_replay_outcome_total`), also in the
collapsed **Back-fill & persistence** row:
**Back-fill & persistence** row:
- **Any sustained fallback rate** — too few connected peers support the
`LedgerReplay` protocol feature, so every historical ledger is fetched
whole instead of as a delta. Back-fill still completes, just far slower,
@@ -3482,7 +3482,7 @@ panel it reads.
on?**
Every step above reads a native metric, which is an aggregate: it says how
much and how often, never _which one_. This step reads the **span-derived**
panels in the collapsed **Spans & traces** row, which answer the "which"
panels in the **Spans & traces** row, which answer the "which"
questions the aggregates structurally cannot — and each point on them is
backed by a trace, so it can be clicked through to the individual fetch,
request or dial.

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

@@ -401,19 +401,35 @@ ValidatorSite::reportFetchOutcome(
// `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.
// Scheme, host, port and path, with the path truncated at the first '?' or
// '#'. Three details:
// - The port appears only when it differs from the scheme's default. The
// Resource constructor fills in 443/https and 80/http when the config
// omits one, so `pUrl.port` alone cannot say whether an operator asked
// for a port; comparing against the default can. Always printing it
// would rewrite the existing `https://vl.ripple.com` series as
// `https://vl.ripple.com:443/` and break continuity, while always
// dropping it would merge two genuinely different local sites that
// differ only by port.
// - 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.
// - parseUrl's host group permits '@', so a malformed URI with two of them
// leaves the tail of the userinfo in `domain`. Cutting at the last '@'
// keeps that out of the label too.
auto const& url = sites_[siteIdx].loadedResource->pUrl;
std::string siteLabel = url.scheme + "://" + url.domain;
std::string host = url.domain;
if (auto const at = host.rfind('@'); at != std::string::npos)
host.erase(0, at + 1);
std::string siteLabel = url.scheme + "://" + host;
auto const defaultPort = url.scheme == "https" ? 443 : 80;
if (url.port && *url.port != defaultPort)
siteLabel += ":" + std::to_string(*url.port);
siteLabel += url.path.substr(0, url.path.find_first_of("?#"));
XRPL_METRIC_COUNTER_INC_LABELED(