Merge pull request #18381 from ghalliday/issue31396

HPCC-31396 Introduce a class for tracking the lifetime of a span Reviewed-By: Rodrigo Pastrana <[email protected]> Merged-by: Gavin Halliday <[email protected]>
hpcc-systems · Mar 7, 2024 · 80adbd9 · 80adbd9
2 parents 34c3937 + f8ee51f
commit 80adbd9
Show file tree

Hide file tree

Showing 13 changed files with 70 additions and 57 deletions.
diff --git a/ecl/eclagent/eclagent.cpp b/ecl/eclagent/eclagent.cpp
@@ -2128,9 +2128,9 @@ void EclAgent::runProcess(IEclProcess *process)
     allocatorMetaCache.setown(createRowAllocatorCache(this));
 
     Owned<IProperties> traceHeaders = extractTraceDebugOptions(queryWorkUnit());
-    Owned<ISpan> requestSpan = queryTraceManager().createServerSpan("run_workunit", traceHeaders);
-    requestSpan->setSpanAttribute("hpcc.wuid", queryWorkUnit()->queryWuid());
+    OwnedSpanScope requestSpan = queryTraceManager().createServerSpan("run_workunit", traceHeaders);
     ContextSpanScope spanScope(updateDummyContextLogger(), requestSpan);
+    requestSpan->setSpanAttribute("hpcc.wuid", queryWorkUnit()->queryWuid());
 
     // a component may specify an alternate name for the agent/workflow memory area,
     // e.g. Thor specifies in "eclAgentMemory"

diff --git a/esp/bindings/http/platform/httpservice.cpp b/esp/bindings/http/platform/httpservice.cpp
@@ -412,13 +412,10 @@ int CEspHttpServer::processRequest()
                 return 0;
             }
 
-            //The context will be destroyed when this request is destroyed. So rather than using
-            //EspContextSpanScope spanContext(*ctx, serverSpan);
-            //which would remove the activeSpan when this function exits, use
-            //setActiveSpan()
-            //It is possible that using EspContextSpanScope would be better
+            //The context will be destroyed when this request is destroyed. So initialise a SpanScope in the context to
+            //ensure the span is also terminated at the same time.
             Owned<ISpan> serverSpan = m_request->createServerSpan(serviceName, methodName);
-            ctx->setActiveSpan(serverSpan);
+            ctx->setRequestSpan(serverSpan);
 
             if (thebinding!=NULL)
             {

diff --git a/esp/platform/esp.hpp b/esp/platform/esp.hpp
@@ -235,15 +235,13 @@ interface IEspContext : extends IInterface
     virtual void setRequest(IHttpMessage* req) = 0;
     virtual IHttpMessage* queryRequest() = 0;
 
-    virtual void setActiveSpan(ISpan * span)=0; // Only call this function directly if this object's lifetime matches the lifetime of the span.  If there is any doubt use EspContextSpanScope(ctx, span)
+    virtual void setRequestSpan(ISpan * span)=0;     // Call this function to set the server span for the query.  The spans's lifetime will match the lifetime of the context object.
     virtual ISpan * queryActiveSpan() const = 0;
     virtual IProperties * getClientSpanHeaders() const = 0;
     virtual const char* getGlobalId() const = 0;
     virtual const char* getCallerId() const = 0;
     virtual const char* getLocalId() const = 0;
 };
-//Class for managing the active span in the context
-using EspContextSpanScope = ContextSpanScopeImp<IEspContext>;
 
 
 typedef unsigned LogLevel;

diff --git a/esp/platform/espcontext.cpp b/esp/platform/espcontext.cpp
@@ -89,7 +89,7 @@ class CEspContext : public CInterface, implements IEspContext
     Owned<IEspSecureContextEx> m_secureContext;
 
     StringAttr   m_transactionID;
-    Owned<ISpan> m_activeSpan;
+    OwnedSpanScope m_requestSpan;    // When the context is destroy the span will end.
     IHttpMessage* m_request;
 
 public:
@@ -114,7 +114,7 @@ class CEspContext : public CInterface, implements IEspContext
         updateTraceSummaryHeader();
         m_secureContext.setown(secureContext);
         m_SecurityHandler.setSecureContext(secureContext);
