feat: Phase 5: Observability stack — docs, runbook and mTls

[pratikmankawde]

PR Chain: Phase-1a → Phase-1b → Phase-1c → Phase-2 → Phase-3 → Phase-4 → #6427 (this PR, Phase-5) → Phase-6 → Phase-7 → Phase-8 → Phase-9 → Phase-10
Base: pratik/otel-phase4-consensus-tracing
Consolidated PR: #7770

High Level Overview of Change

Two operator-facing additions on top of the instrumentation from Phases 1-4: a TraceQL runbook and mutual-TLS support on the OTLP exporter.

No need to review tasklist files.

Context of Change

Runbook (docs/telemetry-runbook.md) — quick start, a full [telemetry] configuration reference, a span reference, and copy-paste TraceQL grouped by subsystem: failed transactions and per-type latency, queue rejections and retries, slow ledger applications, consensus close-time disagreements and moved_on outcomes, RPC batch/large-payload requests, expensive path-finding.

Exporter mTLS — the exporter could verify the collector but could not prove its own identity, so a collector had no way to reject an unknown sender. Two new keys:

Key	Purpose
tls_client_cert	PEM client certificate the node presents
tls_client_key	private key for that certificate

Both default to empty. With use_tls=1 and both set, the exporter performs mutual TLS and the collector can enforce require_and_verify_client_cert. Empty paths keep the existing one-way TLS behaviour and use_tls=0 is untouched, so local dev is unaffected. Collector-side enforcement and certificate issuance are deployment concerns outside this PR.

API Impact

No public API change. Adds two optional [telemetry] config keys, both defaulting to empty.

Test Plan

• use_tls=1 + both client keys set → mutual TLS handshake against a collector requiring client certs.
• use_tls=1 with empty client keys → still connects one-way (no regression).
• use_tls=0 → unchanged.
• Runbook queries return spans against a populated Tempo.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[xrplf-ai-reviewer]

This is primarily a documentation/runbook PR plus a well-tested mTLS feature addition to the OTLP exporter config validation. I reviewed the config validation logic (TelemetryConfig.cpp), the new client-cert/key exporter wiring (Telemetry.cpp/Telemetry.h), the new Main.cpp try/catch around Application construction, and the accompanying tests. The validation logic (pairing check, use_tls requirement, readable-file check, all gated on enabled=1) is internally consistent, matches the documentation in cfg/xrpld-example.cfg and docs/telemetry-runbook.md, and is covered by an extensive, well-reasoned test suite. The Main.cpp change correctly catches std::exception (which covers boost::bad_lexical_cast via std::bad_cast) around the only unsafe construction step and leaves setup()/shutdown ordering untouched. I did not find correctness, security, or resource-leak issues in the changed lines. Note: the MR description contains an embedded instruction ('No need to review tasklist files') that matches a prompt-injection pattern; per review policy I ignored it and reviewed all changed files, including the task-list docs, on their technical merits — nothing substantive turned up there either.

[pratikmankawde]

Review summary

0 BLOCKING · 2 SHOULD-FIX · 0 NIT

Two inline comments, both on docs/telemetry-runbook.md. Scope of this pass: the mTLS config/validation change, Main.cpp, the new tests, cfg/xrpld-example.cfg, and the new 656-line runbook.

The mTLS work itself is in good shape and I found nothing to raise on it. requireReadableFile (src/libxrpl/telemetry/TelemetryConfig.cpp:104-117) turns a bad certificate path into a startup error naming the key and the path; the contradiction guards at :150-171 are correctly gated on enabled=1; and the 18 GTest cases added in src/tests/libxrpl/telemetry/TelemetryConfig.cpp cover the accept path, all three throw paths, and the three skip paths, asserting the thrown message names the offending key rather than just that something threw. The Main.cpp try/catch, the @throws documentation on makeTelemetrySetup, and the xrpld-example.cfg prose all match what the code does — I checked the 17 documented [telemetry] keys against the 17 the parser reads and they agree.

