From 8d2e2d15af374f7a5702c9d83af5137f84244cff Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 9 Sep 2026 13:14:21 +0100 Subject: [PATCH] docs(telemetry): correct stale code citations and the log-correlation claim Every citation below was checked against the file it names: - LedgerMaster.cpp:463 is fixIndex, not the ledger.store span; that guard is at :470 and the insert it wraps at :476 - LedgerMaster.cpp:987 is the tvc assignment, which sits BEFORE the tvc < minVal return at :988; the ledger.validate span opens at :1003 - ServerHandler.cpp:705 is inside makeJsonError; processRequest is at :718 - docker-compose.yml:71 and :75 are comments in the collector's volume block; the loki service is at :112 and its config command at :116 Two claims were also wrong rather than merely stale. Log-trace correlation is gated in CI, because the workflow passes no --skip-loki, and the separate check in integration-test.sh is run by no workflow at all. The Loki label note described the Grafana Cloud collector config rather than the local one: only the cloud variant sets job=xrpld, and the local config's own comment says to select on service_name. The dashboards carry 35 Loki queries, not 38. --- .../05-configuration-reference.md | 2 +- OpenTelemetryPlan/06-implementation-phases.md | 11 ++++++++--- OpenTelemetryPlan/07-observability-backends.md | 2 +- OpenTelemetryPlan/Phase10_taskList.md | 5 +++-- docker/telemetry/TESTING.md | 18 ++++++++++-------- docs/telemetry-runbook.md | 6 +++--- 6 files changed, 26 insertions(+), 18 deletions(-) diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 29614bac71..ab473e667c 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -584,7 +584,7 @@ Fluentd or PerfLog change. Two pieces: > only an **allow-listed** set of resource attributes to indexed stream labels > (`service.name`, `service.namespace`, `service.instance.id`, > `deployment.environment`, the `k8s.*`/`cloud.*` keys); `job` is not on that -> list, and this repo ships no Loki config override — `docker-compose.yml:75` +> list, and this repo ships no Loki config override — `docker-compose.yml:116` > starts Loki with the image's built-in `/etc/loki/local-config.yaml`. `job` > therefore lands in **structured metadata**, which cannot appear in a stream > selector, so `{job="xrpld"}` returns an empty result rather than an error. diff --git a/OpenTelemetryPlan/06-implementation-phases.md b/OpenTelemetryPlan/06-implementation-phases.md index 7ebcc77d1b..850ccf982d 100644 --- a/OpenTelemetryPlan/06-implementation-phases.md +++ b/OpenTelemetryPlan/06-implementation-phases.md @@ -734,13 +734,18 @@ flowchart LR absent or the context is invalid (`Log.cpp:310-318`) - [x] Loki ingests xrpld logs via OTel Collector filelog receiver — `otel-collector-config.yaml:38` (`filelog`); `loki` service in - `docker-compose.yml:71` + `docker-compose.yml:112` - [x] Grafana Tempo → Loki one-click correlation works — `provisioning/datasources/tempo.yaml:32` (`tracesToLogs`) - [x] Grafana Loki → Tempo reverse lookup works via derived field — `provisioning/datasources/loki.yaml:16` (`derivedFields`) -- [ ] Integration test verifies trace_id presence in logs — implemented in the - Phase 10 harness, but CI runs it with `--skip-loki`, so it is not gated +- [ ] Integration test verifies trace_id presence in logs — CI gates this + through the Phase 10 harness's `validate_telemetry.py`, whose + `log.trace_id_present` and `log.trace_id_cross_reference` checks run + because the workflow passes no `--skip-loki`. That harness and + `.github/workflows/telemetry-validation.yml` live on the Phase 10 branch, + not here. `docker/telemetry/integration-test.sh:79-126` carries a separate + trace_id-in-logs check that no workflow under `.github/workflows/` runs - [ ] No performance regression from trace_id injection (< 0.1% overhead) — needs the Phase 10 benchmark suite diff --git a/OpenTelemetryPlan/07-observability-backends.md b/OpenTelemetryPlan/07-observability-backends.md index daa7fa9693..8dcfe61982 100644 --- a/OpenTelemetryPlan/07-observability-backends.md +++ b/OpenTelemetryPlan/07-observability-backends.md @@ -507,7 +507,7 @@ These are journal (`debug.log`) lines, not PerfLog lines — see §7.7.2. > **allow-listed** set of resource attributes to indexed stream labels > (`service.name`, `service.namespace`, `service.instance.id`, > `deployment.environment`, `k8s.*`, `cloud.*`), and `job` is not on it. This -> repo mounts no Loki config override (`docker-compose.yml:75` uses the image's +> repo mounts no Loki config override (`docker-compose.yml:116` uses the image's > built-in `local-config.yaml`), so `job` lands in **structured metadata** — > queryable only with a `|` filter after a selector, never as the selector > itself. A `{job="xrpld"}` query returns empty with no error, which is why this diff --git a/OpenTelemetryPlan/Phase10_taskList.md b/OpenTelemetryPlan/Phase10_taskList.md index 28a1cd28fb..b985b054f7 100644 --- a/OpenTelemetryPlan/Phase10_taskList.md +++ b/OpenTelemetryPlan/Phase10_taskList.md @@ -150,7 +150,7 @@ Before Phases 1-9 can be considered production-ready, we need proof that: - HTTP: `rpc.http_request` → `rpc.process` → `rpc.command.*` - WebSocket: `rpc.ws_message` → `rpc.command.*` — **there is no `rpc.process` on the WS path**. `rpc.process` is created only in - `ServerHandler::processRequest()` (`ServerHandler.cpp:705`), reached from + `ServerHandler::processRequest()` (`ServerHandler.cpp:718`), reached from `processSession(Session, coro)`, i.e. HTTP only. Under WS-only load `rpc.process` never appears, and `rpc.command.*` parents directly to `rpc.ws_message`. @@ -276,7 +276,8 @@ Before Phases 1-9 can be considered production-ready, we need proof that: (totals computed dynamically from `expected_spans.json` / `expected_metrics.json`, not the stale 16 / 22 figures) - [ ] Log-trace correlation validated end-to-end (Loki ↔ Tempo) — implemented, - but CI runs with `--skip-loki`, so it is not gated + and gated in CI: the workflow passes no `--skip-loki`, so + `validate_telemetry.py` builds and runs both log-correlation checks - [ ] All 14 harness-asserted Grafana dashboards render data (no empty panels); 15 on disk - [ ] Benchmark shows < 3% CPU overhead, < 5MB memory overhead diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 73daa140c4..412db2d331 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -631,20 +631,22 @@ curl -sG "http://localhost:3100/loki/api/v1/query" \ Expected: > 0 results. -> **Use `service_name`, not `job`.** The collector's `resource/logs` processor -> applies an `upsert` to **both** `service.name=xrpld` and `job=xrpld` -> (`otel-collector-config.yaml:57-70`), and its comment says the `job` attribute -> is there so operators can paste `{job="xrpld"}`. That does not work: on OTLP -> ingest Loki promotes only an allow-listed set of resource attributes to indexed -> stream labels (`service.name` → `service_name`, plus `service.namespace`, +> **Use `service_name`, not `job`.** The local stack's `resource/logs` processor +> sets one key, `service.name=xrpld` (`otel-collector-config.yaml:84-86`); its +> comment there explains that a custom `job` attribute is not promoted to a +> stream label and tells you to select on `service_name`. Only the Grafana Cloud +> variant also sets `job=xrpld` (`otel-collector-config.grafanacloud.yaml:73-75`). +> Either way `{job="xrpld"}` does not work as a selector: on OTLP ingest Loki +> promotes only an allow-listed set of resource attributes to indexed stream +> labels (`service.name` → `service_name`, plus `service.namespace`, > `service.instance.id`, `deployment.environment`, `k8s.*`, `cloud.*`), and `job` > is not on the list. This repo mounts no Loki config override — the `loki` > service runs the image's built-in `/etc/loki/local-config.yaml` -> (`docker-compose.yml:75`) — so `job` lands in **structured metadata**, which +> (`docker-compose.yml:116`) — so `job` lands in **structured metadata**, which > cannot be a stream selector. `{job="xrpld"}` therefore returns **zero results > with no error**, which reads exactly like "logs are not being ingested". If > this query is empty, check `{service_name="xrpld"}` before debugging the -> pipeline. All 38 Loki queries in the shipped dashboards select on +> pipeline. All 35 Loki queries in the shipped dashboards select on > `service_name`; none uses `job`. ### Step 4: Verify Grafana Tempo-to-Loki correlation diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index adc65f20fe..e1dc0ef7ad 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -985,7 +985,7 @@ flowchart TB (`tvc < minVal`) it returns early with no promotion — a built ledger that loses is abandoned ([LedgerMaster.cpp:980](../src/xrpld/app/ledger/detail/LedgerMaster.cpp#L980); [docs/consensus.md:50](consensus.md)). The `ledger.validate` span is emitted only - inside `checkAccept` ([LedgerMaster.cpp:987](../src/xrpld/app/ledger/detail/LedgerMaster.cpp#L987)). + inside `checkAccept` ([LedgerMaster.cpp:1003](../src/xrpld/app/ledger/detail/LedgerMaster.cpp#L1003)). - **validation-send guard**: broadcast only if `validating_ && isCompatible && !consensusFail && canValidateSeq(seq)` — silently suppressed for incompatible ledgers or an already-validated seq @@ -1152,8 +1152,8 @@ are pending a code fix: - **`ledger.acquire` / `ledger.store` / `ledger.validate` are not reliably roots either.** All three use `SpanGuard::span` ([InboundLedger.cpp:113](../src/xrpld/app/ledger/detail/InboundLedger.cpp#L113), - [LedgerMaster.cpp:463](../src/xrpld/app/ledger/detail/LedgerMaster.cpp#L463), - [987](../src/xrpld/app/ledger/detail/LedgerMaster.cpp#L987)), which inherits the + [LedgerMaster.cpp:470](../src/xrpld/app/ledger/detail/LedgerMaster.cpp#L470), + [1003](../src/xrpld/app/ledger/detail/LedgerMaster.cpp#L1003)), which inherits the ambient span ([SpanGuard.cpp:233](../src/libxrpl/telemetry/SpanGuard.cpp#L233)) rather than `freshRoot` ([245](../src/libxrpl/telemetry/SpanGuard.cpp#L245)) — the same defect as