HPCC-32615 review Thor jlog logging audiences/levels #19099

jakesmith · 2024-09-10T12:44:04Z

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-09-10T12:44:26Z

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

Jirabot Action Result:
Assigning user: [email protected]
Workflow Transition To: Merge Pending
Updated PR

jakesmith · 2024-09-11T08:38:04Z

@ghalliday - consider this stage 1. I've opened another [stage 2] subtask : https://hpccsystems.atlassian.net/browse/HPCC-32646, to look at migrating worker progress logging to a programmer audience.

ghalliday

Generally looks good. A few questions about the new categories.

I think it would be a good time to go through all the legacy macro names and change them so they are explicit. Probably something to do one macro at a time

ghalliday · 2024-09-12T14:00:40Z

thorlcr/activities/thdiskbaseslave.cpp

@@ -119,7 +119,7 @@ void CDiskPartHandlerBase::open()
    {
        StringBuffer locations;
        IException *e = MakeActivityException(&activity, TE_FileNotFound, "No physical file part for logical file %s, found at given locations: %s (Error = %d)", activity.logicalFilename.get(), getFilePartLocations(*partDesc, locations).str(), GetLastError());
-        EXCLOG(e, NULL);
+        ERRLOG(e, "CDiskPartHandlerBase::open()");


Should really be UERRLOG - ERRLOG is deprecated from the jlog comments.

Looks like you made this comment on the 1st commit, it was changed from ERRLOG to IERRLOG in the 3rd commit.
I thought programmer audience vs user is appropriate, since the exception is being fired and propagated back to the user.

ghalliday · 2024-09-12T15:07:23Z

thorlcr/graph/thgraphmaster.cpp

@@ -2145,13 +2145,13 @@ bool CJobMaster::fireException(IException *e)
    {
        case tea_warning:
        {
-            LOG(MCwarning, e);
+            LOG(MCprogress, e);


We should kill MCerror, MCwarning and MCprogress, e.g use MCuserProgress - so it is clear what the target audience is.

I will go through their uses in Thor and transition them

ghalliday · 2024-09-12T15:09:10Z

thorlcr/slave/thslavemain.cpp

@@ -117,7 +117,7 @@ static bool RegisterSelf(SocketEndpoint &masterEp)
 {
    StringBuffer slfStr;
    StringBuffer masterStr;
-    LOG(MCdebugProgress, "registering %s - master %s",slfEp.getEndpointHostText(slfStr).str(),masterEp.getEndpointHostText(masterStr).str());
+    PROGLOG("registering %s - master %s",slfEp.getEndpointHostText(slfStr).str(),masterEp.getEndpointHostText(masterStr).str());


Is this something that should go to the user? Should really be named UPROGLOG to make it clear..

I'm going to introduce UPROGLOG (we have same for UERRLOG vs ERRLOG already) and rename all PROGLOG's to UPROGLOG in Thor.

I think it is debatable whether many current user progress logs should be user or programmer (or perhaps a different audience). If an advanced user wants to see the engine making progress, then some lines like these are useful - but for average ECL user, very few are meaningful..

It would be worth us clearly defining which messages should go to which targets.
It would also make more sense if D was used as the prefixed for debug/programmers rather than I. That would make the functions more consistent. I will take a look at what is there already.

ghalliday · 2024-09-23T10:17:44Z

@jakesmith

jakesmith

@ghalliday - please see new commits.

ghalliday

I should have been clearer that some suggestions should have been for later PRs.
This is now in danger of being too large to merge. Can we split this into 2 so the first set of changes can be reviewed and merged twice.

ghalliday · 2024-09-25T09:21:02Z

thorlcr/master/thgraphmanager.cpp

@@ -621,24 +621,24 @@ void CJobManager::run()
        {
            int p = getRunningMaxPriority(qn);
            if (p)
-                PROGLOG("Dynamic Min priority = %d",p);
+                UPROGLOG("Dynamic Min priority = %d",p);


Are these UPROGLOG messages really likely to be useful to the user? I think worth looking at all of them.

ghalliday · 2024-09-25T09:22:07Z

thorlcr/slave/slavmain.cpp

@@ -63,7 +63,7 @@ bool recvShutdown = false;
 void enableThorSlaveAsDaliClient()
 {
 #ifdef ISDALICLIENT
-    PROGLOG("Slave activated as a Dali client");
+    UPROGLOG("Slave activated as a Dali client");


User not even likely to be able to see this - even if it was relevant.

I knew that the existing PROGLOG (now all UPROGLOG's) would need reviewing beyond this, but as part of this PR's move away from deprecated macros to clarify how they are currently being used (the audience) I did a blanket s&r - reviewing/pruning of which UPROGLOG's we actually want to keep as user audience can come later.

I should have been clearer that some suggestions should have been for later PRs.
This is now in danger of being too large to merge. Can we split this into 2 so the first set of changes can be reviewed and merged twice.

@ghalliday - have rolled back to the original set of changes, and create a new subtask (https://hpccsystems.atlassian.net/browse/HPCC-32727) as a placeholder for the next changes, which I'll attach the other changes to soon.

ghalliday

Looks reasonable. Do you want to squash before merging?

ghalliday · 2024-09-30T09:30:58Z

thorlcr/slave/thslavemain.cpp

@@ -117,7 +117,7 @@ static bool RegisterSelf(SocketEndpoint &masterEp)
 {
    StringBuffer slfStr;
    StringBuffer masterStr;
-    LOG(MCdebugProgress, "registering %s - master %s",slfEp.getEndpointHostText(slfStr).str(),masterEp.getEndpointHostText(masterStr).str());
+    PROGLOG("registering %s - master %s",slfEp.getEndpointHostText(slfStr).str(),masterEp.getEndpointHostText(masterStr).str());


It would be worth us clearly defining which messages should go to which targets.
It would also make more sense if D was used as the prefixed for debug/programmers rather than I. That would make the functions more consistent. I will take a look at what is there already.

jakesmith · 2024-09-30T10:43:48Z

It would be worth us clearly defining which messages should go to which targets.
It would also make more sense if D was used as the prefixed for debug/programmers rather than I. That would make the functions more consistent. I will take a look at what is there already.

Agreed. The I->D change may make sense to include in the pending 'stage2' PR.
Then another subtask to revisit which of these 'progress' logs should be switched to which audience targets.

jakesmith · 2024-09-30T10:48:15Z

@ghalliday - squashed

jakesmith · 2024-10-03T12:30:42Z

@ghalliday - this is approved, but I notice now it needs rebasing. Will do now.

Standarize on DBGLOG/[I]WARNLOG/[I]ERRLOG vs LOG(MCdebugProgress & LOG(MCwarning, etc. Change some PROGLOG's to better audiences (many to DBGLOG) Signed-off-by: Jake Smith <[email protected]>

jakesmith · 2024-10-03T12:37:49Z

@ghalliday - this is approved, but I notice now it needs rebasing. Will do now.

@ghalliday - now rebased to resolve conflict.

jakesmith requested a review from ghalliday September 10, 2024 13:49

ghalliday reviewed Sep 12, 2024

View reviewed changes

jakesmith commented Sep 24, 2024

View reviewed changes

jakesmith requested a review from ghalliday September 24, 2024 11:00

ghalliday reviewed Sep 26, 2024

View reviewed changes

jakesmith force-pushed the HPCC-32615-exclog-audience branch from 736e1a0 to dcaec1b Compare September 26, 2024 09:21

jakesmith requested a review from ghalliday September 26, 2024 09:22

ghalliday approved these changes Sep 30, 2024

View reviewed changes

jakesmith force-pushed the HPCC-32615-exclog-audience branch from dcaec1b to 5f76167 Compare September 30, 2024 10:45

HPCC-32615 review Thor EXCLOG logging

ebdd3a6

Standarize on DBGLOG/[I]WARNLOG/[I]ERRLOG vs LOG(MCdebugProgress & LOG(MCwarning, etc. Change some PROGLOG's to better audiences (many to DBGLOG) Signed-off-by: Jake Smith <[email protected]>

jakesmith force-pushed the HPCC-32615-exclog-audience branch from 5f76167 to ebdd3a6 Compare October 3, 2024 12:37

ghalliday merged commit 1848747 into hpcc-systems:master Oct 3, 2024
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32615 review Thor jlog logging audiences/levels #19099

HPCC-32615 review Thor jlog logging audiences/levels #19099

jakesmith commented Sep 10, 2024 •

edited

Loading

github-actions bot commented Sep 10, 2024

jakesmith commented Sep 11, 2024

ghalliday left a comment

ghalliday Sep 12, 2024

jakesmith Sep 24, 2024

ghalliday Sep 12, 2024

jakesmith Sep 24, 2024 •

edited

Loading

ghalliday Sep 12, 2024

jakesmith Sep 24, 2024

ghalliday Sep 30, 2024

ghalliday commented Sep 23, 2024

jakesmith left a comment

ghalliday left a comment

ghalliday Sep 25, 2024

ghalliday Sep 25, 2024

jakesmith Sep 26, 2024

jakesmith Sep 26, 2024

ghalliday left a comment

ghalliday Sep 30, 2024

jakesmith commented Sep 30, 2024

jakesmith commented Sep 30, 2024

jakesmith commented Oct 3, 2024

jakesmith commented Oct 3, 2024

HPCC-32615 review Thor jlog logging audiences/levels #19099

HPCC-32615 review Thor jlog logging audiences/levels #19099

Conversation

jakesmith commented Sep 10, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Sep 10, 2024

jakesmith commented Sep 11, 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

jakesmith Sep 24, 2024 • edited Loading

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 Sep 23, 2024

jakesmith left a comment

Choose a reason for hiding this comment

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

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Sep 30, 2024

jakesmith commented Sep 30, 2024

jakesmith commented Oct 3, 2024

jakesmith commented Oct 3, 2024

jakesmith commented Sep 10, 2024 •

edited

Loading

jakesmith Sep 24, 2024 •

edited

Loading