HPCC-30401 Code review 3

- Revises logic to create otel span -- Span created if per-span flag set -- or global/component config flag set -- or serverSpan receives valid remote parent - Adds cppunit (cannot test scenario where no span created yet) Signed-off-by: Rodrigo Pastrana <[email protected]>
hpcc-systems · Nov 3, 2023 · 2d5d45b · 2d5d45b
1 parent ffe7d98
commit 2d5d45b
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 47 deletions.
diff --git a/system/jlib/jtrace.cpp b/system/jlib/jtrace.cpp
@@ -16,7 +16,6 @@
 ############################################################################## */
 
 
-//#include "opentelemetry/sdk/trace/random_id_generator_factory.h"
 #include "opentelemetry/trace/semantic_conventions.h" //known span defines
 #include "opentelemetry/context/propagation/global_propagator.h" // context::propagation::GlobalTextMapPropagator::GetGlobalPropagator
 #include "opentelemetry/sdk/trace/tracer_provider_factory.h" //opentelemetry::sdk::trace::TracerProviderFactory::Create(context)
@@ -171,10 +170,6 @@ class CSpan : public CInterfaceOf<ISpan>
         return spanID.get();
     }
 
-    const char * getTraceID() const
-    {
-        return traceID.get();
-    }
 
     ISpan * createClientSpan(const char * name) override;
     ISpan * createInternalSpan(const char * name) override;
@@ -443,7 +438,10 @@ class CSpan : public CInterfaceOf<ISpan>
             hpccLocalId.set(ln_uid::createUniqueIdString().c_str());
 
         //we don't always want to create trace/span IDs
-        if (hasMask(flags, SpanFlags::EnsureTraceId) || queryTraceManager().alwaysCreateTraceIds())
+
+        if (hasMask(flags, SpanFlags::EnsureTraceId) || //per span flags
+            queryTraceManager().alwaysCreateTraceIds() || //Global/conponet flags
+            nostd::get<trace_api::SpanContext>(opts.parent).IsValid()) // valid parent was passed in
         {
             auto provider = opentelemetry::trace::Provider::GetTracerProvider();
 
@@ -484,11 +482,7 @@ class CSpan : public CInterfaceOf<ISpan>
             opts.parent = localParentSpanCtx;
         }
 
-        //attempt to inherit traceID directly from localParent as well
-        if (!isEmptyString(localParentSpan->getTraceID()))
-        {
-            traceID.set(localParentSpan->getTraceID());
-        }
+
     }
 
     void storeTraceID()
@@ -615,9 +609,6 @@ class CServerSpan : public CSpan
     //Remote parent is declared via http headers from client call
     opentelemetry::v1::trace::SpanContext remoteParentSpanCtx = opentelemetry::trace::SpanContext::GetInvalid();
 
-    //Random ID generator for trace and span IDs
-    //std::unique_ptr<opentelemetry::v1::sdk::trace::IdGenerator> randomIDGenerator = opentelemetry::v1::sdk::trace::RandomIdGeneratorFactory::Create();
-
     void setSpanContext(const IProperties * httpHeaders, SpanFlags flags)
     {
         if (httpHeaders)
@@ -651,40 +642,8 @@ class CServerSpan : public CSpan
                 remoteParentSpanCtx = remoteParentSpan->GetContext();
                 opts.parent = remoteParentSpanCtx;
 
-                //at this point we could set the traceID to the remoteParentSpanCtx values
-                char traceId[32] = {0};
-
-                remoteParentSpanCtx.trace_id().ToLowerBase16(traceId);
-                traceID.set(traceId, 32);
             }
         }
-
-       /*if ((!httpHeaders ||  !httpHeaders->hasProp("traceparent")) && queryTraceManager().alwaysCreateTraceIds())
-        {
-            //Generate random trace and span IDs
-            auto randomTraceID = randomIDGenerator->GenerateTraceId();
-            auto randomSpanID = randomIDGenerator->GenerateSpanId();
-
-            //Generate spancontext based on random Trace/SpanIDs
-            remoteParentSpanCtx = opentelemetry::trace::SpanContext(randomTraceID, randomSpanID, opentelemetry::trace::TraceFlags(), true);
-            //Assign new spanctx as this span's remote parent
-            opts.parent = remoteParentSpanCtx;
-
-//this is only for debugging:
-            char trace_id[32] = {0};
-
-            randomTraceID.ToLowerBase16(trace_id);
-            StringAttr traceID;
-            traceID.set(trace_id, 32);
-
-            char span_id[16] = {0};
-            randomSpanID.ToLowerBase16(span_id);
-            StringAttr spanID;
-            spanID.set(span_id, 16);
-
-            DBGLOG("!!No remoteParent received, generating remoteParent IDs: Trace '%s' span '%s' ", traceID.get(), spanID.get());
-        }
-        */
     }
 
     bool getSpanContext(IProperties * ctxProps, bool otelFormatted) const override
@@ -998,7 +957,7 @@ class CTraceManager : implements ITraceManager, public CInterface
         return optAlwaysCreateGlobalIds;
     }
 
-        virtual bool alwaysCreateTraceIds() const
+    virtual bool alwaysCreateTraceIds() const
     {
         return optAlwaysCreateTraceIds;
     }

diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp
@@ -54,6 +54,7 @@ class JlibTraceTest : public CppUnit::TestFixture
         CPPUNIT_TEST(testInvalidPropegatedServerSpan);
         CPPUNIT_TEST(testInternalSpan);
         CPPUNIT_TEST(testMultiNestedSpanTraceOutput);
+        CPPUNIT_TEST(testEnsureTraceID);
     CPPUNIT_TEST_SUITE_END();
 
     const char * simulatedGlobalYaml = R"!!(global:
@@ -107,6 +108,21 @@ class JlibTraceTest : public CppUnit::TestFixture
         initTraceManager("somecomponent", traceConfig, nullptr);
     }
 
+    void testEnsureTraceID()
+    {
+        SpanFlags flags = SpanFlags::EnsureTraceId;
+        Owned<IProperties> emptyMockHTTPHeaders = createProperties();
+        Owned<ISpan> serverSpan = queryTraceManager().createServerSpan("noRemoteParentEnsureTraceID", emptyMockHTTPHeaders, flags);
+
+        Owned<IProperties> retrievedSpanCtxAttributes = createProperties();
+        bool getSpanCtxSuccess = serverSpan->getSpanContext(retrievedSpanCtxAttributes.get(), false);
+
+        CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected getSpanContext failure detected - EnsureTraceID flag was set", true, getSpanCtxSuccess);
+
+        CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected empty TraceID detected", false, isEmptyString(retrievedSpanCtxAttributes->queryProp("traceID")));
+        CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected empty SpanID detected", false, isEmptyString(retrievedSpanCtxAttributes->queryProp("spanID")));
+    }
+
     void testIDPropegation()
     {
         Owned<IProperties> mockHTTPHeaders = createProperties();