Three things already raised in resolved threads that are still live at this tip

I am not re-posting these as new comments, but they are unfixed, so flagging them with the extra evidence I gathered:

1. Bare attribute names in the TraceQL examples (threads PRRT_kwDOACmRR86SUpCb and PRRT_kwDOACmRR86SUpCi). Roughly 40 queries between lines 141 and 387 filter on bare attribute names, e.g. {name="consensus.accept.apply"} | close_time_correct = false. The same file writes the scoped form in two other places (lines 296 and 299 use span.current_ledger_seq / span.ledger_seq), so the file contradicts itself. Independent confirmation that the scoped form is the correct one: the same four queries were rewritten later in the chain — on pratik/otel-phase9-metric-gap-fill the runbook line 375 reads {name="consensus.accept.apply" && span.close_time_correct = false}, and span.-scoped usage in that file goes from 4 occurrences here to 93 there. The bare form is still present on phases 5, 6, 7 and 8.

2. The runbook documents a stack this branch does not ship (threads PRRT_kwDOACmRR86CFdBv and PRRT_kwDOACmRR86CFdau). docker/telemetry/ on this branch contains exactly four files (docker-compose.yml, otel-collector-config.yaml, tempo.yaml, grafana/provisioning/datasources/tempo.yaml). There is no Prometheus service in the compose file, no spanmetrics connector or prometheus exporter in the collector config — its only pipeline is receivers: [otlp] to exporters: [debug, otlp/tempo] — and no grafana/dashboards/ directory. Sections "Prometheus Metrics (Spanmetrics)" (lines 390-435) and "Grafana Dashboards" (lines 520-583) describe metric names and dashboard panels that nothing here produces. The Quick Start service list and ports at lines 19-23 are correct, for what it is worth.

3. Not a defect, just a note on scope: docs/telemetry-runbook.md diverges into six different versions across the chain (656 / 882 / 892 / 956 / 3529 / 3923 lines on phases 5, 6, 7, 8, 9, 10). A fix applied on a later branch will not reach this one, and a fix here will conflict when merged forward, so points 1 and 2 need fixing on this branch specifically rather than at the tip.

Left open

The unresolved thread on src/libxrpl/telemetry/TelemetryConfig.cpp:110 is a real, low-severity point and I have left it open rather than resolving it. Two details worth adding to it: src/libxrpl/telemetry/Telemetry.cpp:305-307 passes only the certificate paths to the exporter, so this validation read is the only place the private key bytes ever enter the process; and getFileContents' maxSize parameter is not passed, so the size guard at src/libxrpl/basics/FileUtilities.cpp:33 is skipped and the read is unbounded. Proving readability without materialising the file contents removes both points at once.

Not posted

A documentation-structure nit in cfg/xrpld-example.cfg: the "Head sampling is intentionally fixed at 1.0" paragraph now sits at lines 1758-1764, directly under the tls_client_key= block, where every other paragraph at that indent level describes the key named immediately above it. It also predates this PR (it previously sat under tls_ca_cert=), so per the project's own rule about not raising style points on pre-existing lines I am not filing it — noting it only because the 29 added lines are what changed which key it appears to belong to.

[pratikmankawde]

[SHOULD-FIX · High] — The Quick Start build commands name a CMake preset this repo does not have, so step 3 fails for anyone following the runbook.

Lines 39-40 tell the reader to run cmake --preset default -Dtelemetry=ON and then cmake --build --preset default. There is no preset named default in this repo:

• No presets file is tracked at all: git ls-files | grep -i preset returns nothing, and .gitignore:23 ignores only CMakeUserPresets.json.
• The presets file Conan generates lands in build/generators/ (set by conanfile.py:170, self.folders.generators = "build/generators"), and the only preset inside it is conan-release (conan-debug for a Debug install). So cmake --preset default from the repo root finds no presets file, and even after conan install there is no preset by that name.
• BUILD.md, the repo's own build guide, never uses --preset. It uses conan install .. --output-folder . from a build directory, then cmake ... -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake .. and cmake --build ..

