HPCC-31775 Hook jlog to use api to log operator messages #18685

shamser · 2024-05-21T12:42:16Z

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-05-21T12:54:10Z

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

Jirabot Action Result:

jakesmith

@shamser - looks good in general. Please see comments.

dali/base/sysinfologger.cpp

jakesmith · 2024-12-05T17:37:07Z

dali/base/daclient.cpp

@@ -141,6 +142,8 @@ bool initClientProcess(IGroup *servergrp, DaliClientRole role, unsigned mpport,
    covengrp->Release();
    queryLogMsgManager()->setSession(myProcessSession());

+    if (getGlobalConfigSP()->getPropBool("@enableGlobalSysLog", true))


are we sure want to default on yet?
should add a entry in values.schema.json and a commented out entry in values.yaml showing how it can be enabled (or disabled if enabled by default).

dali/base/daclient.cpp

dali/base/sysinfologger.hpp

jakesmith · 2024-12-05T17:49:39Z

esp/services/ws_workunits/ws_wudetails.cpp

+                id++;
+            }
+            Owned<ISysInfoLoggerMsgFilter> msgFilter = createSysInfoLoggerMsgFilter();
+            msgFilter->setVisibleOnly();


question: to be later extended so service can pass filter types?

Created a jira for this: https://hpccsystems.atlassian.net/browse/HPCC-33089

testing/unittests/dalitests.cpp

dali/base/sysinfologger.cpp

jakesmith · 2024-12-05T17:57:27Z

dali/base/sysinfologger.cpp

