From 14479896be71ab7439fab1c915d4d8a09d7c8d1d Mon Sep 17 00:00:00 2001 From: James McMullan Date: Wed, 11 Dec 2024 14:09:58 -0500 Subject: [PATCH] Code review changes --- system/jlib/jtrace.cpp | 23 +++++++------- system/jlib/jtrace.hpp | 53 +++++++++++++++++++-------------- testing/unittests/jlibtests.cpp | 11 +++++++ 3 files changed, 53 insertions(+), 34 deletions(-) diff --git a/system/jlib/jtrace.cpp b/system/jlib/jtrace.cpp index f7ae2941b2e..5a88692b329 100644 --- a/system/jlib/jtrace.cpp +++ b/system/jlib/jtrace.cpp @@ -915,12 +915,13 @@ class CNullSpan final : public CInterfaceOf virtual void recordError(const SpanError & error) override {}; virtual void setSpanStatusSuccess(bool spanSucceeded, const char * statusMessage) override {} - virtual const char * queryTraceId() const override { return nullptr; } - virtual const char * querySpanId() const override { return nullptr; } + virtual const char * queryTraceId() const override { return "00000000000000000000000000000000"; } + virtual const char * querySpanId() const override { return "0000000000000000"; } - virtual const char* queryGlobalId() const override { return nullptr; } - virtual const char* queryCallerId() const override { return nullptr; } - virtual const char* queryLocalId() const override { return nullptr; } + // Note: GlobalID & LocalID are created from lnuid, which creates 23 char UIDs (16 rand bytes in base58), and uses "1" for zeroes + virtual const char* queryGlobalId() const override { return "11111111111111111111111"; } + virtual const char* queryCallerId() const override { return ""; } + virtual const char* queryLocalId() const override { return "11111111111111111111111"; } virtual ISpan * createClientSpan(const char * name, const SpanTimeStamp * spanStartTimeStamp = nullptr) override { return getNullSpan(); } virtual ISpan * createInternalSpan(const char * name, const SpanTimeStamp * spanStartTimeStamp = nullptr) override { return getNullSpan(); } @@ -1515,8 +1516,8 @@ ActiveSpanScope::~ActiveSpanScope() ISpan* current = queryThreadedActiveSpan(); if (current != span) { - const char* currSpanID = current != nullptr ? current->querySpanId() : "null"; - const char* expectedSpanID = span != nullptr ? span->querySpanId() : "null"; + const char* currSpanID = current->querySpanId(); + const char* expectedSpanID = span != nullptr ? span->querySpanId() : "0000000000000000"; IERRLOG("~ActiveSpanScope: threadActiveSpan has changed unexpectedly, expected: %s actual: %s", expectedSpanID, currSpanID); return; @@ -1564,19 +1565,19 @@ OwnedActiveSpanScope::~OwnedActiveSpanScope() //--------------------------------------------------------------------------------------------------------------------- -void OwnedSpanScope::setown(ISpan * _span) +void OwnedSpanLifetime::setown(ISpan * _span) { assertex(_span); clear(); span.setown(_span); } -void OwnedSpanScope::set(ISpan * _span) +void OwnedSpanLifetime::set(ISpan * _span) { setown(LINK(_span)); } -void OwnedSpanScope::clear() +void OwnedSpanLifetime::clear() { if (span) { @@ -1585,7 +1586,7 @@ void OwnedSpanScope::clear() } } -OwnedSpanScope::~OwnedSpanScope() +OwnedSpanLifetime::~OwnedSpanLifetime() { clear(); } diff --git a/system/jlib/jtrace.hpp b/system/jlib/jtrace.hpp index 897562c9a05..7c922cf8c23 100644 --- a/system/jlib/jtrace.hpp +++ b/system/jlib/jtrace.hpp @@ -166,10 +166,16 @@ interface ISpan : extends IInterface // OwnedActiveSpanScope controls the lifetime of its ISpan while ActiveSpanScope // does not. In cases where the ISpan will be used from a single thread and within // a single scope OwnedActiveSpanScope should be used. For more complicated scenarios, -// involving multiple threads, time sliced work, etc ActiveSpanScope should be used. +// involving multiple threads, time sliced work, etc ActiveSpanScope should be used +// to associate that ISpan with each processing thread / unit of work, while an +// OwnedSpanLifetime, likely a class member, should control the ISpan lifetime. +// +// When using ActiveSpanScope another class such as OwnedSpanLifetime should be +// used to control the lifetime of the ISpan and the referenced ISpans lifetime +// should be guaranteed to be longer than the ActiveSpanScopes lifetime. //------------------------------------------------------------------------------ -class ActiveSpanScope +class jlib_decl ActiveSpanScope { public: // Captures current threadActiveSpan for prevSpan @@ -192,22 +198,22 @@ class ActiveSpanScope ISpan * prevSpan = nullptr; }; -class jlib_decl OwnedActiveSpanScope +class jlib_decl OwnedSpanLifetime { public: - OwnedActiveSpanScope() = default; - OwnedActiveSpanScope(ISpan * _ptr); - OwnedActiveSpanScope(const OwnedActiveSpanScope& rhs) = delete; - OwnedActiveSpanScope(OwnedActiveSpanScope&& rhs) = default; - ~OwnedActiveSpanScope(); - - inline OwnedActiveSpanScope& operator=(ISpan * ptr) = delete; - inline OwnedActiveSpanScope& operator=(const OwnedActiveSpanScope& rhs) = delete; - inline OwnedActiveSpanScope& operator=(OwnedActiveSpanScope&& rhs) = delete; + OwnedSpanLifetime() = default; + OwnedSpanLifetime(ISpan * _ptr) : span(_ptr) {} + OwnedSpanLifetime(const OwnedSpanLifetime& rhs) = delete; + OwnedSpanLifetime(OwnedSpanLifetime&& rhs) = default; + ~OwnedSpanLifetime(); inline ISpan * operator -> () const { return span; } inline operator ISpan *() const { return span; } + inline OwnedSpanLifetime& operator=(ISpan * ptr) = delete; + inline OwnedSpanLifetime& operator=(const OwnedSpanLifetime& rhs) = delete; + inline OwnedSpanLifetime& operator=(OwnedSpanLifetime&& rhs) = delete; + void clear(); ISpan * query() const { return span; } void set(ISpan * _span); @@ -215,25 +221,24 @@ class jlib_decl OwnedActiveSpanScope private: Owned span; - ISpan * prevSpan = nullptr; }; -class jlib_decl OwnedSpanScope +class jlib_decl OwnedActiveSpanScope { public: - OwnedSpanScope() = default; - OwnedSpanScope(ISpan * _ptr) : span(_ptr) {} - OwnedSpanScope(const OwnedSpanScope& rhs) = delete; - OwnedSpanScope(OwnedSpanScope&& rhs) = default; - ~OwnedSpanScope(); + OwnedActiveSpanScope() = default; + OwnedActiveSpanScope(ISpan * _ptr); + OwnedActiveSpanScope(const OwnedActiveSpanScope& rhs) = delete; + OwnedActiveSpanScope(OwnedActiveSpanScope&& rhs) = default; + ~OwnedActiveSpanScope(); + + inline OwnedActiveSpanScope& operator=(ISpan * ptr) = delete; + inline OwnedActiveSpanScope& operator=(const OwnedActiveSpanScope& rhs) = delete; + inline OwnedActiveSpanScope& operator=(OwnedActiveSpanScope&& rhs) = delete; inline ISpan * operator -> () const { return span; } inline operator ISpan *() const { return span; } - inline OwnedSpanScope& operator=(ISpan * ptr) = delete; - inline OwnedSpanScope& operator=(const OwnedSpanScope& rhs) = delete; - inline OwnedSpanScope& operator=(OwnedSpanScope&& rhs) = delete; - void clear(); ISpan * query() const { return span; } void set(ISpan * _span); @@ -241,8 +246,10 @@ class jlib_decl OwnedSpanScope private: Owned span; + ISpan * prevSpan = nullptr; }; + extern jlib_decl IProperties * getClientHeaders(const ISpan * span); extern jlib_decl IProperties * getSpanContext(const ISpan * span); diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp index 7e324c5c8aa..e9c8c275386 100644 --- a/testing/unittests/jlibtests.cpp +++ b/testing/unittests/jlibtests.cpp @@ -363,6 +363,16 @@ class JlibTraceTest : public CppUnit::TestFixture savedSpan.clear(); } + void nullSpanTest() + { + Owned nullSpan = getNullSpan(); + CPPUNIT_ASSERT(nullSpan->queryTraceId() != nullptr); + CPPUNIT_ASSERT(nullSpan->querySpanId() != nullptr); + CPPUNIT_ASSERT(nullSpan->queryGlobalId() != nullptr); + CPPUNIT_ASSERT(nullSpan->queryCallerId() != nullptr); + CPPUNIT_ASSERT(nullSpan->queryLocalId() != nullptr); + } + void testTraceDisableConfig() { Owned testTree = createPTreeFromYAMLString(disableTracingYaml, ipt_none, ptr_ignoreWhiteSpace, nullptr); @@ -4190,6 +4200,7 @@ class JLibOpensslAESTest : public CppUnit::TestFixture E->Release(); } } + // Note: Using OpenTelemetry null span } void test()