-        m_activeSpan.set(getNullSpan());
+        m_requestSpan.setown(getNullSpan());
     }
 
     ~CEspContext()
@@ -626,30 +626,30 @@ class CEspContext : public CInterface, implements IEspContext
     {
         return m_request;
     }
-    virtual void setActiveSpan(ISpan * span) override
+    virtual void setRequestSpan(ISpan * span) override
     {
-        m_activeSpan.set(span);
+        m_requestSpan.set(span);
     }
     virtual ISpan * queryActiveSpan() const override
     {
-        return m_activeSpan;
+        return m_requestSpan;
     }
 
     virtual const char* getGlobalId() const override
     {
-        return m_activeSpan->queryGlobalId();
+        return m_requestSpan->queryGlobalId();
     }
     virtual const char* getCallerId() const override
     {
-        return m_activeSpan->queryCallerId();
+        return m_requestSpan->queryCallerId();
     }
     virtual const char* getLocalId() const override
     {
-        return m_activeSpan->queryLocalId();
+        return m_requestSpan->queryLocalId();
     }
     virtual IProperties * getClientSpanHeaders() const override
     {
-        return ::getClientHeaders(m_activeSpan);
+        return ::getClientHeaders(m_requestSpan);
     }
 };
 

diff --git a/esp/services/esdl_svc_engine/esdl_binding.cpp b/esp/services/esdl_svc_engine/esdl_binding.cpp
@@ -1571,9 +1571,8 @@ void EsdlServiceImpl::sendTargetSOAP(IEspContext & context,
         httpclient->setPassword(password.str());
     }
 
-    Owned<ISpan> clientSpan;
     ISpan * activeSpan = context.queryActiveSpan();
-    clientSpan.setown(activeSpan->createClientSpan("soapcall"));
+    OwnedSpanScope clientSpan(activeSpan->createClientSpan("soapcall"));
 
     Owned<IProperties> headers = ::getClientHeaders(clientSpan);
     StringBuffer status;

