core review comments

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>

include/xrpl/telemetry/DiscardFlag.h

@@ -1,25 +1,86 @@
 #pragma once
 
-/** Thread-local flag for span discard signaling.
+/** Thread-local discard signaling between SpanGuard and the span processor.
 
-    SpanGuard::discard() sets gTlDiscardCurrentSpan to true before calling
-    Span::End(). The OTel SDK calls SpanProcessor::OnEnd() synchronously on
-    the same thread, so FilteringSpanProcessor checks and clears this flag
-    in OnEnd() to drop the span before it enters the batch export queue.
+    SpanGuard::discard() wants to drop a span without sending it to the
+    exporter. The OTel SDK calls SpanProcessor::OnEnd() synchronously on the
+    same thread that calls Span::End(), so a thread-local flag set just before
+    End() and read inside OnEnd() lets FilteringSpanProcessor drop the span
+    before it enters the batch export queue.
 
-    This side-channel avoids inspecting the Recordable's internals (which
-    vary by exporter type — SpanData vs OtlpRecordable).
+    This side-channel avoids inspecting the Recordable's internals (which vary
+    by exporter type — SpanData vs OtlpRecordable).
+
+    The raw flag lives in `detail` and is mutated only through DiscardScope, a
+    RAII guard that sets it on construction and clears it on destruction. This
+    keeps the set/clear lifetime bound to a scope (so the flag cannot leak onto
+    the next span even if End() were to throw) and prevents any class which includes this header
+    from flipping the flag directly. FilteringSpanProcessor reads it through
+    isDiscardingCurrentSpan().
 
     Kept in a separate header to avoid transitive include bloat: SpanGuard.h
-    only needs this flag, not the full Telemetry.h with BasicConfig/Journal.
+    only needs this signaling, not the full Telemetry.h with BasicConfig/Journal.
+
+    Usage:
+    @code
+        // In SpanGuard::discard():
+        {
+            DiscardScope discardScope;  // flag set for this scope only
+            span->End();                // OnEnd() runs synchronously, sees flag
+        }                               // flag cleared here, unconditionally
+    @endcode
+
+    @note Thread safety: the flag is thread-local, so each thread observes only
+    its own discard signal — no synchronization is required.
 
     @see SpanGuard::discard(), FilteringSpanProcessor (Telemetry.cpp)
 */
 
 namespace xrpl::telemetry {
 
-/** When true, the FilteringSpanProcessor drops the current span in
-    OnEnd(). Set by SpanGuard::discard(), cleared by OnEnd(). */
+namespace detail {
+
+/** Internal thread-local discard flag. Mutate only via DiscardScope; read
+    only via isDiscardingCurrentSpan(). Not intended for direct use. */
 inline thread_local bool gTlDiscardCurrentSpan = false;
 
+}  // namespace detail
+
+/** RAII guard that marks the current thread's span for discard.
+
+    Sets the thread-local discard flag on construction and clears it on
+    destruction, so a span ended within the guard's scope is dropped by
+    FilteringSpanProcessor::OnEnd() while the flag stays confined to that scope.
+    Non-copyable and non-movable — its sole purpose is the scoped flag lifetime.
+*/
+class DiscardScope
+{
+public:
+    DiscardScope() noexcept
+    {
+        detail::gTlDiscardCurrentSpan = true;
+    }
+
+    ~DiscardScope()
+    {
+        detail::gTlDiscardCurrentSpan = false;
+    }
+
+    DiscardScope(DiscardScope const&) = delete;
+    DiscardScope&
+    operator=(DiscardScope const&) = delete;
+    DiscardScope(DiscardScope&&) = delete;
+    DiscardScope&
+    operator=(DiscardScope&&) = delete;
+};
+
+/** @return true if the current thread is inside a DiscardScope, i.e. the span
+    ending now should be dropped rather than exported. Read by
+    FilteringSpanProcessor::OnEnd(). */
+[[nodiscard]] inline bool
+isDiscardingCurrentSpan() noexcept
+{
+    return detail::gTlDiscardCurrentSpan;
+}
+
 }  // namespace xrpl::telemetry

