fix(telemetry): address Phase 1b code review findings

- SpanContext::isValid(): add inline no-op when XRPL_ENABLE_TELEMETRY
  is not defined, preventing a linker error if called in that path
- linkedSpan(): set kIsRootSpanKey on the StartSpanOptions parent
  context so linked spans start a genuinely independent sub-tree
  instead of silently becoming children of the current active span
- Telemetry::instance_: use std::atomic with acquire/release ordering
  to avoid a data race between start()/stop() and factory methods
  called from worker threads

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Pratik Mankawde
2026-04-17 17:15:36 +01:00
parent 714ac49f47
commit ad233846fa
3 changed files with 30 additions and 6 deletions

View File

@@ -139,8 +139,16 @@ public:
SpanContext() = default;
/** @return true if this context holds a valid trace context. */
#ifdef XRPL_ENABLE_TELEMETRY
bool
isValid() const;
#else
bool
isValid() const
{
return false;
}
#endif
};
// ---------------------------------------------------------------------------

View File

@@ -72,6 +72,7 @@
#include <xrpl/basics/BasicConfig.h>
#include <xrpl/beast/utility/Journal.h>
#include <atomic>
#include <chrono>
#include <memory>
#include <string>
@@ -92,9 +93,12 @@ class Telemetry
/** Global singleton pointer, set by start()/stop() in the active
implementation. Allows SpanGuard factory methods to access the
Telemetry instance without callers passing it explicitly.
Atomic with acquire/release ordering: start()/stop() store on
the initialization thread, factory methods load on worker threads.
@see setInstance(), getInstance()
*/
inline static Telemetry* instance_ = nullptr;
inline static std::atomic<Telemetry*> instance_{nullptr};
public:
/** Get the global Telemetry instance.
@@ -103,7 +107,7 @@ public:
static Telemetry*
getInstance()
{
return instance_;
return instance_.load(std::memory_order_acquire);
}
/** Set the global Telemetry instance.
@@ -114,7 +118,7 @@ public:
static void
setInstance(Telemetry* t)
{
instance_ = t;
instance_.store(t, std::memory_order_release);
}
/** Configuration parsed from the [telemetry] section of xrpld.cfg.

View File

@@ -200,7 +200,13 @@ SpanGuard::linkedSpan(std::string_view name) const
auto tracer = tel->getTracer("xrpld");
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;
return SpanGuard(
std::make_unique<Impl>(tracer->StartSpan(
std::string(name), {}, {{spanCtx, {{"xrpl.link.type", "follows_from"}}}}, opts)));
@@ -218,16 +224,22 @@ SpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx)
auto tracer = tel->getTracer("xrpld");
// Extract the span from the captured context to get its SpanContext.
auto parentSpan = otel_trace::GetSpan(linkCtx.impl_->ctx);
if (!parentSpan || !parentSpan->GetContext().IsValid())
auto linkSpan = otel_trace::GetSpan(linkCtx.impl_->ctx);
if (!linkSpan || !linkSpan->GetContext().IsValid())
return {};
// 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;
return SpanGuard(
std::make_unique<Impl>(tracer->StartSpan(
std::string(name),
{},
{{parentSpan->GetContext(), {{"xrpl.link.type", "follows_from"}}}},
{{linkSpan->GetContext(), {{"xrpl.link.type", "follows_from"}}}},
opts)));
}