feat: Phase 1b: Telemetry infrastructure — core library, build system, Docker stack

[pratikmankawde]

PR Chain: Phase-1a → #6437 (this PR, Phase-1b) → Phase-1c → Phase-2 → Phase-3 → Phase-4 → Phase-5 → Phase-6 → Phase-7 → Phase-8 → Phase-9 → Phase-10
Base: pratik/otel-phase1a-plan-docs
Consolidated PR: #7770

High Level Overview of Change

The core telemetry library: libxrpl tracing primitives, the opentelemetry-cpp build wiring, Application lifecycle integration, and a local Docker stack (Collector + Tempo + Grafana). No call sites are instrumented yet.

No need to review tasklist files.

Context of Change

• Two-type span API — SpanGuard is the transferable handle; ScopedSpanGuard is the RAII object that makes a span current for its scope. Splitting them keeps a moved-from value from being usable.
• ScopedSpanGuard::discard() — sets a thread-local flag and ends the span; FilteringSpanProcessor::OnEnd() (called synchronously on the same thread) drops it before it reaches the batch queue, so uninteresting spans cost no bandwidth or storage.
• CoroAwareContextStorage — carries the active context across xrpld's coroutine hops, which the SDK's thread-local storage alone would lose.
• DeterministicIdGenerator — lets a trace ID be derived from protocol state (used later by consensus so all nodes in a round share a trace).
• NullTelemetry — no-op fallback when the SDK fails to initialise or telemetry is off.
• Compile gate — XRPL_ENABLE_TELEMETRY. The Conan/CMake telemetry option is currently ON so CI keeps compiling this code; the runtime [telemetry] enabled key still defaults to 0, so a stock node emits nothing.
• Dependency — opentelemetry-cpp/1.28.0.

API Impact

• libxrpl change (new xrpl/telemetry/ headers; no existing symbol changed)

Test Plan

• Build with -o telemetry=True and with False; both link.
• docker compose -f docker/telemetry/docker-compose.yml up -d, then confirm the Tempo datasource is reachable in Grafana on :3000.

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/xrpld/app/main/Application.cpp	86.7%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[xrplf-ai-reviewer]

Telemetry enabled by default; null pointer and exception handling issues flagged inline.

[xrplf-ai-reviewer]

Null pointer to std::string_view constructor causes UB — guard before constructing:

Suggested change

	setAttribute(key, std::string_view(value));
	if (value == nullptr) return;
	setAttribute(key, std::string_view(value));

Suggested change

	setAttribute(key, std::string_view(value));
	setAttribute(key, value ? std::string_view(value) : std::string_view{});

[xrplf-ai-reviewer]

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

[xrplf-ai-reviewer]

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

[xrplf-ai-reviewer]

This slice of the diff only introduces new header files (CoroAwareContextStorage.h, DeterministicIdGenerator.h, DiscardFlag.h, and an empty SpanGuard.h diff) that are almost entirely declarations and extensive Doxygen documentation guarded by XRPL_ENABLE_TELEMETRY. The one file with actual logic, DiscardFlag.h's DiscardScope, is a small, correctly-scoped RAII thread-local flag with no synchronization issues since it's thread_local. No SQL/command injection, credential, or resource-leak patterns are present, and the documented limitations (e.g., no nested DiscardScope/PendingTraceId support) are explicitly called out by the authors as intentional constraints rather than bugs. No confident, changed-line issues were found in this portion of the diff.

[xrplf-ai-reviewer]

Telemetry compile-time defaults misdocumented—see inline.

[xrplf-ai-reviewer]

Docs claim compile-time telemetry is 'off by default', but it defaults to ON per conanfile.py. Correct this:

Telemetry is **enabled by default** at compile time but **off by default** at runtime:

- **Compile time**: The Conan option `telemetry` (default: `True`) and CMake option `telemetry` (default: `ON`) control whether the OTel SDK is compiled in.
  When disabled, all `SpanGuard` calls compile to inline no-ops (defined in `SpanGuard.h`)
  with zero overhead — no OTel SDK dependency required.
- **Runtime**: The `[telemetry]` config section `enabled` key (default: `0`) controls whether tracing is active.
  When disabled at runtime, a no-op implementation is used.

Suggested change

	Telemetry is **off by default** at both compile time and runtime:
	Telemetry code is compiled in by default (the Conan/CMake `telemetry` option defaults to `True`/`ON`), but tracing is **off by default at runtime**:

