From 6a988d60840d7be97c9c04c48d56ffcbad63218f Mon Sep 17 00:00:00 2001
From: Rodrigo Pastrana <Rodrigo.Pastrana@lexisnexisrisk.com>
Date: Fri, 22 Sep 2023 15:13:45 -0400
Subject: [PATCH] HPCC-30301 Code review 1

- Utilizes beforeDispose
- Reverts derived destructor pattern
- Introduces leaf/branch toString dynamic output

Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexisrisk.com>
---
 system/jlib/jtrace.cpp | 76 ++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/system/jlib/jtrace.cpp b/system/jlib/jtrace.cpp
index fe86d6cb5c8..17021bf6f15 100644
--- a/system/jlib/jtrace.cpp
+++ b/system/jlib/jtrace.cpp
@@ -149,33 +149,50 @@ class CSpan : public CInterfaceOf<ISpan>
     CSpan() = delete;
     virtual ~CSpan()
     {
-        //Derived class specific behavior such as toString() should be
-        //called from the destructor of the derived class
         if (span != nullptr)
             span->End();
     }
 
+    virtual void beforeDispose() override
+    {
+        StringBuffer out;
+        toString(out);
+        DBGLOG("Span end: {%s}", out.str());
+    }
+
     ISpan * createClientSpan(const char * name) override;
     ISpan * createInternalSpan(const char * name) override;
 
     void toString(StringBuffer & out) const override
+    {
+        toString(out, true);
+    }
+
+    virtual void toString(StringBuffer & out, bool isLeaf) const
     {
         out.append(",\"Name\":\"").append(name.get()).append("\"");
-        if (!isEmptyString(hpccGlobalId.get()))
-            out.append(",\"HPCCGlobalID\":\"").append(hpccGlobalId.get()).append("\"");
-        if (!isEmptyString(hpccCallerId.get()))
-            out.append(",\"HPCCCallerID\":\"").append(hpccCallerId.get()).append("\"");
+        if (isLeaf)
+        {
+            if (!isEmptyString(hpccGlobalId.get()))
+                out.append(",\"HPCCGlobalID\":\"").append(hpccGlobalId.get()).append("\"");
+            if (!isEmptyString(hpccCallerId.get()))
+                out.append(",\"HPCCCallerID\":\"").append(hpccCallerId.get()).append("\"");
+        }
 
         if (span != nullptr)
         {
-            out.append(",\"SpanID\":\"").append(spanID.get()).append("\"")
-            .append(",\"TraceID\":\"").append(traceID.get()).append("\"")
-            .append(",\"TraceFlags\":\"").append(traceFlags.get()).append("\"");
+            out.append(",\"SpanID\":\"").append(spanID.get()).append("\"");
+
+            if (isLeaf)
+            {
+                out.append(",\"TraceID\":\"").append(traceID.get()).append("\"")
+                 .append(",\"TraceFlags\":\"").append(traceFlags.get()).append("\"");
+            }
 
             if (localParentSpan != nullptr)
             {
                 out.append(",\"LocalParentSpan\":{ ");
-                localParentSpan->toString(out);
+                localParentSpan->toString(out, false);
                 out.append(" }");
             }
         }
@@ -317,9 +334,6 @@ class CSpan : public CInterfaceOf<ISpan>
         localParentSpan = parent;
         if (localParentSpan != nullptr)
             tracerName.set(parent->queryTraceName());
-        //type = spanType;
-
-       // init();
     }
 
     CSpan(const char * spanName, const char * nameOfTracer)
@@ -327,8 +341,6 @@ class CSpan : public CInterfaceOf<ISpan>
         name.set(spanName);
         localParentSpan = nullptr;
         tracerName.set(nameOfTracer);
-        //type = spanType;
-        
     }
 
     void init()
@@ -437,13 +449,6 @@ class CSpan : public CInterfaceOf<ISpan>
 class CInternalSpan : public CSpan
 {
 public:
-    ~CInternalSpan() override
-    {
-        StringBuffer out;
-        toString(out);
-        DBGLOG("Span end: {%s}", out.str());
-    }
-
     CInternalSpan(const char * spanName, CSpan * parent)
     : CSpan(spanName, parent)
     {
@@ -451,23 +456,16 @@ class CInternalSpan : public CSpan
         init();
     }
 
-    void toString(StringBuffer & out) const override
+    void toString(StringBuffer & out, bool isLeaf) const override
     {
         out.append("\"Type\":\"Internal\"");
-        CSpan::toString(out);
+        CSpan::toString(out, isLeaf);
     }
 };
 
 class CClientSpan : public CSpan
 {
 public:
-    ~CClientSpan() override
-    {
-        StringBuffer out;
-        toString(out);
-        DBGLOG("Span end: {%s}", out.str());
-    }
-
     CClientSpan(const char * spanName, CSpan * parent)
     : CSpan(spanName, parent)
     {
@@ -475,10 +473,10 @@ class CClientSpan : public CSpan
         init();
     }
 
-    void toString(StringBuffer & out) const override
+    void toString(StringBuffer & out, bool isLeaf) const override
     {
         out.append("\"Type\":\"Client\"");
-        CSpan::toString(out);
+        CSpan::toString(out, isLeaf);
     }
 };
 
@@ -578,14 +576,6 @@ class CServerSpan : public CSpan
     }
 
 public:
-
-    ~CServerSpan() override
-    {
-        StringBuffer out;
-        toString(out);
-        DBGLOG("Span end: {%s}", out.str());
-    }
-
     CServerSpan(const char * spanName, const char * tracerName_, StringArray & httpHeaders)
     : CSpan(spanName, tracerName_)
     {
@@ -602,10 +592,10 @@ class CServerSpan : public CSpan
         init();
     }
 
-    void toString(StringBuffer & out) const override
+    void toString(StringBuffer & out, bool isLeaf) const override
     {
         out.append("\"Type\":\"Server\"");
-        CSpan::toString(out);
+        CSpan::toString(out, isLeaf);
 
         if (remoteParentSpanCtx.IsValid())
         {