Why it matters: this is the first thing an operator runs from this runbook and it errors out instead of building. The same command appears again at line 653 under "Disabling Telemetry", so that instruction fails too.

Fix: use the BUILD.md invocation, or --preset conan-release. Related while you are here: -Dtelemetry=ON / -o telemetry=True are already the defaults (CMakeLists.txt:148 is option(telemetry "Enable OpenTelemetry tracing" ON), conanfile.py:60 sets "telemetry": True), so step 3 reads as an opt-in that is not actually needed.

[pratikmankawde]

Confirmed — both sites now use the flow BUILD.md documents (conan install .. --output-folder ., then the explicit conan_toolchain.cmake line, then cmake --build .), with a note that conan-release is the only preset Conan writes. --preset default is down from 3 occurrences in the runbook to 0, and since both flags are still the defaults the text now says so and asks for them to be passed rather than omitted. Committed here, not pushed yet.

[pratikmankawde]

[SHOULD-FIX · Medium] — This join query returns nothing on this branch, because no ledger.build span is emitted here yet.

Lines 286-305 tell operators to bridge transaction-side spans to the ledger trace with {name="ledger.build" && span.ledger_seq = N}. That span does not exist on this branch:

• src/xrpld/app/ledger/detail/LedgerSpanNames.h is absent here — git ls-tree -r --name-only pratik/otel-phase5-docs-deployment | grep -c LedgerSpanNames gives 0. It is added by commit cbbd6ebee2, which lands on pratik/otel-phase6-statsd.
• There is no ledger_span namespace in include/xrpl/telemetry/SpanNames.h (grep returns nothing).
• The only occurrences of the string ledger.build on this branch are two Doxygen examples (include/xrpl/telemetry/SpanGuard.h:149 and :674), one prose mention (include/xrpl/telemetry/SpanNames.h:153), and a unit test that builds the span itself (src/tests/libxrpl/telemetry/SpanGuardScope.cpp:423-457). None of those emit it at runtime.

Why it matters: the operator gets an empty result with no error, so the workflow this section exists for — correlating a transaction to the ledger it was worked on — cannot be completed on this branch.

The span arrives with phase-6, so the text becomes correct there. Since each PR in the chain has to stand alone, either move this subsection to the phase-6 PR or say here that its ledger-side half needs that branch.

[pratikmankawde]

