HPCC-29334 Code review2

- Adds ISpan query Trace/Span ID methods - Jlog tracks linked Ispan Signed-off-by: Rodrigo Pastrana <[email protected]>
hpcc-systems · May 3, 2024 · 6fe65db · 6fe65db
1 parent 8f97317
commit 6fe65db
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 17 deletions.
diff --git a/system/jlib/jlog.hpp b/system/jlib/jlog.hpp
@@ -328,7 +328,6 @@ typedef enum
 #define MSGFIELD_STANDARD LogMsgField(MSGFIELD_timeDate | MSGFIELD_msgID | MSGFIELD_process | MSGFIELD_thread | MSGFIELD_code | MSGFIELD_quote | MSGFIELD_prefix | MSGFIELD_audience)
 #define MSGFIELD_LEGACY LogMsgField(MSGFIELD_timeDate | MSGFIELD_milliTime | MSGFIELD_msgID | MSGFIELD_process | MSGFIELD_thread | MSGFIELD_code | MSGFIELD_quote | MSGFIELD_prefix)
 #else
-//should trace/span be included in the standard fields? As-is, stderr output will not include trace/span
 #ifdef _CONTAINERIZED
 #define MSGFIELD_STANDARD LogMsgField(MSGFIELD_span | MSGFIELD_trace | MSGFIELD_job | MSGFIELD_timeDate | MSGFIELD_milliTime | MSGFIELD_msgID | MSGFIELD_process | MSGFIELD_thread | MSGFIELD_code | MSGFIELD_quote | MSGFIELD_class | MSGFIELD_audience)
 #else
@@ -598,33 +597,39 @@ class jlib_decl LogMsgJobInfo
     bool                      isDeserialized = false;
 };
 
+#define UNK_LOG_ENTRY "UNK"
 class jlib_decl LogMsgTraceInfo
 {
 public:
-    LogMsgTraceInfo() {}
-    LogMsgTraceInfo(ISpan * span)
+    LogMsgTraceInfo() = default;
+    LogMsgTraceInfo(ISpan * _span) : span(_span)
+    {
+    }
+
+    const char * queryTraceID() const
     {
         if (span)
         {
-            Owned<IProperties> retrievedSpanCtxAttributes = createProperties();
-            queryThreadedActiveSpan()->getSpanContext(retrievedSpanCtxAttributes.get());
-            //too many string copies? maybe store LogMsgTraceInfo in thread context instead of ispan?
-            if (retrievedSpanCtxAttributes->hasProp("traceID"))
-                setTraceID(retrievedSpanCtxAttributes->queryProp("traceID"));
-            if (retrievedSpanCtxAttributes->hasProp("spanID")) //can we ever have a spanid sans traceid?
-                setSpanID(retrievedSpanCtxAttributes->queryProp("spanID"));
+            const char * traceId = span->queryTraceId();
+            if (traceId)
+                return traceId;
         }
+        return UNK_LOG_ENTRY;
     }
 
-    ~LogMsgTraceInfo() {};
+    const char * querySpanID() const
+    {
+        if (span)
+        {
+            const char * spanId = span->querySpanId();
+            if (spanId)
+                return spanId;
+        }
+        return UNK_LOG_ENTRY;
+    }
 
-    const char * queryTraceID() const { return traceID.get();}
-    const char * querySpanID() const { return spanID.get();};
-    void setTraceID(const char * theTraceID) { traceID.set(theTraceID);};
-    void setSpanID(const char * theSpanID) { spanID.set(theSpanID);};
 private:
-    StringAttr          traceID = "UNK";
-    StringAttr          spanID = "UNK";
+    Linked<ISpan> span;
 };
 
 class jlib_decl LogMsg : public CInterface

diff --git a/system/jlib/jtrace.cpp b/system/jlib/jtrace.cpp
@@ -791,6 +791,16 @@ class CSpan : public CInterfaceOf<ISpan>
         recordError(SpanError(msg.str(), e->errorCode(), spanFailed, escapedScope));
     };
 
+    virtual const char * queryTraceId() const override
+    {
+        return traceID.get(); 
+    }
+
+    virtual const char * querySpanId() const override
+    {
+        return spanID.get();
+    }
+
 protected:
     CSpan(const char * spanName)
     {
@@ -901,6 +911,9 @@ class CNullSpan final : public CInterfaceOf<ISpan>
     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* queryGlobalId() const override { return nullptr; }
     virtual const char* queryCallerId() const override { return nullptr; }
     virtual const char* queryLocalId() const override { return nullptr; }

diff --git a/system/jlib/jtrace.hpp b/system/jlib/jtrace.hpp
@@ -124,6 +124,8 @@ interface ISpan : extends IInterface
     virtual void recordException(IException * e, bool spanFailed = true, bool escapedScope = true) = 0;
     virtual void recordError(const SpanError & error = SpanError()) = 0;
     virtual void setSpanStatusSuccess(bool spanSucceeded, const char * statusMessage = NO_STATUS_MESSAGE) = 0;
+    virtual const char * queryTraceId() const = 0;
+    virtual const char * querySpanId() const = 0;
 
     virtual ISpan * createClientSpan(const char * name) = 0;
     virtual ISpan * createInternalSpan(const char * name) = 0;

diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp
@@ -67,6 +67,7 @@ class JlibTraceTest : public CppUnit::TestFixture
         CPPUNIT_TEST(manualTestsDeclaredFailures);
         CPPUNIT_TEST(manualTestScopeEnd);
         CPPUNIT_TEST(testActiveSpans);
+        CPPUNIT_TEST(testSpanFetchMethods);
 
         //CPPUNIT_TEST(testJTraceJLOGExporterprintResources);
         //CPPUNIT_TEST(testJTraceJLOGExporterprintAttributes);
@@ -370,6 +371,25 @@ class JlibTraceTest : public CppUnit::TestFixture
         CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected empty SpanID detected", false, isEmptyString(retrievedSpanCtxAttributes->queryProp("spanID")));
     }
 
+    void testSpanFetchMethods()
+    {
+        SpanFlags flags = SpanFlags::EnsureTraceId;
+        Owned<IProperties> emptyMockHTTPHeaders = createProperties();
+        OwnedSpanScope serverSpan = queryTraceManager().createServerSpan("noRemoteParentEnsureTraceID", emptyMockHTTPHeaders, flags);
+
+        Owned<IProperties> retrievedSpanCtxAttributes = createProperties();
+        serverSpan->getSpanContext(retrievedSpanCtxAttributes.get());
+
+        const char * traceID = retrievedSpanCtxAttributes->queryProp("traceID");
+        const char * spanID = retrievedSpanCtxAttributes->queryProp("spanID");
+
+        CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected empty TraceID detected", false, isEmptyString(traceID));
+        CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected empty SpanID detected", false, isEmptyString(spanID));
+
+        CPPUNIT_ASSERT_MESSAGE("Queried traceID mismatch vs traceID from attributes",  strsame(serverSpan->queryTraceId(), traceID));
+        CPPUNIT_ASSERT_MESSAGE("Queried spanID mismatch vs spanID from attributes",  strsame(serverSpan->querySpanId(), spanID));
+}
+
     void testIDPropegation()
     {
         Owned<IProperties> mockHTTPHeaders = createProperties();