Review feedback.

hpcc-systems · Nov 22, 2024 · 227fd45 · 227fd45
1 parent 76212fa
commit 227fd45
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 55 deletions.
diff --git a/esp/platform/application_config.cpp b/esp/platform/application_config.cpp
@@ -439,14 +439,11 @@ void setLDAPSecurityInWSAccess(IPropertyTree *legacyEsp, IPropertyTree *legacyLd
 
 // Copy trace flags from appEsp to legacyEsp. The source is expected to be an application's `esp`
 // configuration object. The destination is expected to be the `EspProcess` element.
-void copyTraceFlags(IPropertyTree *legacyEsp, IPropertyTree *appEsp)
+inline static void addTraceFlags(IPropertyTree *legacyEsp, IPropertyTree *appEsp)
 {
     IPropertyTree *traceFlags = appEsp->queryPropTree(propTraceFlags);
     if (traceFlags)
-    {
-        IPropertyTree *legacyTraceFlags = legacyEsp->addPropTree(propTraceFlags);
-        copyAttributes(legacyTraceFlags, traceFlags);
-    }
+        legacyEsp->setPropTree(propTraceFlags, traceFlags);
 }
 
 IPropertyTree *buildApplicationLegacyConfig(const char *application, const char* argv[])
@@ -482,6 +479,6 @@ IPropertyTree *buildApplicationLegacyConfig(const char *application, const char*
     IPropertyTree *appDirectories = appEspConfig->queryPropTree("directories");
     copyDirectories(legacyDirectories, appDirectories);
 
-    copyTraceFlags(legacyEsp, appEspConfig);
+    addTraceFlags(legacyEsp, appEspConfig);
     return legacy.getClear();
 }
diff --git a/esp/platform/espp.cpp b/esp/platform/espp.cpp
@@ -50,9 +50,6 @@
 #include "jmetrics.hpp"
 #include "workunit.hpp"
 #include "esptrace.h"
-#include "tokenserialization.hpp"
-
-static TokenDeserializer deserializer;
 
 using namespace hpccMetrics;
 
@@ -358,7 +355,7 @@ static void usage()
 
 IPropertyTree *buildApplicationLegacyConfig(const char *application, const char* argv[]);
 
-// Modified version of jlib's loadTraceFlags. The modification adds special handling for traceLevel.
+// Modified version of jlib's loadTraceFlags. The modification adds special handling for traceDetail
 //
 // Using loadTraceFlags, `traceStandard: true`, `traceStandard: false`, and `traceStandard: maybe`
 // are non-intuitively equivalent. The property name is the only thing that matters, with the
@@ -370,43 +367,45 @@ IPropertyTree *buildApplicationLegacyConfig(const char *application, const char*
 // should be discouraged, using `helm install --set` to change the level will add a new property
 // rather than changing the existing one.
 //
-// With loadEspTraceFlags, `traceLevel` is the only property that sets the trace level. The value
+// With loadEspTraceFlags, `traceDetail` is the only property that sets the trace level. The value
 // is both relevant and intuitive, and can be overridden with `helm install --set`.
 static TraceFlags loadEspTraceFlags(const IPropertyTree *ptree, const std::initializer_list<TraceOption> &optNames, TraceFlags dft)
 {
     for (const TraceOption& option: optNames)
     {
         VStringBuffer attrName("@%s", option.name);
-        const char* value = nullptr;
-        if (!(value = ptree->queryProp(attrName)))
+        const char* value = ptree->queryProp(attrName);
+        if (!value)
         {
             attrName.setCharAt(0, '_');
-            if (!(value = ptree->queryProp(attrName)))
+            value = ptree->queryProp(attrName);
+            if (!value)
                 continue;
         }
-        if (streq(value, "default")) // allow a configuration to explicitly request a default value
+        if (strieq(value, "default")) // allow a configuration to explicitly request a default value
             continue;
-        if (streq(option.name, propTraceLevel)) // non-Boolean traceLevel
+        if (option.value == traceDetail) // non-Boolean traceDetail
         {
-            unsigned level = unsigned(dft & TraceFlags::LevelMask);
             dft &= ~TraceFlags::LevelMask;
-            if (streq(value, "traceStandard"))
+            if (strieq(value, "standard"))
                 dft |= traceStandard;
-            else if (streq(value, "traceDetailed"))
+            else if (strieq(value, "detailed"))
                 dft |= traceDetailed;
-            else if (streq(value, "traceMax"))
+            else if (strieq(value, "max"))
                 dft |= traceMax;
-            else if (deserializer(value, level) == Deserialization_SUCCESS)
-                dft |= TraceFlags(level) & TraceFlags::LevelMask;
             else
-                dft |= TraceFlags(level);
+            {
+                char* endptr = nullptr;
+                unsigned tmp = strtoul(value, &endptr, 10);
+                if (endptr && !*endptr && TraceFlags(tmp) <= TraceFlags::LevelMask)
+                    dft |= TraceFlags(tmp) & TraceFlags::LevelMask;
+            }
         }
         else if (option.value <= TraceFlags::LevelMask) // block individual trace level names
             continue;
         else // Boolean trace options
         {
-            bool flag = false;
-            deserializer(value, flag);
+            bool flag = strToBool(value);
             if (flag)
                 dft |= option.value;
             else
@@ -429,10 +428,10 @@ void initializeTraceFlags(CEspConfig* config)
 #ifdef _DEBUG
     if (!pTraceTree)
     {
-        static const char * defaultTraceYaml = R"!!(
-traceLevel: traceMax
+        static const char * defaultTraceFlagsYaml = R"!!(
+traceDetail: max
 )!!";
-        pTraceTree.setown(createPTreeFromYAMLString(defaultTraceYaml));
+        pTraceTree.setown(createPTreeFromYAMLString(defaultTraceFlagsYaml));
     }
 #endif
 

diff --git a/esp/platform/esptrace.h b/esp/platform/esptrace.h
@@ -23,29 +23,17 @@
 // Refer to helm/examples/esp/README.md for more information on the trace options.
 
 constexpr const char* propTraceFlags = "traceFlags";
-constexpr const char* propTraceLevel = "traceLevel";
 
-// ESP process names for use with options mapping
-constexpr const char* eclwatchApplication = "eclwatch";
-constexpr const char* eclservicesApplication = "eclservices";
-constexpr const char* eclqueriesApplication = "eclqueires";
-constexpr const char* esdlsandboxApplication = "esdl-sandbox";
-constexpr const char* esdlApplication = "esdl";
-constexpr const char* sql2eclApplication = "sql2ecl";
-constexpr const char* dfsApplication = "dfs";
-constexpr const char* ldapenvironmentApplication = "ldapenvironment";
-constexpr const char* loggingserviceApplication = "loggingservice";
-
-// Trace options for ESP
-constexpr TraceFlags traceLevel = TraceFlags::LevelMask;
+// TODO: This can move to jtrace.hpp when loadTraceOptions is updated with ESP logic...
+constexpr TraceFlags traceDetail = TraceFlags(0xFFFFFFFF);
 
 // Trace option list fragment for jtrace-defined options used by ESPs
 #define PLATFORM_OPTIONS_FRAGMENT
 
 // Trace option list fragment for options used by most ESPs
 #define ESP_OPTIONS_FRAGMENT \
     PLATFORM_OPTIONS_FRAGMENT \
-    TRACEOPT(traceLevel),
+    TRACEOPT(traceDetail),
 
 // Trace option initializer list for ESPs that do not define their own options.
 constexpr std::initializer_list<TraceOption> espTraceOptions

diff --git a/helm/examples/esp/README.md b/helm/examples/esp/README.md
@@ -16,32 +16,32 @@ Flags will be added to this list as tracing logic is updated in ESP code. For ex
 
 Flags defined in `esp/platform/esptrace.h` and applicable to most, if not all, ESP configurations.
 
-#### traceLevel
+#### traceDetail
 
-Set the defailt trace level in the process. Accepted values are:
-- 1, traceStandard: most important output
-- 2, traceDetailed: average verbosity output
-- 3, traceMax: highest verbosity output
-- default: use existing level
-  - Release builds default to traceStandard
-  - Debug builds default to traceMax
-- 0, traceNone, *all other values*: no trace output
+Set the default trace level in the process. Accepted case-insensitive values are:
+- `1`, `standard`: most important output
+- `2`, `detailed`: average verbosity output
+- `3`, `max`: highest verbosity output
+- `default`: use existing level
+  - Release builds default to `standard`
+  - Debug builds default to `max`
+- `0`, `none`, *all other values*: no trace output
 
 ## Process Configuration
 
 ### Containerized
 
 #### esp.yaml
 
-Each ESP application's configuration object may embed one instance of a `traceFlags` object. Within this object, at most one property per [available flag]]#availableflags] is expected. Properties not described here are ignored.
+Each ESP application's configuration object may embed one instance of a `traceFlags` object. Within this object, at most one property per [accepted flag](#accepted-flags) is expected. Properties not described here are ignored.
 
 For example, the `eclwatch` process might be configured to use detailed reporting like this:
 
 ```yml
 esp:
 - name: eclwatch
   traceFlags:
-    traceLevel: 2
+    traceDetail: 2
 ```
 
 ## Cluster Overrides
@@ -56,7 +56,7 @@ The previous YAML example may be reproduced in XML with the following:
 
 ```xml
 <EspProcess ...>
-  <traceFlags traceLevel="2" />
+  <traceFlags traceDetail="2" />
   ...
 <EspProcess>
 ```