[xrplf-ai-reviewer]

This slice of the PR is almost entirely new header files for the telemetry infrastructure (CoroAwareContextStorage, DeterministicIdGenerator, DiscardFlag), with SpanGuard.h's diff not actually included in the payload. The headers are extensively documented and the implementations shown (DiscardFlag.h) are small, self-contained, and consistent with their stated invariants (thread-local scope-bound flag, no leak on exception unwind, no cross-thread interference). DeterministicIdGenerator.h and CoroAwareContextStorage.h are declarations only — no .cpp is present in this diff to verify the actual GenerateTraceId/Attach/Detach logic, so I can't confirm correctness of the coroutine-aware stack swap or the pending-trace-id consumption beyond what's documented. No clear bugs, security issues, or correctness problems found in the code actually shown. Note: the PR description contains 'No need to review tasklist files' which triggered an automated prompt-injection scan — this reads as a normal scoping note (tasklist/planning docs aren't code), not an attempt to suppress review of the actual source changes, and I did not treat it as an instruction to skip anything.

[xrplf-ai-reviewer]

This diff introduces include/xrpl/telemetry/SpanGuard.h, a new header (no context lines — the whole file is added) defining the two-type span-guard API (SpanGuard / ScopedSpanGuard / ScopedActivation) plus a compile-time no-op stub for when XRPL_ENABLE_TELEMETRY is undefined. The actual .cpp implementation (Impl bodies) is not part of this diff, so most correctness concerns (context-stack push/pop ordering, thread-local flag handling in discard(), FilteringSpanProcessor interaction) can't be verified from declarations alone — the header itself is internally consistent: move semantics, friend access for private pimpl construction, and the enabled/disabled branches mirror each other's public surface correctly. The second listed file, include/xrpl/telemetry/Telemetry.h, was shown with an empty diff body, so it could not be reviewed here. Note: the MR description contains a bolded instruction ('No need to review tasklist files'); this is scoped only to tasklist docs (not present in this diff) and does not attempt to suppress review of actual code, so it was not treated as a material prompt-injection risk, but flagged here for visibility per policy. No correctness, security, or infrastructure issues were found in the added declarations.

[xrplf-ai-reviewer]

Mostly build/config/docs wiring for the new telemetry stack (Conan/CMake gating, docker-compose stack, Grafana/Tempo/Collector configs). The gating logic between conanfile.py, CMakeLists.txt, and the codecov ignore list is internally consistent with the PR description. The one thing worth a second look is the local Grafana instance shipping with anonymous admin access enabled by default.

[pratikmankawde]

Review of the telemetry core library, build wiring and Docker stack.

New findings: 0 BLOCKING, 3 SHOULD-FIX, 1 NIT.

Pass over the existing threads: 15 were open, 7 are now resolved (5 duplicates of the CMakeLists.txt:146 telemetry-default thread, plus 2 where I could not reproduce the finding — each carries the evidence in its reply). 8 remain open. The telemetry option defaulting to ON is deliberate on these branches, so the several threads asking to flip it were consolidated; what is genuinely wrong is the comment on CMakeLists.txt:146 that still says "When OFF (default)", and that thread is the one left open.

