HPCC-32465 Add ESP support for trace level #19246

timothyklemm · 2024-10-29T14:38:11Z

Define an ESP process configuration node that supports specification of global TraceFlags values for each ESP.
Reserve traceDetail to request a specific trace level (0: none, 1: standard, 2: detailed, 3: max). This replaces acting on the last observed occurrence of either traceNone, traceStandard, traceDetailed, and traceMax.
Override default flag settings when not configured and a debug build.

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2024-10-29T14:38:32Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32465

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

timothyklemm · 2024-10-31T01:35:45Z

@ghalliday you'll notice I've defined my own version of loadTraceFlags for the ESP. I opened https://hpccsystems.atlassian.net/browse/HPCC-32477 to potentially change the jlib implementation. This PR implements the change that Jira suggests.

Also, #19007 is a draft that is stacked on top of this, if you want to see a bigger example of this change in use.

ghalliday

@timothyklemm The change makes sense, but better would be to enhance the existing loadTraceFlags with a consistent super-set of the functionality. A few other comments on the code as it stands.

ghalliday · 2024-11-04T09:51:01Z

esp/platform/application_config.cpp

+void copyTraceFlags(IPropertyTree *legacyEsp, IPropertyTree *appEsp)
+{
+    IPropertyTree *traceFlags = appEsp->queryPropTree(propTraceFlags);
+    if (traceFlags)


minor: Possibly simpler as

if (traceFlags) legacyEsp->setPropTree(propTraceFlags, LINK(match));

(This copyTree function should be moved to jptree.hpp...)

The simplification of this method is straightforward. Adding a function to jptree ks less so. This function, making assumptions about source and target nodes, is too specialized to belong in jlib. My first thought for generalizing it involves target node, source node, and source path, with the copied node always added as a child of the target. This might not be general enough, if someone might want to specify a target path as well.

If a generalized copy function should be part of this PR, what would your expectation be for its interface/behavior?

There is an existing copyTree function that is clone in workunit.cpp and espp.cpp. It would make sense for this to be migrated into jptree, and for this function to make use of it. Not for this PR though.

ghalliday · 2024-11-04T09:53:53Z

esp/platform/espp.cpp

+    {
+        VStringBuffer attrName("@%s", option.name);
+        const char* value = nullptr;
+        if (!(value = ptree->queryProp(attrName)))


style: We generally avoid assignments inside if statements - they are hard to read.

Will change.

ghalliday · 2024-11-04T09:58:55Z

esp/platform/espp.cpp

+        else // Boolean trace options
+        {
+            bool flag = false;
+            deserializer(value, flag);


minor: Code elsewhere would use strToBool(value). Does the deserializer provide other benefits?

I already had the deserializer for integral trace levels. Just reused it for consistency in the method. Moving logic out to jlib might make the deserializer unpalatable in both cases since its definition is in an ESP header.

ghalliday · 2024-11-04T10:00:20Z

esp/platform/espp.cpp

+        {
+            unsigned level = unsigned(dft & TraceFlags::LevelMask);
+            dft &= ~TraceFlags::LevelMask;
+            if (streq(value, "traceStandard"))


tracelevel=standard

would be more intuitive than

tracelevel=traceStandard

also, case insensitive comparison would be more user friendly.

ghalliday · 2024-11-04T10:03:20Z

esp/platform/esptrace.h

+// ESP process names for use with options mapping
+constexpr const char* eclwatchApplication = "eclwatch";
+constexpr const char* eclservicesApplication = "eclservices";
+constexpr const char* eclqueriesApplication = "eclqueires";


Unused, and has a typo in the string.

I suppose the definitions are premature. I defined them so mapTraceOptions will eventually be able to use different option lists for distinct ESPs. Not sure how many new options will be defined for most platform ESPs, but I have a lot in mind for esdl and esdl-sandbox (when/if we return focus to DESDL) that might also extend into loggingservice.

Are you asking for removal until needed, or is fixing the type sufficient?

I don't mind, probably cleaner to remove.
The danger with adding constants or other items that are not actually used, is that when they come to be used they are not checked, because they are assumed to be correct because they are already checked in, but equally there is a danger that items that are not used aren't checked as carefully when they are merged.

ghalliday · 2024-11-04T10:05:03Z

esp/platform/esptrace.h

+constexpr const char* loggingserviceApplication = "loggingservice";
+
+// Trace options for ESP
+constexpr TraceFlags traceLevel = TraceFlags::LevelMask;


This looks is confusing. Why have a constant called traceLevel which is actually a bit mask? It is likely to lead to confusion if there was code which said
if (traceLevel == ....)

Given my use of TRACEOPT, which is its own issue, I needed a flag named traceLevel to build the option. I assigned the mask instead of an individual level hoping it would be clear that the property wasn't tied to that single value. Avoiding TRACEOPT makes this unnecessary.

ghalliday · 2024-11-04T10:16:23Z

esp/platform/esptrace.h

+// Trace option list fragment for options used by most ESPs
+#define ESP_OPTIONS_FRAGMENT \
+    PLATFORM_OPTIONS_FRAGMENT \
+    TRACEOPT(traceLevel),


I think what you actually want to do is no use TRACEOPT, but use

{ "traceLevel", traceNone },

if you want to support traceLevel.

Will change.

ghalliday · 2024-11-04T10:19:51Z

esp/platform/espp.cpp

+        }
+        if (streq(value, "default")) // allow a configuration to explicitly request a default value
+            continue;
+        if (streq(option.name, propTraceLevel)) // non-Boolean traceLevel


This logic makes sense, but would probably make more sense added to loadTraceFlags.

I would add add an extra enumeration traceDetail[Level?] = 0xffffffff, and check that value with this processing:

if (option.value == traceDetail)...

(I am suggesting traceDetail rather than traceLevel to avoid clashes with the existing traceLevel option which has different ranges of values?)

Agree on loadTraceFlags. With multiple comments here, suggest we fix this here, then it will be easy to lift and shift into jlib.

I don't understand your intent for traceDetail. Would it replace the string comparison on the name, or would it be in addition to that?

It would replace the comparison on the name.

ghalliday · 2024-11-04T10:27:22Z

esp/platform/espp.cpp

+            else if (streq(value, "traceMax"))
+                dft |= traceMax;
+            else if (deserializer(value, level) == Deserialization_SUCCESS)
+                dft |= TraceFlags(level) & TraceFlags::LevelMask;


minor: range checking on level - this could be used to set any of the other flags as well. I suspect that should be blocked.

I don't think it will set any other flags given the application of the level mask, but it does contradict the README which states that all other values are equivalent to traceNone. Will add a range check, and remove the following else block that also contradicts the README by treating all other values as "default".

You are correct, I missed the level mask.

ghalliday · 2024-11-04T10:31:04Z

@timothyklemm I only read your comment about the related PR afterwards. I agree, and suggest you make the changes in the lib function.

timothyklemm

@ghalliday I'm clear on most of your feedback, but I do have a couple questions.

timothyklemm · 2024-11-04T12:37:37Z

esp/platform/esptrace.h

+constexpr const char* loggingserviceApplication = "loggingservice";
+
+// Trace options for ESP
+constexpr TraceFlags traceLevel = TraceFlags::LevelMask;


Given my use of TRACEOPT, which is its own issue, I needed a flag named traceLevel to build the option. I assigned the mask instead of an individual level hoping it would be clear that the property wasn't tied to that single value. Avoiding TRACEOPT makes this unnecessary.

timothyklemm · 2024-11-04T14:01:57Z

esp/platform/espp.cpp

+        else // Boolean trace options
+        {
+            bool flag = false;
+            deserializer(value, flag);


I already had the deserializer for integral trace levels. Just reused it for consistency in the method. Moving logic out to jlib might make the deserializer unpalatable in both cases since its definition is in an ESP header.

timothyklemm · 2024-11-04T14:16:37Z

esp/platform/espp.cpp

+            else if (streq(value, "traceMax"))
+                dft |= traceMax;
+            else if (deserializer(value, level) == Deserialization_SUCCESS)
+                dft |= TraceFlags(level) & TraceFlags::LevelMask;


I don't think it will set any other flags given the application of the level mask, but it does contradict the README which states that all other values are equivalent to traceNone. Will add a range check, and remove the following else block that also contradicts the README by treating all other values as "default".

timothyklemm · 2024-11-04T14:17:04Z

esp/platform/espp.cpp

+        {
+            unsigned level = unsigned(dft & TraceFlags::LevelMask);
+            dft &= ~TraceFlags::LevelMask;
+            if (streq(value, "traceStandard"))


timothyklemm · 2024-11-04T14:22:38Z

esp/platform/espp.cpp

+        }
+        if (streq(value, "default")) // allow a configuration to explicitly request a default value
+            continue;
+        if (streq(option.name, propTraceLevel)) // non-Boolean traceLevel


Agree on loadTraceFlags. With multiple comments here, suggest we fix this here, then it will be easy to lift and shift into jlib.

timothyklemm · 2024-11-04T14:26:13Z

esp/platform/espp.cpp

+        }
+        if (streq(value, "default")) // allow a configuration to explicitly request a default value
+            continue;
+        if (streq(option.name, propTraceLevel)) // non-Boolean traceLevel


I don't understand your intent for traceDetail. Would it replace the string comparison on the name, or would it be in addition to that?

timothyklemm · 2024-11-04T14:26:51Z

esp/platform/espp.cpp

+    {
+        VStringBuffer attrName("@%s", option.name);
+        const char* value = nullptr;
+        if (!(value = ptree->queryProp(attrName)))


Will change.

timothyklemm · 2024-11-04T14:27:43Z

esp/platform/esptrace.h

+// Trace option list fragment for options used by most ESPs
+#define ESP_OPTIONS_FRAGMENT \
+    PLATFORM_OPTIONS_FRAGMENT \
+    TRACEOPT(traceLevel),


Will change.

timothyklemm · 2024-11-04T14:41:03Z

esp/platform/esptrace.h

+// ESP process names for use with options mapping
+constexpr const char* eclwatchApplication = "eclwatch";
+constexpr const char* eclservicesApplication = "eclservices";
+constexpr const char* eclqueriesApplication = "eclqueires";


I suppose the definitions are premature. I defined them so mapTraceOptions will eventually be able to use different option lists for distinct ESPs. Not sure how many new options will be defined for most platform ESPs, but I have a lot in mind for esdl and esdl-sandbox (when/if we return focus to DESDL) that might also extend into loggingservice.

Are you asking for removal until needed, or is fixing the type sufficient?

timothyklemm · 2024-11-04T14:45:16Z

esp/platform/application_config.cpp

+void copyTraceFlags(IPropertyTree *legacyEsp, IPropertyTree *appEsp)
+{
+    IPropertyTree *traceFlags = appEsp->queryPropTree(propTraceFlags);
+    if (traceFlags)


timothyklemm · 2024-11-27T11:20:35Z

@ghalliday I've updated based on your earlier comments. Not sure what to make of the failing test... both GitHub and VS Code handle the internal link that the test claims to be invalid as expected.

ghalliday · 2024-11-27T14:54:41Z

@AttilaVamos please can you look at the broken link report. It is from a relative path. Is this a problem with the hyper-link checking?

ghalliday

Looks good. I think it is worth replacing/updating the loadTraceFlags function with this enhanced version.

ghalliday · 2024-11-28T12:44:35Z

esp/platform/application_config.cpp

+void copyTraceFlags(IPropertyTree *legacyEsp, IPropertyTree *appEsp)
+{
+    IPropertyTree *traceFlags = appEsp->queryPropTree(propTraceFlags);
+    if (traceFlags)


There is an existing copyTree function that is clone in workunit.cpp and espp.cpp. It would make sense for this to be migrated into jptree, and for this function to make use of it. Not for this PR though.

ghalliday · 2024-11-28T12:45:56Z

esp/platform/espp.cpp

+                char* endptr = nullptr;
+                unsigned tmp = strtoul(value, &endptr, 10);
+                if (endptr && !*endptr && TraceFlags(tmp) <= TraceFlags::LevelMask)
+                    dft |= TraceFlags(tmp) & TraceFlags::LevelMask;


trivial: no need for the mask any more, now the assign is protected.

ghalliday

Looks good. Please squash and I will merge.

timothyklemm · 2024-12-06T14:21:11Z

@ghalliday it's squashed.

ghalliday · 2024-12-09T12:05:52Z

@timothyklemm is the first commit required? Please can you separate that out into a separate PR - I would have some related comments that are unrelated to the tracing flags.

timothyklemm · 2024-12-09T12:38:29Z

Oops. There already is a separate PR for the first commit. The two commits are unrelated but combine to form the base for the latest version of the security manager decorator. I forgot to unstack them.

timothyklemm · 2024-12-09T12:52:00Z

@ghalliday now decoupled from the unrelated commit.

ghalliday

@timothyklemm one issue spotted on final review.

ghalliday · 2024-12-09T16:39:23Z

esp/platform/application_config.cpp

+{
+    IPropertyTree *traceFlags = appEsp->queryPropTree(propTraceFlags);
+    if (traceFlags)
+        legacyEsp->setPropTree(propTraceFlags, traceFlags);


This is missing a LINK(). I think it is likely to cause a crash when esp terminates.

@ghalliday made and squashed the one line change.

- Define an ESP process configuration node that supports specification of global TraceFlags values for each ESP. - Reserve traceDetail to request a specific trace level (0: none, 1: standard, 2: detailed, 3: max). This replaces acting on the last observed occurrence of either traceNone, traceStandard, traceDetailed, and traceMax. - Override default flag settings when not configured and a debug build. Signed-off-by: Tim Klemm <tim.klemm@lexisnexisrisk.com>

github-actions · 2024-12-18T14:18:11Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32465

Jirabot Action Result:
Workflow Transition To: Merge Pending

github-actions · 2025-01-06T15:27:23Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32465

Jirabot Action Result:
Workflow Transition To: Merge Pending

ghalliday · 2025-01-06T15:27:30Z

Close and reopened to force all tests to be rerun (to check failures really are false positives)

github-actions · 2025-01-09T16:59:56Z

Jirabot Action Result:
Added fix version: 9.10.0
Workflow Transition: 'Resolve issue'

timothyklemm requested a review from ghalliday October 31, 2024 01:30

ghalliday requested changes Nov 4, 2024

View reviewed changes

timothyklemm commented Nov 4, 2024

View reviewed changes

timothyklemm force-pushed the hpcc-32465-esp-global-trace branch 2 times, most recently from 227fd45 to 8a615da Compare November 22, 2024 20:54

timothyklemm requested a review from ghalliday November 27, 2024 11:18

ghalliday reviewed Nov 28, 2024

View reviewed changes

timothyklemm requested a review from ghalliday December 4, 2024 13:52

ghalliday approved these changes Dec 6, 2024

View reviewed changes

timothyklemm force-pushed the hpcc-32465-esp-global-trace branch from 67ec81a to 7e44732 Compare December 6, 2024 12:59

timothyklemm force-pushed the hpcc-32465-esp-global-trace branch from 7e44732 to caa4517 Compare December 9, 2024 12:42

ghalliday reviewed Dec 9, 2024

View reviewed changes

timothyklemm force-pushed the hpcc-32465-esp-global-trace branch from caa4517 to 2ad17a1 Compare December 9, 2024 17:58

ghalliday closed this Dec 18, 2024

ghalliday reopened this Dec 18, 2024

ghalliday closed this Jan 6, 2025

ghalliday reopened this Jan 6, 2025

ghalliday merged commit eb69d88 into hpcc-systems:master Jan 9, 2025
145 of 157 checks passed

HPCC-32465 Add ESP support for trace level #19246

HPCC-32465 Add ESP support for trace level #19246

Conversation

timothyklemm commented Oct 29, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Oct 29, 2024

timothyklemm commented Oct 31, 2024 • edited Loading

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Nov 4, 2024

timothyklemm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyklemm commented Nov 27, 2024

ghalliday commented Nov 27, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

timothyklemm commented Dec 6, 2024

ghalliday commented Dec 9, 2024

timothyklemm commented Dec 9, 2024

timothyklemm commented Dec 9, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

github-actions bot commented Jan 6, 2025

ghalliday commented Jan 6, 2025

github-actions bot commented Jan 9, 2025

timothyklemm commented Oct 29, 2024 •

edited

Loading

timothyklemm commented Oct 31, 2024 •

edited

Loading