From cc4457b5a2b47e05bde18979a2e05b5d55df750e Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 7 Jul 2026 14:27:01 +0100 Subject: [PATCH] fix(telemetry): address phase-7 review comments - cmake: keep the opentelemetry-cpp umbrella target for the beast metrics link and document why. The reviewer suggested linking individual component targets to avoid over-linking, but the OTel Conan package under-declares inter-component dependencies (the OTLP client references sdk::common symbols without a declared edge), so naming components directly reorders the static link into an unresolvable state. Verified by building xrpl_tests both ways. - OTelCollector.h: add usage examples, thread-safety and limitations @note blocks to the class doc. - OTelCollector.cpp: correct the @param name docs on the instrument Impl constructors to describe the already-formatName()'d value. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmake/XrplCore.cmake | 8 ++++- include/xrpl/beast/insight/OTelCollector.h | 34 +++++++++++++++++++++ src/libxrpl/beast/insight/OTelCollector.cpp | 16 +++++++--- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/cmake/XrplCore.cmake b/cmake/XrplCore.cmake index fa54ca7255..a6d004ad07 100644 --- a/cmake/XrplCore.cmake +++ b/cmake/XrplCore.cmake @@ -78,7 +78,13 @@ include(target_link_modules) # Level 01 add_module(xrpl beast) target_link_libraries(xrpl.libxrpl.beast PUBLIC xrpl.imports.main) -# OTelCollector in beast/insight uses OTel Metrics SDK when telemetry is enabled. +# OTelCollector in beast/insight uses the OTel Metrics SDK when telemetry is +# enabled. Link the Conan-provided umbrella target rather than individual +# component targets: the OTel package's per-component dependency graph is +# under-declared (e.g. the OTLP client references sdk::common symbols without +# declaring the edge), so naming components directly reorders the static-link +# line into an unresolvable state. The umbrella carries the full, internally +# consistent graph the package authors validated. if(telemetry) target_link_libraries( xrpl.libxrpl.beast diff --git a/include/xrpl/beast/insight/OTelCollector.h b/include/xrpl/beast/insight/OTelCollector.h index f3387614a8..e043531f99 100644 --- a/include/xrpl/beast/insight/OTelCollector.h +++ b/include/xrpl/beast/insight/OTelCollector.h @@ -54,6 +54,40 @@ namespace beast::insight { * - Meter -> OTel Counter (monotonic, unsigned) * - Hook -> Called by PeriodicMetricReader at collection time * + * Example — primary use (create the collector and record a metric): + * @code + * auto collector = beast::insight::OTelCollector::New( + * "http://localhost:4318/v1/metrics", // OTLP/HTTP endpoint + * "xrpld", // metric name prefix + * "node-1", // service.instance.id + * "xrpld", // service.name + * "mainnet", // xrpl.network.type + * journal); + * + * auto counter = collector->makeCounter("ledgers", "closed"); + * ++counter; // exported on the next PeriodicMetricReader tick + * @endcode + * + * Example — edge case (telemetry disabled at compile time): New() + * returns a NullCollector, so callers need no #ifdef guard. The same + * instruments compile and run, but recording is a no-op. + * @code + * auto collector = beast::insight::OTelCollector::New( + * endpoint, prefix, instanceId, serviceName, networkType, journal); + * auto gauge = collector->makeGauge("peers", "count"); + * gauge = 42; // silently discarded when XRPL_ENABLE_TELEMETRY is off + * @endcode + * + * @note Thread safety: instrument recording (Counter::Add, + * Histogram::Record, and atomic gauge writes) is thread-safe and + * may be called concurrently. Instrument *creation* (make_counter, + * make_gauge, etc.) is serialized by an internal mutex. Hook and + * observable-gauge callbacks run on the SDK's collection thread, so + * any state they read must itself be thread-safe. + * @note Limitations: metrics export over OTLP/HTTP only (no gRPC); the + * PeriodicMetricReader interval is fixed at 1s; and gauge values + * are stored as int64_t, so fractional gauges are truncated. + * * @see StatsDCollector for the StatsD-based alternative. * @see NullCollector for the no-op fallback. */ diff --git a/src/libxrpl/beast/insight/OTelCollector.cpp b/src/libxrpl/beast/insight/OTelCollector.cpp index 9893fbedef..62bab9526e 100644 --- a/src/libxrpl/beast/insight/OTelCollector.cpp +++ b/src/libxrpl/beast/insight/OTelCollector.cpp @@ -142,7 +142,9 @@ class OTelCounterImpl : public CounterImpl { public: /** - * @param name Fully-qualified metric name (prefix.group.name). + * @param name Export-ready metric name, already run through + * formatName() by the collector: prefix prepended and + * dots replaced with underscores (e.g. "xrpld_rpc_size"). * @param meter OTel Meter used to create the counter instrument. */ OTelCounterImpl( @@ -182,7 +184,9 @@ class OTelEventImpl : public EventImpl { public: /** - * @param name Fully-qualified metric name (prefix.group.name). + * @param name Export-ready metric name, already run through + * formatName() by the collector: prefix prepended and + * dots replaced with underscores (e.g. "xrpld_rpc_size"). * @param meter OTel Meter used to create the histogram instrument. */ OTelEventImpl( @@ -227,7 +231,9 @@ class OTelGaugeImpl : public GaugeImpl { public: /** - * @param name Fully-qualified metric name (prefix.group.name). + * @param name Export-ready metric name, already run through + * formatName() by the collector: prefix prepended + * and dots replaced with underscores. * @param meter OTel Meter used to create the observable gauge. * @param collector Owning collector, used to invoke hooks before reads. */ @@ -300,7 +306,9 @@ class OTelMeterImpl : public MeterImpl { public: /** - * @param name Fully-qualified metric name (prefix.group.name). + * @param name Export-ready metric name, already run through + * formatName() by the collector: prefix prepended and + * dots replaced with underscores (e.g. "xrpld_rpc_size"). * @param meter OTel Meter used to create the counter instrument. */ OTelMeterImpl(