HPCC-30470 codereviewy

- Removes span start/end logging config options Signed-off-by: Rodrigo Pastrana <[email protected]>
hpcc-systems · Dec 6, 2023 · cc7ff82 · cc7ff82
1 parent fcfcaaf
commit cc7ff82
Show file tree

Hide file tree

Showing 5 changed files with 0 additions and 122 deletions.
diff --git a/helm/examples/tracing/README.md b/helm/examples/tracing/README.md
@@ -11,8 +11,6 @@ All configuration options detailed here are part of the HPCC Systems Helm chart,
 - disabled - (default: false) disables tracking and reporting of internal traces and spans
 - alwaysCreateGlobalIds - If true, assign newly created global ID to any requests that do not supply one.
 - optAlwaysCreateTraceIds - If true components generate trace/span ids if none are provided by the remote caller.
-- logSpanStart - If true, generate a log entry whenever a span is started (default: false)
-- logSpanFinish - If true, generate a log entry whenever a span is finished (default: false)
 - exporter - Defines The type of exporter in charge of forwarding span data to target back-end
   - type - (defalt: JLOG) "OTLP-HTTP" | "OTLP-GRCP" | "OS" | "JLOG" | "NONE"
   - JLOG

diff --git a/helm/hpcc/values.schema.json b/helm/hpcc/values.schema.json
@@ -1135,16 +1135,6 @@
               "description": "Defines the manner in which trace data is processed - in batches, or simple as available"
             }
           }
-        },
-        "logSpanStart": {
-          "type": "boolean",
-          "description": "If true, generate a log entry whenever a span is started",
-          "default": false
-        },
-        "logSpanFinish": {
-          "type": "boolean",
-          "description": "If true, generate a log entry whenever a span is finished",
-          "default": false
         }
       },
       "additionalProperties": { "type": ["integer", "string", "boolean"] }

diff --git a/system/jlib/jtrace.cpp b/system/jlib/jtrace.cpp
@@ -430,8 +430,6 @@ class CTraceManager : implements ITraceManager, public CInterface
     bool enabled = true;
     bool optAlwaysCreateGlobalIds = false;
     bool optAlwaysCreateTraceIds = true;
-    bool optLogSpanStart = false;
-    bool optLogSpanFinish = false;
     StringAttr moduleName;
     nostd::shared_ptr<opentelemetry::trace::Tracer> tracer;
 
@@ -463,16 +461,6 @@ class CTraceManager : implements ITraceManager, public CInterface
         return optAlwaysCreateTraceIds;
     }
 
-    bool logSpanStart() const
-    {
-        return optLogSpanStart;
-    }
-
-    bool logSpanFinish() const
-    {
-        return optLogSpanFinish;
-    }
-
     nostd::shared_ptr<opentelemetry::trace::Tracer> queryTracer() const
     {
         return tracer;
@@ -497,13 +485,6 @@ class CSpan : public CInterfaceOf<ISpan>
         //Record the span as complete before we output the logging for the end of the span
         if (span != nullptr)
             span->End();
-
-        if (queryInternalTraceManager().logSpanFinish())
-        {
-            StringBuffer out;
-            toLog(out);
-            LOG(MCmonitorEvent, "SpanFinish: {%s}", out.str());
-        }
     }
 
     const char * getSpanID() const
@@ -514,17 +495,6 @@ class CSpan : public CInterfaceOf<ISpan>
     ISpan * createClientSpan(const char * name) override;
     ISpan * createInternalSpan(const char * name) override;
 
-    virtual void toLog(StringBuffer & out) const override
-    {
-        out.append(",\"Name\":\"").append(name.get()).append("\"");
-
-        if (span != nullptr)
-        {
-            out.append(",\"TraceID\":\"").append(traceID.get()).append("\"");
-            out.append(",\"SpanID\":\"").append(spanID.get()).append("\"");
-        }
-    }
-
     virtual void toString(StringBuffer & out) const
     {
         toString(out, true);
@@ -717,13 +687,6 @@ class CSpan : public CInterfaceOf<ISpan>
             if (span != nullptr)
             {
                 storeSpanContext();
-
-                if (queryInternalTraceManager().logSpanStart())
-                {
-                    StringBuffer out;
-                    toLog(out);
-                    LOG(MCmonitorEvent, "SpanStart: {%s}", out.str());
-                }
             }
         }
     }
@@ -806,7 +769,6 @@ class CNullSpan : public CInterfaceOf<ISpan>
     virtual bool getSpanContext(IProperties * ctxProps, bool otelFormatted) const override { return false; }
 
     virtual void toString(StringBuffer & out) const override {}
-    virtual void toLog(StringBuffer & out) const override {}
     virtual void getLogPrefix(StringBuffer & out) const override {}
 
     virtual const char* queryGlobalId() const override { return nullptr; }
@@ -869,15 +831,6 @@ class CChildSpan : public CSpan
         return localParentSpan->queryLocalId();
     }
 
