From 227fd45ddb1363ad37a3be1bc3dd690761746fd8 Mon Sep 17 00:00:00 2001 From: Tim Klemm Date: Thu, 7 Nov 2024 08:16:14 -0500 Subject: [PATCH] Review feedback. --- esp/platform/application_config.cpp | 9 ++---- esp/platform/espp.cpp | 43 ++++++++++++++--------------- esp/platform/esptrace.h | 18 ++---------- helm/examples/esp/README.md | 24 ++++++++-------- 4 files changed, 39 insertions(+), 55 deletions(-) diff --git a/esp/platform/application_config.cpp b/esp/platform/application_config.cpp index f8a466e8d1c..a43d3b2fbd1 100644 --- 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 index 466062878a7..48f110e9fb5 100644 --- 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 &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 index 0b2780ce8b0..e854fe1f0d6 100644 --- a/esp/platform/esptrace.h +++ b/esp/platform/esptrace.h @@ -23,21 +23,9 @@ // 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 @@ -45,7 +33,7 @@ constexpr TraceFlags traceLevel = TraceFlags::LevelMask; // 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 espTraceOptions diff --git a/helm/examples/esp/README.md b/helm/examples/esp/README.md index 05bb4bc5994..2d4f6807b4a 100644 --- a/helm/examples/esp/README.md +++ b/helm/examples/esp/README.md @@ -16,16 +16,16 @@ 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 @@ -33,7 +33,7 @@ Set the defailt trace level in the process. Accepted values are: #### 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: @@ -41,7 +41,7 @@ For example, the `eclwatch` process might be configured to use detailed reportin 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 - + ... ```