include/xrpl/telemetry/Telemetry.h

@@ -88,6 +88,13 @@
 
 namespace xrpl::telemetry {
 
+#ifdef XRPL_ENABLE_TELEMETRY
+/** OTel instrumentation scope (tracer) name. Identifies this library as the
+    source of spans; distinct from the `service.name` resource attribute
+    (Setup::serviceName), which is config-overridable. */
+inline constexpr std::string_view kTracerName{"xrpld"};
+#endif
+
 class Telemetry
 {
     /** Global singleton pointer, set by start()/stop() in the active
@@ -165,7 +172,7 @@ public:
         std::uint32_t batchSize = 512;
 
         /** Delay between batch exports. */
-        std::chrono::milliseconds batchDelay{5000};
+        std::chrono::milliseconds batchDelay = std::chrono::milliseconds{5000};
 
         /** Maximum number of spans queued before dropping. */
         std::uint32_t maxQueueSize = 2048;
@@ -255,7 +262,7 @@ public:
         @return A shared pointer to the Tracer.
     */
     virtual opentelemetry::nostd::shared_ptr<opentelemetry::trace::Tracer>
-    getTracer(std::string_view name = "xrpld") = 0;
+    getTracer(std::string_view name = kTracerName) = 0;
 
     /** Start a new span on the current thread's context.
 

src/libxrpl/telemetry/NullTelemetry.cpp

@@ -41,7 +41,7 @@ class NullTelemetry : public Telemetry
     Setup const setup_;
 
 public:
-    explicit NullTelemetry(Setup setup) : setup_(std::move(setup))
+    explicit NullTelemetry(Setup setup) : setup_{std::move(setup)}
     {
     }
 

src/libxrpl/telemetry/SpanGuard.cpp

@@ -28,6 +28,7 @@
 #include <opentelemetry/context/context.h>
 #include <opentelemetry/context/runtime_context.h>
 #include <opentelemetry/nostd/shared_ptr.h>
+#include <opentelemetry/semconv/exception_attributes.h>
 #include <opentelemetry/trace/context.h>
 #include <opentelemetry/trace/scope.h>
 #include <opentelemetry/trace/span.h>
@@ -36,6 +37,7 @@
 
 #include <cstdint>
 #include <exception>
+#include <format>
 #include <memory>
 #include <string>
 #include <string_view>
@@ -138,6 +140,13 @@ isCategoryEnabled(Telemetry const& tel, TraceCategory cat)
 
 namespace {
 
+// Span-link attribute marking a "follows-from" (causal, non-parent) link,
+// emitted by both linkedSpan() overloads. Custom xrpl attribute — not part
+// of the OTel semantic conventions, so defined here rather than pulled from
+// <opentelemetry/semconv/...>.
+constexpr char const* kLinkTypeKey = "xrpl.link.type";
+constexpr char const* kLinkTypeFollowsFrom = "follows_from";
+
 // Map a TraceCategory to an OTel SpanKind so Tempo's service-graph /
 // RED metrics see the correct direction. RPC spans are emitted at the
 // server entry point (handler dispatch), Peer spans at inbound-message
@@ -168,7 +177,7 @@ SpanGuard::span(TraceCategory cat, std::string_view prefix, std::string_view nam
     auto* tel = Telemetry::getInstance();
     if ((tel == nullptr) || !tel->isEnabled() || !isCategoryEnabled(*tel, cat))
         return {};
-    auto fullName = std::string(prefix) + "." + std::string(name);
+    auto fullName = std::format("{}.{}", prefix, name);
     return SpanGuard(std::make_unique<Impl>(tel->startSpan(fullName, categoryToSpanKind(cat))));
 }
 
@@ -206,19 +215,17 @@ SpanGuard::linkedSpan(std::string_view name) const
     if ((tel == nullptr) || !tel->isEnabled())
         return {};
 
-    auto tracer = tel->getTracer("xrpld");
+    auto tracer = tel->getTracer();
     auto spanCtx = impl_->span->GetContext();
 
     // Mark as root span so it starts a new trace sub-tree rather than
     // inheriting the current thread's active span as parent.
     otel_trace::StartSpanOptions opts;
-    opentelemetry::context::Context rootCtx;
-    rootCtx = rootCtx.SetValue(otel_trace::kIsRootSpanKey, true);
-    opts.parent = rootCtx;
+    opts.parent = opentelemetry::context::Context{otel_trace::kIsRootSpanKey, true};
 
     return SpanGuard(
         std::make_unique<Impl>(tracer->StartSpan(
-            std::string(name), {}, {{spanCtx, {{"xrpl.link.type", "follows_from"}}}}, opts)));
+            std::string(name), {}, {{spanCtx, {{kLinkTypeKey, kLinkTypeFollowsFrom}}}}, opts)));
 }
 
 SpanGuard
@@ -230,7 +237,7 @@ SpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx)
     if ((tel == nullptr) || !tel->isEnabled())
         return {};
 
-    auto tracer = tel->getTracer("xrpld");
+    auto tracer = tel->getTracer();
 
     // Extract the span from the captured context to get its SpanContext.
     auto linkSpan = otel_trace::GetSpan(linkCtx.impl_->ctx);
@@ -240,15 +247,13 @@ SpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx)
     // Mark as root span so it starts a new trace sub-tree rather than
     // inheriting the current thread's active span as parent.
     otel_trace::StartSpanOptions opts;
-    opentelemetry::context::Context rootCtx;
-    rootCtx = rootCtx.SetValue(otel_trace::kIsRootSpanKey, true);
-    opts.parent = rootCtx;
+    opts.parent = opentelemetry::context::Context{otel_trace::kIsRootSpanKey, true};
 
     return SpanGuard(
         std::make_unique<Impl>(tracer->StartSpan(
             std::string(name),
             {},
-            {{linkSpan->GetContext(), {{"xrpl.link.type", "follows_from"}}}},
+            {{linkSpan->GetContext(), {{kLinkTypeKey, kLinkTypeFollowsFrom}}}},
             opts)));
 }
 
@@ -260,7 +265,7 @@ SpanGuard::captureContext() const
     if (!impl_)
         return {};
     auto ctx = opentelemetry::context::RuntimeContext::GetCurrent();
-    return SpanContext(std::make_shared<SpanContext::Impl>(ctx));
+    return SpanContext(std::make_shared<SpanContext::Impl>(std::move(ctx)));
 }
 
 // ===== Attribute setters ===================================================
@@ -331,9 +336,13 @@ SpanGuard::recordException(std::exception const& e)
 {
     if (!impl_)
         return;
+    namespace semconv_exc = opentelemetry::semconv::exception;
+    // Event name "exception" and the attribute keys follow the OTel semantic
+    // conventions; the keys come from semconv constants rather than literals.
     impl_->span->AddEvent(
         "exception",
-        {{"exception.type", typeid(e).name()}, {"exception.message", std::string(e.what())}});
+        {{semconv_exc::kExceptionType, typeid(e).name()},
+         {semconv_exc::kExceptionMessage, std::string(e.what())}});
     impl_->span->SetStatus(otel_trace::StatusCode::kError, e.what());
 }
 
@@ -342,16 +351,19 @@ SpanGuard::discard()
 {
     if (impl_)
     {
-        gTlDiscardCurrentSpan = true;
-        impl_->span->End();
-        // Clear here so discard() owns the flag's whole lifetime
-        // (set -> End -> clear) in one scope, rather than relying on
-        // FilteringSpanProcessor::OnEnd() to clear it. Today every valid guard
-        // wraps a recording span (head sampling is 1.0), so OnEnd() always runs
-        // and clearing here is equivalent — but colocating set and clear keeps
-        // the flag leak-proof if a later phase can hand back a non-recording
-        // span (e.g. honoring a non-sampled remote parent during propagation).
-        gTlDiscardCurrentSpan = false;
+        {
+            // DiscardScope owns the flag's whole lifetime: it sets the flag,
+            // and clears it on scope exit — even if End() were to throw. The
+            // SDK invokes FilteringSpanProcessor::OnEnd() synchronously from
+            // End() on this thread, so the flag is observed while still set.
+            // Today every valid guard wraps a recording span (head sampling is
+            // 1.0), so OnEnd() always runs — but scoping set/clear keeps the
+            // flag leak-proof if a later phase can hand back a non-recording
+            // span (e.g. honoring a non-sampled remote parent during
+            // propagation), so it can never spill onto the next span.
+            DiscardScope discardScope;
+            impl_->span->End();
+        }
         impl_->span = nullptr;  // prevent ~Impl from calling End() again
         impl_.reset();
     }

src/libxrpl/telemetry/Telemetry.cpp

@@ -62,9 +62,10 @@ namespace resource = opentelemetry::sdk::resource;
 /** SpanProcessor decorator that drops discarded spans.
 
     Wraps a delegate processor (typically BatchSpanProcessor). In OnEnd(),
-    checks the gTlDiscardCurrentSpan thread-local flag. If set (by
-    SpanGuard::discard()), the span is silently dropped — never entering
-    the batch queue, never sent over the network, never stored.
+    calls isDiscardingCurrentSpan(). If the calling thread is inside a
+    DiscardScope (entered by SpanGuard::discard()), the span is silently
+    dropped — never entering the batch queue, never sent over the network,
+    never stored.
 
     Uses a thread-local flag rather than inspecting Recordable attributes
     because the Recordable type varies by exporter (SpanData for simple
@@ -88,7 +89,7 @@ namespace resource = opentelemetry::sdk::resource;
           +---------------------+
 
     @note Thread safety: OnEnd() may be called concurrently from multiple
-    threads. The gTlDiscardCurrentSpan flag is thread-local, so each
+    threads. The discard flag behind DiscardScope is thread-local, so each
     thread's discard state is independent — no synchronization needed.
 */
 class FilteringSpanProcessor : public trace_sdk::SpanProcessor
@@ -118,11 +119,11 @@ public:
     void
     OnEnd(std::unique_ptr<trace_sdk::Recordable>&& span) noexcept override
     {
-        if (gTlDiscardCurrentSpan)
+        if (isDiscardingCurrentSpan())
         {
-            // SpanGuard::discard() set the flag on this thread just before
-            // calling Span::End(), which invokes OnEnd() synchronously.
-            // Drop the span.
+            // SpanGuard::discard() is inside a DiscardScope on this thread,
+            // which it entered just before calling Span::End() — and End()
+            // invokes OnEnd() synchronously. Drop the span.
             return;
         }
         delegate_->OnEnd(std::move(span));
@@ -389,7 +390,7 @@ public:
     }
 
     opentelemetry::nostd::shared_ptr<trace_api::Tracer>
-    getTracer(std::string_view name) override
+    getTracer(std::string_view name = kTracerName) override
     {
         if (!sdkProvider_)
             return trace_api::Provider::GetTracerProvider()->GetTracer(std::string(name));
@@ -399,7 +400,7 @@ public:
     opentelemetry::nostd::shared_ptr<trace_api::Span>
     startSpan(std::string_view name, trace_api::SpanKind kind) override
     {
-        auto tracer = getTracer("xrpld");
+        auto tracer = getTracer();
         trace_api::StartSpanOptions opts;
         opts.kind = kind;
         return tracer->StartSpan(std::string(name), opts);
@@ -411,7 +412,7 @@ public:
         opentelemetry::context::Context const& parentContext,
         trace_api::SpanKind kind) override
     {
-        auto tracer = getTracer("xrpld");
+        auto tracer = getTracer();
         trace_api::StartSpanOptions opts;
         opts.kind = kind;
         opts.parent = parentContext;