From bcddee96d31a4db886358c7c739a45e926043c18 Mon Sep 17 00:00:00 2001
From: Rodrigo Pastrana <Rodrigo.Pastrana@lexisnexisrisk.com>
Date: Tue, 25 Jun 2024 09:49:39 -0400
Subject: [PATCH] HPCC-32131 Code review 1

- Reenabled commented out logic
- Establishes default batch mode based on exporter type
- Removes useful debug log

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

diff --git a/system/jlib/jtrace.cpp b/system/jlib/jtrace.cpp
index 7f49bf31117..660e377e854 100644
--- a/system/jlib/jtrace.cpp
+++ b/system/jlib/jtrace.cpp
@@ -494,7 +494,7 @@ class CTraceManager : implements ITraceManager, public CInterface
     void initTracerProviderAndGlobalInternals(const IPropertyTree * traceConfig);
     void initTracer(const IPropertyTree * traceConfig);
     void cleanupTracer();
-    std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> createExporter(const IPropertyTree * exportConfig);
+    std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> createExporter(const IPropertyTree * exportConfig, bool & shouldBatch);
     std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor> createProcessor(const IPropertyTree * exportConfig);
 
 public:
@@ -1159,7 +1159,7 @@ IProperties * getSpanContext(const ISpan * span)
 
 //---------------------------------------------------------------------------------------------------------------------
 
-std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> CTraceManager::createExporter(const IPropertyTree * exportConfig)
+std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> CTraceManager::createExporter(const IPropertyTree * exportConfig, bool & shouldBatch)
 {
     assertex(exportConfig);
 
@@ -1172,6 +1172,7 @@ std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> CTraceManager::createEx
         if (stricmp(exportType.str(), "OS")==0) //To stdout/err
         {
             LOG(MCoperatorInfo, "Tracing exporter set OS");
+            shouldBatch = false;
             return opentelemetry::exporter::trace::OStreamSpanExporterFactory::Create();
         }
         else if (stricmp(exportType.str(), "OTLP")==0 || stricmp(exportType.str(), "OTLP-HTTP")==0)
@@ -1255,6 +1256,7 @@ std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> CTraceManager::createEx
             if (logFlags == SpanLogFlags::LogNone)
                 logFlags = DEFAULT_SPAN_LOG_FLAGS;
 
+            shouldBatch = false;
             LOG(MCoperatorInfo, "Tracing exporter set to JLog: logFlags( LogAttributes LogParentInfo %s)", logFlagsStr.str());
             return JLogSpanExporterFactory::Create(logFlags);
         }
@@ -1268,10 +1270,11 @@ std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> CTraceManager::createEx
 
 std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor> CTraceManager::createProcessor(const IPropertyTree * exportConfig)
 {
+    bool batchDefault = true;
     std::unique_ptr<opentelemetry::v1::sdk::trace::SpanExporter> exporter;
     try
     {
-        exporter = createExporter(exportConfig);
+        exporter = createExporter(exportConfig, batchDefault);
     }
     catch(const std::exception& e) //polymorphic type std::exception
     {
@@ -1285,7 +1288,11 @@ std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor> CTraceManager::createP
     if (!exporter)
         return nullptr;
 
-    if (exportConfig->getPropBool("batch/@enabled", true))
+    if ( dynamic_cast<opentelemetry::exporter::trace::OStreamSpanExporterFactory * >(exporter.get()) != nullptr ||
+         dynamic_cast<JLogSpanExporterFactory * >(exporter.get()) != nullptr)
+        batchDefault = false;
+
+    if (exportConfig->getPropBool("batch/@enabled", batchDefault))
     {
         //Groups several spans together, before sending them to an exporter.
         opentelemetry::v1::sdk::trace::BatchSpanProcessorOptions options;
@@ -1304,7 +1311,6 @@ std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor> CTraceManager::createP
          */
         options.max_export_batch_size = exportConfig->getPropInt("batch/@maxExportBatchSize", 512);
 
-        DBGLOG("Tracing exporter set to batch mode");
         return opentelemetry::sdk::trace::BatchSpanProcessorFactory::Create(std::move(exporter), options);
     }
 
@@ -1333,12 +1339,12 @@ void CTraceManager::initTracerProviderAndGlobalInternals(const IPropertyTree * t
         enableDefaultLogExporter = traceConfig->getPropBool("enableDefaultLogExporter", enableDefaultLogExporter);
     }
 
-    //if (enableDefaultLogExporter)
-    //{
+    if (enableDefaultLogExporter)
+    {
         //Simple option to create logging to the log file - primarily to aid developers.
-    //    std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> exporter = JLogSpanExporterFactory::Create(DEFAULT_SPAN_LOG_FLAGS);
-    //    processors.push_back(opentelemetry::sdk::trace::SimpleSpanProcessorFactory::Create(std::move(exporter)));
-    //}
+        std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> exporter = JLogSpanExporterFactory::Create(DEFAULT_SPAN_LOG_FLAGS);
+        processors.push_back(opentelemetry::sdk::trace::SimpleSpanProcessorFactory::Create(std::move(exporter)));
+    }
 
     opentelemetry::sdk::resource::ResourceAttributes resourceAtts = 
         {