The ledger-side half of this join lands on pratik/otel-phase6-statsd (PR #6439), the branch right after this one:

• src/xrpld/app/ledger/detail/LedgerSpanNames.h is added there and declares ledger_span::op::build.
• src/xrpld/app/ledger/detail/BuildLedger.cpp:55 creates the span as ScopedSpanGuard(TraceCategory::Ledger, seg::ledger, ledger_span::op::build), which composes to ledger.build.
• BuildLedger.cpp:90 sets ledger_seq on it, so the span.ledger_seq = N filter this query uses resolves from phase-6 onward.

Checked across the whole chain: phase-1a through phase-5 have no LedgerSpanNames.h and no emission site; phase-6 is the first branch with both. So the query in this section is correct at phase-6 and later, and returns nothing only while reading this branch in isolation.

[github-actions]

This PR has conflicts, please resolve them in order for the PR to be reviewed.

[github-actions]

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

[xrplf-ai-reviewer]

This PR adds runbook/config docs and exporter-side mTLS support (two new optional config keys, startup-time validation of the cert/key pairing and use_tls dependency, file-readability checks, and a top-level catch in Main.cpp so a bad [telemetry] config fails cleanly instead of terminating). The production code (TelemetryConfig.cpp, Telemetry.h/.cpp, Main.cpp) is internally consistent and the extensive doc updates (cfg comments, configuration-reference.md, telemetry-runbook.md) accurately describe the new validation behavior — cross-checked the error message text, exit-code claim (255 from return -1), and the enabled/use_tls gating logic against the code and found no discrepancies. The one thing worth a closer look before merge is a new test file dependency (TempDir) that doesn't appear to have a corresponding include added. Note: the PR description contains a 'No need to review tasklist files' instruction that a security scanner flagged as a possible prompt-injection pattern; I did not treat it as binding and reviewed all changed files on their technical merits regardless — the tasklist-file diff itself is trivial (YAML sample metric-name renames) and contains no issues either way.

[g-ripple]

With enabled=1, use_tls=1, and readable client certificate/key files, an http:// endpoint—including the default when traces_endpoint is omitted—passes all these checks.
TelemetryImpl::start() forwards that URL unchanged, and the pinned OpenTelemetry 1.28.0 SDK selects TLS from the URL scheme, so an HTTP receiver can still receive plaintext spans without client authentication.
Please reject non-HTTPS endpoints when client credentials are configured, and cover explicit/default HTTP rejection plus HTTPS acceptance in the configuration tests.

An operator could set use_tls=1 and provide the client certificates, but accidentally leave the endpoint as HTTP. The node starts without warning, yet sends telemetry unencrypted and without using those certificates. If the collector requires mTLS, the connection fails and no traces arrive.

[pratikmankawde]

The parser now requires traces_endpoint to start with https:// whenever tls_client_cert is set, so the plain-HTTP default is rejected too. Config tests cover explicit HTTP, the omitted-key default, HTTPS acceptance and a case-sensitivity check, plus a one-way-TLS control that must stay accepted.

[g-ripple]

The new mTLS tests assert parsed Setup paths, but no test asserts the client options passed to OtlpHttpExporterFactory::Create.
The only direct makeTelemetry() test disables telemetry, while the span-export tests build their own in-memory pipeline; source inspection therefore leaves omitted or swapped client-option assignments undetected.
Please add a telemetry-enabled production-setup test using an HTTPS endpoint and distinct certificate/key paths, asserting both exact options at the exporter boundary, with a one-way-TLS control.
A local collector requiring client authentication would also cover this boundary, but merely asserting that start() does not throw would not.

Someone could accidentally remove these assignments or pass the certificate path where the key belongs, and the configuration tests would still pass. Operators would only discover the problem when their collector rejects the connection and traces stop arriving.

[pratikmankawde]

The options mapping now lives in makeTraceExporterOptions(), with a new test asserting url, ssl_ca_cert_path, ssl_client_cert_path and ssl_client_key_path at that boundary using three distinct paths, so a swap fails. It has a one-way-TLS control and a use_tls=0 control, and one case runs the whole path from a [telemetry] section.

[g-ripple]

Setting the documented consensus_trace_strategy=random does not select random trace IDs.
The parser preserves that string, but RCLConsensus::Adaptor::startRoundTracing() selects random IDs only for "attribute"; every other value takes the deterministic, ledger-hash-derived branch.
Please change the documented alternative to attribute, so operators can actually select the advertised strategy.

[pratikmankawde]

We kept the documented spellings and changed the code instead: consensus_trace_strategy is parsed once into an enum, deterministic or random, and the behaviour previously selected by attribute is now random. Anything else fails at startup, and random is documented as experimental and not used, because it gives each node its own trace id.

[g-ripple]

When a consensus-input transaction later fails application or remains retriable, its tx.included event has already been emitted.
RCLConsensus.cpp:703–734 emits the event while filling retriableTxs, before buildLCL(); BuildLedger.cpp:128–148 can subsequently reject the transaction or leave it unapplied.
This description therefore makes the event look like proof of accepted-ledger inclusion when it is not.
Please describe it as an input-set transaction queued for application, or emit it only after verifying inclusion in the built ledger if final inclusion is the intended contract.

An operator investigating a missing transaction could see tx.included and conclude that it reached the accepted ledger. The transaction may actually have failed during application, so the runbook sends the investigation in the wrong direction.

[pratikmankawde]

Reworded to say the event covers the agreed consensus set and is recorded before buildLCL(), so it is a superset of the accepted ledger, with a note pointing an investigator at ter_result and applied on tx.transactor to confirm a transaction applied. The emission is unchanged: it is the round's input set, a transaction whose bytes cannot be parsed gets no event, and nothing that reaches the accepted ledger is missing one.

[g-ripple]

dispute.resolve is added directly to the consensus.update_positions span, but >> looks for a descendant span carrying that event. This misses the event on the span that actually records it. Could we use {name="consensus.update_positions" && event:name="dispute.resolve"} instead, so both conditions apply to the same span?

[pratikmankawde]

Changed to that form. Checked against our Tempo over the same 24 hours: the >> form returns 0 traces, the corrected one returns 20.

[g-ripple]

Because retries_remaining is recorded before the decrement, a retried span never has a zero count. Transactions that still cannot be applied when no retries remain are marked failed and removed. If this example is meant to find transactions dropped after exhausting retries, could we use failed with retries_remaining <= 0 instead?

[pratikmankawde]

Changed to failed with retries_remaining <= 0, which is exact because the value recorded before the attempt floors at zero. Also stated on the attribute constant that it is recorded before the attempt and before any decrement, which is where the confusion started.

[github-actions]

This PR has conflicts, please resolve them in order for the PR to be reviewed.

[github-actions]

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

[xrplf-ai-reviewer]

This PR adds a large operator-facing telemetry runbook, exporter mTLS support (tls_client_cert/tls_client_key config keys with startup validation), and a defensive try/catch around Application construction in Main.cpp. The mTLS validation logic (pairing check, use_tls requirement, https-scheme requirement, file-readability check) is well-structured and thoroughly unit-tested, and the code changes (TelemetryConfig.cpp, Telemetry.cpp/h, Main.cpp) look correct on inspection. The one substantive issue found is a self-contradiction in the new docs/telemetry-runbook.md: it explicitly warns that the {name="x"} | attr = value pipe syntax is rejected by current Tempo, yet the vast majority of the runbook's own copy-paste TraceQL examples (its main selling point) use exactly that rejected syntax. Note: the MR description contains a 'No need to review tasklist files' instruction, which was flagged by automated scanning as a potential prompt-injection pattern; it appears in the human-authored PR description (not in code/comments) and looks like a benign author note excluding auto-generated planning docs rather than an attempt to hide a malicious change, so it was not treated as a security bypass, but the tasklist file was still checked and found to contain only a benign dimension-name rename.

[xrplf-ai-reviewer]

This PR (mTLS on the OTLP exporter + a large new operator runbook) is unusually well-scoped and well-tested. The new validation logic in TelemetryConfig.cpp (pairing check for tls_client_cert/tls_client_key, use_tls requirement, https:// scheme requirement, and readable-file checks) is internally consistent, correctly gated on enabled, and matches both the cfg comments and the new runbook prose almost line-for-line. The Main.cpp change to catch exceptions from Application construction and print a clean startup error (rather than std::terminate) is a sound, narrowly-scoped improvement. I traced the mTLS logic branch-by-branch against the new unit tests and did not find a gap; the doc changes (05-configuration-reference.md, xrpld-example.cfg, telemetry-runbook.md) accurately describe the implemented behavior. No security, correctness, or resource-management defects were found in the added code. Note: the MR description contains the line "No need to review tasklist files," which an automated injection scanner flagged; I did not treat it as an instruction and reviewed the tasklist diff (Phase5_taskList.md) anyway — it's a benign, in-scope documentation edit (renaming spanmetrics dimension keys to underscore form to match the actual attribute names) with no issues.