-   virtual void toLog(StringBuffer & out) const override
-    {
-        CSpan::toLog(out);
-
-        out.append(",\"ParentSpanID\": \"");
-        out.append(localParentSpan->getSpanID());
-        out.append("\"");
-    }
-
     virtual void toString(StringBuffer & out, bool isLeaf) const
     {
         CSpan::toString(out, isLeaf);
@@ -901,12 +854,6 @@ class CInternalSpan : public CChildSpan
         init(SpanFlags::None);
     }
 
-    void toLog(StringBuffer & out) const override
-    {
-        out.append("\"Type\":\"Internal\"");
-        CChildSpan::toLog(out);
-    }
-
     void toString(StringBuffer & out, bool isLeaf) const override
     {
         out.append("\"Type\":\"Internal\"");
@@ -924,12 +871,6 @@ class CClientSpan : public CChildSpan
         init(SpanFlags::None);
     }
 
-    void toLog(StringBuffer & out) const override
-    {
-        out.append("\"Type\":\"Client\"");
-        CChildSpan::toLog(out);
-    }
-
     void toString(StringBuffer & out, bool isLeaf) const override
     {
         out.append("\"Type\":\"Client\"");
@@ -1054,28 +995,6 @@ class CServerSpan : public CSpan
         return hpccLocalId.get();
     }
 
-   virtual void toLog(StringBuffer & out) const override
-    {
-        out.append("\"Type\":\"Server\"");
-        CSpan::toLog(out);
-
-        if (!isEmptyString(hpccGlobalId.get()))
-            out.append(",\"GlobalID\":\"").append(hpccGlobalId.get()).append("\"");
-        if (!isEmptyString(hpccCallerId.get()))
-            out.append(",\"CallerID\":\"").append(hpccCallerId.get()).append("\"");
-        if (!isEmptyString(hpccLocalId.get()))
-            out.append(",\"LocalID\":\"").append(hpccLocalId.get()).append("\"");
-
-        if (remoteParentSpanCtx.IsValid())
-        {
-            out.append(",\"ParentSpanID\":\"");
-            char spanId[16] = {0};
-            remoteParentSpanCtx.span_id().ToLowerBase16(spanId);
-            out.append(16, spanId)
-            .append("\"");
-        }
-    }
-
     virtual void toString(StringBuffer & out, bool isLeaf) const override
     {
         out.append("\"Type\":\"Server\"");
@@ -1341,8 +1260,6 @@ void CTraceManager::initTracer(const IPropertyTree * traceConfig)
         //Non open-telemetry tracing configuration
         if (traceConfig)
         {
-            optLogSpanStart = traceConfig->getPropBool("@logSpanStart", optLogSpanStart);
-            optLogSpanFinish = traceConfig->getPropBool("@logSpanFinish", optLogSpanFinish);
             optAlwaysCreateGlobalIds = traceConfig->getPropBool("@alwaysCreateGlobalIds", optAlwaysCreateGlobalIds);
             optAlwaysCreateTraceIds = traceConfig->getPropBool("@alwaysCreateTraceIds", optAlwaysCreateTraceIds);
         }

diff --git a/system/jlib/jtrace.hpp b/system/jlib/jtrace.hpp
@@ -62,7 +62,6 @@ interface ISpan : extends IInterface
     virtual void addSpanEvent(const char * eventName, IProperties * attributes) = 0;
     virtual bool getSpanContext(IProperties * ctxProps, bool otelFormatted) const = 0;
     virtual void toString(StringBuffer & out) const = 0;
-    virtual void toLog(StringBuffer & out) const = 0;
     virtual void getLogPrefix(StringBuffer & out) const = 0;
 
     virtual ISpan * createClientSpan(const char * name) = 0;

diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp
@@ -472,32 +472,6 @@ class JlibTraceTest : public CppUnit::TestFixture
         Owned<ISpan> internalSpan2 = internalSpan->createInternalSpan("internalSpan2");
 
         StringBuffer out;
-        out.set("{");
-        internalSpan2->toLog(out);
-        out.append("}");
-        {
-            Owned<IPropertyTree> jtraceAsTree;
-            try
-            {
-                jtraceAsTree.setown(createPTreeFromJSONString(out.str()));
-            }
-            catch (IException *e)
-            {
-                StringBuffer msg;
-                msg.append("Unexpected toLog format failure detected: ");
-                e->errorMessage(msg);
-                e->Release();
-                CPPUNIT_ASSERT_MESSAGE(msg.str(), false);
-            }
-
-            CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected toLog format failure detected", true, jtraceAsTree != nullptr);
-            CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected missing 'TraceID' entry in toLog output", true, jtraceAsTree->hasProp("TraceID"));
-            CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected missing 'SpanID' entry in toLog output", true, jtraceAsTree->hasProp("SpanID"));
-            CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected missing 'Name' entry in toLog output", true, jtraceAsTree->hasProp("Name"));
-            CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected missing 'Type' entry in toLog output", true, jtraceAsTree->hasProp("Type"));
-            CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected missing 'ParentSpanID' entry in toLog output", true, jtraceAsTree->hasProp("ParentSpanID"));
-        }
-
         out.set("{");
         internalSpan2->toString(out);
         out.append("}");