+    else
+    {
+        queryLogMsgManager()->removeMonitor(msgHandler);
+        msgHandler = NULL;


trivial: nullptr

jakesmith · 2024-12-05T18:05:45Z

dali/base/sysinfologger.cpp

+        ILogMsgFilter * operatorFilter = getCategoryLogMsgFilter(MSGAUD_operator,
+                                                                 MSGCLS_disaster|MSGCLS_error|MSGCLS_warning,
+                                                                 WarnMsgThreshold);
+        queryLogMsgManager()->addMonitorOwn(msgHandler, operatorFilter);


it's slightly unsafe to pass ownership to the handler, but keep a reference to it (in the static).
Should change so it calls addMonitor( not addMonitorOwn, and change the static to static Owned msgHandler.
Also better if the static declaration is outside of the function, because being inside causes the compiler (>= c++11) to wrap them with thread safety code (and overhead).

jakesmith

@shamser - please see follow comments/questions.

jakesmith · 2024-12-16T17:47:40Z

helm/hpcc/values.schema.json

+        },
+	"enableGlobalSysLog": {
+	  "type": "boolean",
+          "description" : "Enable the reporting of important user/operator messages in ECL Watch"


trivial: may not involve the UI (eclwatch), e.g. may programmatically get these messages from the esp service via soap. I'd rephrase to "Enable centralized reporting of important user/operator logging messages"

jakesmith · 2024-12-16T17:49:45Z

helm/hpcc/values.yaml

@@ -24,6 +24,9 @@ global:
  # logging sets the default logging information for all components. Can be overridden locally
  logging:
    detail: 80
+    # GlobalSysLog records user/operator messages to dali so that messages are reported in ECL Watch


trivial: EclWatch may not be the only consumer/client of the service - I'd reword to convey that it is accessible via service (used by EclWatch)

jakesmith · 2024-12-16T17:52:03Z

dali/base/daclient.cpp

@@ -142,8 +142,9 @@ bool initClientProcess(IGroup *servergrp, DaliClientRole role, unsigned mpport,
    covengrp->Release();
    queryLogMsgManager()->setSession(myProcessSession());

-    if (getGlobalConfigSP()->getPropBool("@enableGlobalSysLog", true))
-        UseDaliForOperatorMessages();
+    if (getComponentConfigSP()->getPropBool("logging/@enableGlobalSysLog") || getGlobalConfigSP()->getPropBool("logging/@enableGlobalSysLog"))


this should be:

if (getComponentConfigSP()->getPropBool("logging/@enableGlobalSysLog", getGlobalConfigSP()->getPropBool("logging/@enableGlobalSysLog")))

so that the global property is the default, but the component can override, i.e. including setting it to false.

jakesmith · 2024-12-16T18:13:39Z

esp/services/ws_workunits/ws_wudetails.cpp

@@ -502,6 +510,27 @@ void WUDetails::processRequest(IEspWUDetailsRequest &req, IEspWUDetailsResponse
                    espWuResponseNote->setErrorCode_null();
                espWuResponseNote->setSeverity(cur.queryProp("@severity"));
                espWuResponseNote->setCost(0);
+                VStringBuffer idstr("%" I64F "u", id|COMPONENT_NOTE_MASK);


The id's here, being returned in setId (WuResponseNote::Id), they're 64-bit ..
For the non-static warnings, they will have been created by makeMessageId ? i.e. the bottom 21 bits correspond to the year/month/day, the id lies in the top bits (id<<21).
Is that correct?

If so, then the id's created here for static's should follow the same format (and use makeMessageId). Otherwise these 32-bit id's with the top-bit set will always decodeMessageId to an id of 0, and some bits set for the date.

And, I think COMPONENT_NOTE_MASK should move to sysinfologger.hpp, and be used as the upper limit to wrap on instead of the current UINT_MAX.

jakesmith · 2024-12-16T18:30:23Z

dali/base/sysinfologger.cpp

+        if (!msgHandler)
+        {
+            msgHandler.setown(getDaliMsgLoggerHandler());
+            ILogMsgFilter * operatorFilter = getCategoryLogMsgFilter(MSGAUD_operator,


please check, but this looks like it leaks ? addMonitor (via LogMsgMonitor ctor) links both the handler and the filter.

Regarding id's: I agree that different uses of the id may cause confusion and may lead to bugs in the future. I have created a PR to distinguish between message id's and "sequence numbers". Should this be merged first: #19367 @jakesmith

Yes, it looks like a leak.

jakesmith · 2024-12-16T18:34:25Z

esp/services/ws_workunits/ws_wudetails.cpp

+            catch(IException *e)
+            {
+                // Non-existant global messages may mean that /SysInfoLogs hasn't been created
+                // so catch and ignore the exception


This exception handler looks too broad, catching and silently ignoring any exception from the block of code above.
Would it make more sense to go around createSysInfoLoggerMsgIterator only?

And/or should createSysInfoLoggerMsgIterator be not throw an error if no messages yet - but instead return an iterator that returns none ?

The conclusion from an offline chat was to allow createSysInfoLoggerMsgIterator to return a null iterator, i.e. do not throw an exception if SYS_INFO_ROOT doesn't exist (i.e. if there have not been any messages yet).
I think there should be a single static factory method that makes the IRemoteConnection to SYS_INFO_ROOT, if it doesn't exist it creates/returns a CNullMsgIterator, if the connection is made, then it passes it to a ctor of CSysInfoLoggerMsgIterator.

This can be left to a subsequent JIRA/PR. @shamser - please open one and reference/link it to this one.

Use id or msgid for unique SysInfoLogger message identifier Use SeqNum for the sequence number used to the generated ensure id/msgid is unique Signed-off-by: Shamser Ahmed <[email protected]>

Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue31775 branch 2 times, most recently from 2a91c83 to 868c78c Compare May 22, 2024 12:36

shamser force-pushed the issue31775 branch 2 times, most recently from fa54e96 to 7e305d4 Compare May 30, 2024 12:52

shamser force-pushed the issue31775 branch from 7e305d4 to 1f03c93 Compare October 11, 2024 12:51

shamser changed the base branch from master to candidate-9.8.x October 11, 2024 12:52

shamser marked this pull request as ready for review October 11, 2024 12:52

shamser requested a review from jakesmith October 11, 2024 12:53

shamser force-pushed the issue31775 branch from 1f03c93 to 2cf2397 Compare November 20, 2024 12:42

shamser changed the base branch from candidate-9.8.x to master November 20, 2024 12:42

jakesmith reviewed Dec 5, 2024

View reviewed changes

shamser force-pushed the issue31775 branch from 2cf2397 to acb2cc7 Compare December 9, 2024 13:13

shamser requested a review from jakesmith December 9, 2024 13:14

jakesmith reviewed Dec 16, 2024

View reviewed changes

shamser added 4 commits January 10, 2025 15:23

HPCC-33142 Distinguish between id and sequence number in SysInfoLogger

d4940a7

Use id or msgid for unique SysInfoLogger message identifier Use SeqNum for the sequence number used to the generated ensure id/msgid is unique Signed-off-by: Shamser Ahmed <[email protected]>

HPCC-31775 Hook jlog to use api to log operator messages

93b9c4b

Signed-off-by: Shamser Ahmed <[email protected]>

HPCC-31775 Changes following review

2861a84

Signed-off-by: Shamser Ahmed <[email protected]>

HPCC-31775 Changes following review

b0f5701

Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue31775 branch from acb2cc7 to b0f5701 Compare January 13, 2025 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-31775 Hook jlog to use api to log operator messages #18685

HPCC-31775 Hook jlog to use api to log operator messages #18685

shamser commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024

jakesmith left a comment

jakesmith Dec 5, 2024

jakesmith Dec 5, 2024

shamser Dec 6, 2024

jakesmith Dec 5, 2024

jakesmith Dec 5, 2024

jakesmith left a comment

jakesmith Dec 16, 2024

jakesmith Dec 16, 2024

jakesmith Dec 16, 2024

jakesmith Dec 16, 2024

jakesmith Dec 16, 2024

shamser Dec 18, 2024

shamser Dec 18, 2024

jakesmith Dec 16, 2024

jakesmith Dec 17, 2024 •

edited

Loading

HPCC-31775 Hook jlog to use api to log operator messages #18685

Are you sure you want to change the base?

HPCC-31775 Hook jlog to use api to log operator messages #18685

Conversation

shamser commented May 21, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented May 21, 2024

jakesmith 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

jakesmith 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

jakesmith Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

shamser commented May 21, 2024 •

edited

Loading

jakesmith Dec 17, 2024 •

edited

Loading