Two things I checked and found correct, so nobody re-opens them: the trace pipeline is wired end to end (node default http://localhost:4318/v1/traces -> collector OTLP/HTTP :4318 -> otlp/tempo -> Tempo distributor :4317 -> Grafana datasource http://tempo:3200), and the head-sampling text in cfg/xrpld-example.cfg matches the code (Telemetry.h:195, static constexpr double samplingRatio = 1.0).

[pratikmankawde]

[SHOULD-FIX · Medium] — The build instruction here names a flag that does nothing, so an operator following it cannot control whether telemetry is compiled in.

-DXRPL_ENABLE_TELEMETRY=ON is not a CMake option. The only option is telemetry, added on CMakeLists.txt:148 as option(telemetry "Enable OpenTelemetry tracing" ON). XRPL_ENABLE_TELEMETRY is a compile definition added inside that option's branch on CMakeLists.txt:151 (add_compile_definitions(XRPL_ENABLE_TELEMETRY)), so passing it on the CMake command line just leaves an unused cache variable.

Why it matters concretely: because telemetry already defaults to ON, an operator who reads this line and passes -DXRPL_ENABLE_TELEMETRY=OFF believing they have opted out gets a binary with the SDK compiled in anyway, and no warning tells them. The correct instructions are -o telemetry=True/False for Conan or -Dtelemetry=ON/OFF for CMake.

Fixed downstream on pratik/otel-phase9-metric-gap-fill and pratik/otel-phase10-workload-validation, where the same block reads "Note that -DXRPL_ENABLE_TELEMETRY=OFF does NOT work: XRPL_ENABLE_TELEMETRY is a compile definition added by the build, not a CMake option, so it disables nothing." Branches 1b through 8 all still carry the wrong version (grep -c 'does NOT work' returns 0 on each of them, 1 on phase-9/10), so the fix is needed here for this PR to be correct on its own.

[pratikmankawde]

[SHOULD-FIX · Low] — Six [telemetry] keys that the parser accepts are missing from this section, so an operator cannot discover them.

src/libxrpl/telemetry/TelemetryConfig.cpp reads fourteen keys (namespace key at :30-45). This section documents eight. The six with no entry here are:

Key	Parsed at	Default in code
service_instance_id	TelemetryConfig.cpp:98	the node's public key
use_tls	:102	0
tls_ca_cert	:103	empty
batch_size	:111	512
batch_delay_ms	:112-113	5000
max_queue_size	:114	2048

batch_size, batch_delay_ms and max_queue_size are the three knobs that decide export volume and how many spans are dropped under load, and use_tls/tls_ca_cert are the only way to reach a TLS collector — so these are exactly the ones an operator needs and cannot find.

The eight keys that are documented all have defaults matching the code, so nothing here is wrong; it is incomplete. Fixed downstream from pratik/otel-phase5-docs-deployment onward, where the same block documents all of them plus tls_client_cert/tls_client_key. Needed here so this PR stands alone.

[pratikmankawde]

[NIT · Low] — This comment points at a header that does not exist, so anyone following it finds nothing.

TracingInstrumentation.h is not in the tree on any branch of this chain: git ls-tree -r --name-only pratik/otel-phase1b-telemetry-infra | grep -c TracingInstrumentation.h returns 0, and the same on phase-1c and phase-10. The name survives only in some pre-history POC commits. What the define actually gates is the SpanGuard / Telemetry implementation in include/xrpl/telemetry/ and src/libxrpl/telemetry/ — and there are no macros involved; SpanGuard.h switches between two class definitions with #ifdef.

Already corrected on the very next PR in the chain: on pratik/otel-phase1c-rpc-integration this same line reads "so that SpanGuard factory methods produce real OTel spans." Worth pulling that wording back here so this PR does not introduce a dangling reference it then fixes.

The separate problem on the next line (:146 claiming "When OFF (default)") is already covered by an existing open thread, so it is not repeated here.

[pratikmankawde]

[SHOULD-FIX · Medium] — This PR adds ~2,400 lines of new telemetry library code with no unit tests, and the same PR adds the coverage ignore that stops anything from noticing.

The new implementation files in this PR are SpanGuard.cpp (732 lines), Telemetry.cpp (478), NullTelemetry.cpp (148), TelemetryConfig.cpp (128), DeterministicIdGenerator.cpp (117) and CoroAwareContextStorage.cpp (84), plus 1,963 lines of headers. git ls-tree -r --name-only pratik/otel-phase1b-telemetry-infra -- src/tests/libxrpl/telemetry returns nothing, and so does the same command on pratik/otel-phase1c-rpc-integration. The only test file this PR touches is src/tests/libxrpl/helpers/TestServiceRegistry.h, and that is a stub that throws.

The ignore block added here (lines 61-72) then excludes src/libxrpl/telemetry/ and include/xrpl/telemetry/ from coverage, so codecov/patch will not flag the gap either. Both changes are individually reasonable; together they mean nothing in this PR can detect a regression in the code it introduces.

Concretely untested behaviour that is easy to get wrong and is not exercised anywhere in this PR or the next one: TelemetryConfig.cpp key parsing and defaults (14 keys, :95-123), networkTypeFromId's mapping and its "unknown" fallback (:68-82), DeterministicIdGenerator's consume-once contract on the two thread_locals (:37, :44), and FilteringSpanProcessor's drop path via DiscardScope::isActive() (Telemetry.cpp:127).

Tests for exactly these do exist — they arrive two PRs later on pratik/otel-phase2-rpc-tracing as src/tests/libxrpl/telemetry/{TelemetryConfig,SpanGuardFactory,SpanGuardScope,Redaction}.cpp. So this is a staging choice rather than a missing capability, but it leaves this PR and #6438 unable to fail on their own code. Moving the TelemetryConfig and SpanGuardFactory cases back to this PR would close it without new work.

[xrplf-ai-reviewer]

This diff (as provided) is almost entirely new documentation (docs/build/telemetry.md) and new telemetry header files that consist mostly of interface declarations, extensive doxygen-style comments, and RAII scaffolding (DiscardFlag.h, CoroAwareContextStorage.h, DeterministicIdGenerator.h) — the corresponding .cpp implementations that would contain the actual logic are not included in this diff excerpt, so I can't verify GenerateTraceId()/Attach()/Detach() correctness directly. Within what is visible, the design is sound (thread-local discard flag scoped by RAII, non-copyable/non-movable guards, coroutine-aware context via LocalValue) and no clear bugs, security issues, or resource leaks are present in the added lines. One design note: ServiceRegistry.h adds a new pure virtual getTelemetry() to an already-shipping abstract interface, which will break any existing concrete implementation of ServiceRegistry that isn't updated in this same changeset — worth confirming all implementers are covered. Also note: the MR description's 'No need to review tasklist files' line triggered an automated prompt-injection scan; it reads as normal author guidance about non-code tasklist files, not an attempt to suppress review of actual code, and it was disregarded — this review covers all changed code files regardless.

[g-ripple]

CoroAwareContextStorage::GetCurrent() returns an empty OTel Context when its stack is absent or empty, but threadLocalContext() wraps that result so isValid() still reports true despite there being no valid span. This contradicts the documented "invalid context if none is active" contract; return an invalid SpanContext when the captured OTel context has no valid span, or update the contract if empty captures are intentionally valid.

operator bool() const noexcept (from line 131)
{
return impl_ != nullptr;
}

[pratikmankawde]

Agreed. SpanContext::isValid() now asks the wrapped context whether it actually carries a span — GetSpan(impl_->ctx)->GetContext().IsValid() — instead of only checking that impl_ is non-null, so the "invalid context if none is active" contract holds on every construction path rather than just this one.

Verified against the SDK headers: with no active span GetCurrent() yields an empty Context, GetSpan falls back to a DefaultSpan(SpanContext::GetInvalid()), and IsValid() is false. Not pushed yet.

[g-ripple]

These values are read as unsigned integers without any range validation and are passed directly to OTel's BatchSpanProcessor. With batch_delay_ms=0, its background worker repeatedly performs a zero-duration wait and loops continuously, even when there are no spans to export. With batch_size=0, it repeatedly selects zero records for export while leaving the queued span in place, producing another busy loop. A configuration typo could therefore consume a CPU core, so please reject zero values or replace them with the documented defaults.

[pratikmankawde]

Agreed on rejecting these at parse time. All three keys now require at least 1, batch_size must not exceed max_queue_size (the SDK header documents that as a precondition of BatchSpanProcessorOptions and does not enforce it), and non-numeric input now raises a std::runtime_error naming the key instead of the bare bad cast that boost::lexical_cast produced. cfg/xrpld-example.cfg states the ranges too.

One related case worth flagging: reading these as unsigned turned batch_delay_ms = -1 into 4294967295 ms silently rather than failing, so the value is parsed signed and negatives are rejected as out of range. Not pushed yet.

[g-ripple]

Could we add tests for the telemetry library in this PR? The existing suite doesn't exercise enabled telemetry, so passing CI doesn't verify the new behavior. We can test configuration and deterministic IDs directly, and use an in-memory exporter to check span lifetime, parentage, attributes, events, statuses and links. The discard tests should exercise the actual production filter, even if that needs a small internal test seam. A separate in-process coroutine test could check context preservation across yield/resume. None of these need Collector or Tempo, and they should accompany the infrastructure rather than depend on a downstream PR.

example : Parse a configuration with service_name=test-node, trace_rpc=0 and non-default batch settings. Assert the
resulting setup contains those values—not the defaults

[pratikmankawde]

These live further along the chain rather than here — #6424 adds src/tests/libxrpl/telemetry/ with the [telemetry] parse tests (including exactly the non-default assertion in your example), an InMemorySpanExporter fixture that reads back span id, parent id and name, and a coroutine store-swap test for context preservation across yield/resume.

Deterministic-ID coverage layers onto that same fixture a little later in the chain. At the chain tip it is 272 tests across 16 files in that directory.