From 3590becc2395bb054e5aad7bf2ce99cd729d609b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 5 Dec 2024 14:53:53 -0800 Subject: [PATCH] Use a SpanId class to wrap the span id rather than just a uint64_T --- src/workerd/io/trace-test.c++ | 16 ++++----- src/workerd/io/trace.c++ | 32 +++++++++++++----- src/workerd/io/trace.h | 61 ++++++++++++++++++++++++++++++++--- 3 files changed, 88 insertions(+), 21 deletions(-) diff --git a/src/workerd/io/trace-test.c++ b/src/workerd/io/trace-test.c++ index ce76ef92e62..765ca97dc0e 100644 --- a/src/workerd/io/trace-test.c++ +++ b/src/workerd/io/trace-test.c++ @@ -93,7 +93,7 @@ KJ_TEST("InvocationSpanContext") { static constexpr auto kCheck = TraceId(0x2a2a2a2a2a2a2a2a, 0x2a2a2a2a2a2a2a2a); KJ_EXPECT(sc->getTraceId() == kCheck); KJ_EXPECT(sc->getInvocationId() == kCheck); - KJ_EXPECT(sc->getSpanId() == 0); + KJ_EXPECT(sc->getSpanId() == SpanId(1)); // And serialize that to a capnp struct... capnp::MallocMessageBuilder builder; @@ -104,7 +104,7 @@ KJ_TEST("InvocationSpanContext") { auto sc2 = KJ_ASSERT_NONNULL(InvocationSpanContext::fromCapnp(root.asReader())); KJ_EXPECT(sc2->getTraceId() == kCheck); KJ_EXPECT(sc2->getInvocationId() == kCheck); - KJ_EXPECT(sc2->getSpanId() == 0); + KJ_EXPECT(sc2->getSpanId() == SpanId(1)); KJ_EXPECT(sc2->isTrigger()); // The one that has been deserialized from capnp cannot create children... @@ -112,24 +112,24 @@ KJ_TEST("InvocationSpanContext") { sc2->newChild(); KJ_FAIL_ASSERT("should not be able to create child span with SpanContext from capnp"); } catch (kj::Exception& ex) { - // KJ_EXPECT(ex.getDescription() == - // "expected !isTrigger(); unable to create child spans on this context"_kj); + KJ_EXPECT(ex.getDescription() == + "expected !isTrigger(); unable to create child spans on this context"_kj); } auto sc3 = sc->newChild(); KJ_EXPECT(sc3->getTraceId() == kCheck); KJ_EXPECT(sc3->getInvocationId() == kCheck); - KJ_EXPECT(sc3->getSpanId() == 1); + KJ_EXPECT(sc3->getSpanId() == SpanId(2)); - auto sc4 = InvocationSpanContext::newForInvocation(sc2); + auto sc4 = InvocationSpanContext::newForInvocation(sc2, fakeEntropySource); KJ_EXPECT(sc4->getTraceId() == kCheck); KJ_EXPECT(sc4->getInvocationId() == kCheck); - KJ_EXPECT(sc4->getSpanId() == 0); + KJ_EXPECT(sc4->getSpanId() == SpanId(3)); auto& sc5 = KJ_ASSERT_NONNULL(sc4->getParent()); KJ_EXPECT(sc5->getTraceId() == kCheck); KJ_EXPECT(sc5->getInvocationId() == kCheck); - KJ_EXPECT(sc5->getSpanId() == 0); + KJ_EXPECT(sc5->getSpanId() == SpanId(1)); KJ_EXPECT(sc5->isTrigger()); } diff --git a/src/workerd/io/trace.c++ b/src/workerd/io/trace.c++ index 735fee5d270..3186ef7bb54 100644 --- a/src/workerd/io/trace.c++ +++ b/src/workerd/io/trace.c++ @@ -154,6 +154,21 @@ TraceId TraceId::fromEntropy(kj::Maybe entropySource) { return TraceId(getRandom64Bit(entropySource), getRandom64Bit(entropySource)); } +kj::String SpanId::toGoString() const { + kj::Vector s(16); + addHex(s, id); + s.add('\0'); + return kj::String(s.releaseAsArray()); +} + +SpanId SpanId::fromEntropy(kj::Maybe entropySource) { + return SpanId(getRandom64Bit(entropySource)); +} + +kj::String KJ_STRINGIFY(const SpanId& id) { + return id; +} + kj::String KJ_STRINGIFY(const TraceId& id) { return id; } @@ -162,18 +177,18 @@ InvocationSpanContext::InvocationSpanContext(kj::Badge, kj::Maybe entropySource, TraceId traceId, TraceId invocationId, - uint64_t spanId, + SpanId spanId, kj::Maybe> parentSpanContext) : entropySource(entropySource), traceId(kj::mv(traceId)), invocationId(kj::mv(invocationId)), - spanId(spanId), + spanId(kj::mv(spanId)), parentSpanContext(kj::mv(parentSpanContext)) {} kj::Rc InvocationSpanContext::newChild() { KJ_ASSERT(!isTrigger(), "unable to create child spans on this context"); return kj::rc(kj::Badge(), entropySource, traceId, - invocationId, getRandom64Bit(entropySource), addRefToThis()); + invocationId, SpanId::fromEntropy(entropySource), addRefToThis()); } kj::Rc InvocationSpanContext::newForInvocation( @@ -186,7 +201,8 @@ kj::Rc InvocationSpanContext::newForInvocation( return ctx->traceId; }).orDefault([&] { return TraceId::fromEntropy(entropySource); }); return kj::rc(kj::Badge(), entropySource, - kj::mv(traceId), TraceId::fromEntropy(entropySource), 0, kj::mv(parent)); + kj::mv(traceId), TraceId::fromEntropy(entropySource), SpanId::fromEntropy(entropySource), + kj::mv(parent)); } TraceId TraceId::fromCapnp(rpc::InvocationSpanContext::TraceId::Reader reader) { @@ -1315,10 +1331,10 @@ kj::Maybe getParentContextFromSpan( } } // namespace -tracing::TailEvent::Context::Context(TraceId traceId, TraceId invocationId, kj::uint spanId) - : traceId(traceId), - invocationId(invocationId), - spanId(spanId) {} +tracing::TailEvent::Context::Context(TraceId traceId, TraceId invocationId, SpanId spanId) + : traceId(kj::mv(traceId)), + invocationId(kj::mv(invocationId)), + spanId(kj::mv(spanId)) {} tracing::TailEvent::Context::Context(rpc::InvocationSpanContext::Reader reader) : traceId(TraceId::fromCapnp(reader.getTraceId())), diff --git a/src/workerd/io/trace.h b/src/workerd/io/trace.h index ecd77b218e4..0ef231b6188 100644 --- a/src/workerd/io/trace.h +++ b/src/workerd/io/trace.h @@ -120,6 +120,56 @@ class TraceId final { }; constexpr TraceId TraceId::nullId = nullptr; +// A 64-bit span identifier. +class SpanId final { + public: + // A null span ID. This is only acceptable for use in tests. + constexpr SpanId(decltype(nullptr)): id(0) {} + + constexpr SpanId(uint64_t id): id(id) {} + constexpr SpanId(const SpanId& other) = default; + constexpr SpanId& operator=(const SpanId& other) = default; + constexpr SpanId(SpanId&& other): id(other.id) { + other.id = 0; + } + constexpr SpanId& operator=(SpanId&& other) { + id = other.id; + other.id = 0; + return *this; + } + constexpr operator bool() const { + return id != 0; + } + constexpr bool operator==(const SpanId& other) const { + return id == other.id; + } + constexpr bool operator==(decltype(nullptr)) const { + return id == 0; + } + + inline operator kj::String() const { + return toGoString(); + } + + inline operator uint64_t() const { + return id; + } + + kj::String toGoString() const; + + static const SpanId nullId; + + constexpr uint64_t getId() const { + return id; + } + + static SpanId fromEntropy(kj::Maybe entropy = kj::none); + + private: + uint64_t id; +}; +constexpr SpanId SpanId::nullId = nullptr; + // The InvocationSpanContext is a tuple of a trace id, invocation id, and span id. // The trace id represents a top-level request and should be shared across all // invocation spans and events within those spans. The invocation id identifies @@ -135,7 +185,7 @@ class InvocationSpanContext final: public kj::Refcounted, kj::Maybe entropySource, TraceId traceId, TraceId invocationId, - uint64_t spanId = 0ULL, + SpanId spanId, kj::Maybe> parentSpanContext = kj::none); KJ_DISALLOW_COPY_AND_MOVE(InvocationSpanContext); @@ -147,7 +197,7 @@ class InvocationSpanContext final: public kj::Refcounted, return invocationId; } - inline const uint64_t getSpanId() const { + inline const SpanId& getSpanId() const { return spanId; } @@ -189,7 +239,7 @@ class InvocationSpanContext final: public kj::Refcounted, kj::Maybe entropySource; const TraceId traceId; const TraceId invocationId; - const uint64_t spanId; + const SpanId spanId; // The parentSpanContext can be either a direct parent or a trigger // context. If it is a trigger context, then it should have the same @@ -198,6 +248,7 @@ class InvocationSpanContext final: public kj::Refcounted, const kj::Maybe> parentSpanContext; }; +kj::String KJ_STRINGIFY(const SpanId& id); kj::String KJ_STRINGIFY(const TraceId& id); kj::String KJ_STRINGIFY(const kj::Rc& context); @@ -606,14 +657,14 @@ struct TailEvent final { using Event = kj::OneOf; struct Context final { - explicit Context(TraceId traceId, TraceId invocationId, kj::uint spanId); + explicit Context(TraceId traceId, TraceId invocationId, SpanId spanId); Context(rpc::InvocationSpanContext::Reader reader); Context(Context&&) = default; Context& operator=(Context&&) = default; KJ_DISALLOW_COPY(Context); TraceId traceId; TraceId invocationId; - kj::uint spanId; + SpanId spanId; void copyTo(rpc::InvocationSpanContext::Builder builder); Context clone();