diff --git a/esp/services/ws_ecl/ws_ecl_service.cpp b/esp/services/ws_ecl/ws_ecl_service.cpp
@@ -1978,9 +1978,8 @@ int CWsEclBinding::submitWsEclWorkunit(IEspContext & context, WsEclWuInfo &wsinf
 
     bool noTimeout = false;
 
-    Owned<ISpan> clientSpan;
     ISpan * activeSpan = context.queryActiveSpan();
-    clientSpan.setown(activeSpan->createClientSpan("run_workunit"));
+    OwnedSpanScope clientSpan(activeSpan->createClientSpan("run_workunit"));
     Owned<IProperties> httpHeaders = ::getClientHeaders(clientSpan);
     recordTraceDebugOptions(workunit, httpHeaders);
 

diff --git a/esp/services/ws_workunits/ws_workunitsHelpers.cpp b/esp/services/ws_workunits/ws_workunitsHelpers.cpp
@@ -3779,7 +3779,7 @@ void WsWuHelpers::submitWsWorkunit(IEspContext& context, IConstWorkUnit* cw, con
     }
 
     ISpan * activeSpan = context.queryActiveSpan();
-    Owned<ISpan> clientSpan(activeSpan->createClientSpan("run_workunit"));
+    OwnedSpanScope clientSpan(activeSpan->createClientSpan("run_workunit"));
     Owned<IProperties> httpHeaders = ::getClientHeaders(clientSpan);
     recordTraceDebugOptions(wu, httpHeaders);
 

diff --git a/roxie/ccd/ccdlistener.cpp b/roxie/ccd/ccdlistener.cpp
@@ -1200,7 +1200,7 @@ class RoxieWorkUnitWorker : public RoxieQueryWorker
         Owned<StringContextLogger> logctx = new StringContextLogger(wuid.get());
 
         Owned<IProperties> traceHeaders = extractTraceDebugOptions(wu);
-        Owned<ISpan> requestSpan = queryTraceManager().createServerSpan("run_workunit", traceHeaders);
+        OwnedSpanScope requestSpan = queryTraceManager().createServerSpan("run_workunit", traceHeaders);
         requestSpan->setSpanAttribute("hpcc.wuid", wuid);
         ContextSpanScope spanScope(*logctx, requestSpan);
 
@@ -1381,6 +1381,7 @@ class RoxieProtocolMsgContext : implements IHpccProtocolMsgContext, public CInte
     Owned<CDebugCommandHandler> debugCmdHandler;
     Owned<StringContextLogger> logctx;
     Owned<IQueryFactory> queryFactory;
+    OwnedSpanScope requestSpan;
 
     SocketEndpoint ep;
     time_t startTime;
@@ -1484,8 +1485,7 @@ class RoxieProtocolMsgContext : implements IHpccProtocolMsgContext, public CInte
         if (spanName.length())
             spanName.append('/');
         spanName.append(spanQueryName);
-        Owned<ISpan> requestSpan = queryTraceManager().createServerSpan(spanName, allHeaders, flags);
-        //The span has a lifetime the same length as the logctx, so no need to restore it at the end of the query
+        requestSpan.setown(queryTraceManager().createServerSpan(spanName, allHeaders, flags));
         logctx->setActiveSpan(requestSpan);
 
         const char * globalId = requestSpan->queryGlobalId();

diff --git a/system/jlib/jtrace.cpp b/system/jlib/jtrace.cpp
@@ -536,8 +536,7 @@ class CSpan : public CInterfaceOf<ISpan>
     virtual void beforeDispose() override
     {
         //Record the span as complete before we output the logging for the end of the span
-        if (span != nullptr)
-            span->End();
+        endSpan();
     }
 
     const char * getSpanID() const
@@ -548,6 +547,13 @@ class CSpan : public CInterfaceOf<ISpan>
     ISpan * createClientSpan(const char * name) override;
     ISpan * createInternalSpan(const char * name) override;
 
+    virtual void endSpan() final override
+    {
+        //It is legal to call endSpan multiple times, but only the first call will have any effect
+        if (span != nullptr)
+            span->End();
+    }
+
     virtual void toString(StringBuffer & out) const
     {
         toString(out, true);
@@ -822,7 +828,7 @@ class CSpan : public CInterfaceOf<ISpan>
 
 };
 
-class CNullSpan : public CInterfaceOf<ISpan>
+class CNullSpan final : public CInterfaceOf<ISpan>
 {
 public:
     CNullSpan() = default;
@@ -832,6 +838,7 @@ class CNullSpan : public CInterfaceOf<ISpan>
     virtual void setSpanAttributes(const IProperties * attributes) override {}
     virtual void addSpanEvent(const char * eventName) override {}
     virtual void addSpanEvent(const char * eventName, IProperties * attributes) override {};
+    virtual void endSpan() override {}
     virtual void getSpanContext(IProperties * ctxProps) const override {}
     virtual void getClientHeaders(IProperties * clientHeaders) const override {}
 

diff --git a/system/jlib/jtrace.hpp b/system/jlib/jtrace.hpp
@@ -62,6 +62,7 @@ interface ISpan : extends IInterface
     virtual void setSpanAttributes(const IProperties * attributes) = 0;
     virtual void addSpanEvent(const char * eventName) = 0;
     virtual void addSpanEvent(const char * eventName, IProperties * attributes) = 0;
+    virtual void endSpan() = 0; // Indicate that the span has ended even if it has not yet been destroyed
     virtual void getSpanContext(IProperties * ctxProps) const = 0;
     virtual void getClientHeaders(IProperties * clientHeaders) const = 0;
     virtual void toString(StringBuffer & out) const = 0;
@@ -77,6 +78,18 @@ interface ISpan : extends IInterface
     virtual const char* queryLocalId() const = 0;
 };
 
+class OwnedSpanScope : public Owned<ISpan>
+{
+public:
+    inline OwnedSpanScope() { }
+    inline OwnedSpanScope(ISpan * _ptr) : Owned<ISpan>(_ptr) { }
+    ~OwnedSpanScope()
+    {
+        if (get())
+            get()->endSpan();
+    }
+};
+
 extern jlib_decl IProperties * getClientHeaders(const ISpan * span);
 extern jlib_decl IProperties * getSpanContext(const ISpan * span);