HPCC-35558 Standardize logaccess plugin load logic #20792

rpastrana · 2026-01-05T23:37:47Z

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 · 2026-01-05T23:38:08Z

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

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

Copilot

Pull request overview

This PR standardizes the logic for loading logaccess plugins by introducing a diagnostic framework to better identify and report plugin loading failures. The key changes include adding an enum and struct for diagnostic states, a new diagnostic function that safely attempts plugin loading without raising exceptions, and enhanced error reporting in the WsLogAccess health report endpoint.

Added diagnostic infrastructure (enum and struct) to track plugin loading states
Created diagnoseLogAccessPluginLoad() function for safe plugin loading diagnostics
Enhanced health report endpoint with detailed diagnostic messages based on loading state

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
system/jlib/jlog.hpp	Added `LogAccessDiagnosticState` enum, `LogAccessPluginDiagnostics` struct, and `diagnoseLogAccessPluginLoad()` function declaration for diagnostic infrastructure
system/jlib/jlog.cpp	Refactored `queryRemoteLogAccessor()` to use consistent plugin naming and implemented new `diagnoseLogAccessPluginLoad()` diagnostic function
esp/services/ws_logaccess/WsLogAccessService.cpp	Enhanced `onGetHealthReport()` to use diagnostic framework and provide detailed, state-specific error messages

system/jlib/jlog.cpp

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

system/jlib/jlog.cpp

Copilot · 2026-01-06T03:57:10Z

system/jlib/jlog.cpp

+        e->errorMessage(msg);
+        diagnostics.statusMessage.setf("%s: Exception during plugin diagnostics: %s", methodName, msg.str());
+        e->Release();
+    }
+    catch (...)
+    {


The outer exception handler at lines 3567-3572 swallows the exception after logging it to diagnostics.statusMessage. If getGlobalConfigSP()->getPropTree() throws an exception, this is caught but the function continues to execute without setting any diagnostic state. Consider setting a diagnostic state (such as LoadFailed) when exceptions are caught at the outer level to ensure diagnostics.logAccessPluginLoadState is always set to a meaningful value before returning.

Suggested change

e->errorMessage(msg);

diagnostics.statusMessage.setf("%s: Exception during plugin diagnostics: %s", methodName, msg.str());

e->Release();

}

catch (...)

{

e->errorMessage(msg);

diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadFailed;

diagnostics.statusMessage.setf("%s: Exception during plugin diagnostics: %s", methodName, msg.str());

e->Release();

}

catch (...)

{

diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadFailed;

Copilot · 2026-01-06T03:57:10Z

system/jlib/jlog.cpp

+        e->errorMessage(msg);
+        diagnostics.statusMessage.setf("%s: Exception during plugin diagnostics: %s", methodName, msg.str());
+        e->Release();
+    }
+    catch (...)
+    {


Similarly, the catch-all handler at lines 3574-3577 logs an error but doesn't set a diagnostic state. This means if an unknown exception occurs, diagnostics.logAccessPluginLoadState will remain at its default value of ConfigNotFound (from line 1813), which is misleading. Set logAccessPluginLoadState to LoadFailed or another appropriate state before returning.

Suggested change

e->errorMessage(msg);

diagnostics.statusMessage.setf("%s: Exception during plugin diagnostics: %s", methodName, msg.str());

e->Release();

}

catch (...)

{

e->errorMessage(msg);

diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadFailed;

diagnostics.statusMessage.setf("%s: Exception during plugin diagnostics: %s", methodName, msg.str());

e->Release();

}

catch (...)

{

diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadFailed;

Copilot · 2026-01-06T03:57:10Z

esp/services/ws_logaccess/WsLogAccessService.cpp

+        switch (diagnostics.logAccessPluginLoadState)
+        {
+            case LogAccessDiagnosticState::ConfigNotFound:
+            {
+                messages.append("Warning: No logaccess plugin configuration found.");
+                code.set("Warning");
+                break;
+            }
+            case LogAccessDiagnosticState::ConfigFoundNoType:
+            {
+                VStringBuffer configTypeMsg("LogAccess configuration found but plugin type not specified: '%s'", diagnostics.statusMessage.str());
+                messages.append(configTypeMsg.str());
+                code.set("Fail");
+                break;
+            }
+            case LogAccessDiagnosticState::LoadFailed:
+            {
+                // Configuration exists, report on load attempt
+                VStringBuffer configMsg("LogAccess configuration found - Plugin Type: '%s'", diagnostics.pluginType.str());
+                messages.append(configMsg.str());
+
+                VStringBuffer failMsg("Failed to verify plugin '%s': %s", diagnostics.libName.str(), diagnostics.statusMessage.str());
+                messages.append(failMsg.str());
+                code.set("Fail");
+                break;
+            }
+            case LogAccessDiagnosticState::LoadSucceeded:
+            {
+                VStringBuffer configMsg("LogAccess configuration found - Plugin Type: '%s'", diagnostics.pluginType.str());
+                messages.append(configMsg.str());
+                VStringBuffer successMsg("Plugin library '%s' and factory procedure verified successfully. Plugin instance failed to initialize during service startup - check service logs for initialization errors.", diagnostics.libName.str());
+                messages.append(successMsg.str());
+                code.set("Warning");
+                break;
+            }
+            default:
+            {
+                messages.append("Unknown diagnostic state encountered");
+                code.set("Fail");
+                break;
+            }
+        }
+
+        // Add configuration details if requested
+        if (options.IncludeConfiguration)
+        {
+            const char* stateStr = "";
+            switch (diagnostics.logAccessPluginLoadState)
+            {
+                case LogAccessDiagnosticState::ConfigNotFound:
+                    stateStr = "ConfigNotFound";
+                    break;
+                case LogAccessDiagnosticState::ConfigFoundNoType:
+                    stateStr = "ConfigFoundNoType";
+                    break;
+                case LogAccessDiagnosticState::LoadFailed:
+                    stateStr = "LoadFailed";
+                    break;
+                case LogAccessDiagnosticState::LoadSucceeded:
+                    stateStr = "LoadSucceeded";
+                    break;
+                default:
+                    stateStr = "Unknown";
+                    break;
+            }


There's code duplication in the two switch statements at lines 410-451 and 457-474 that handle LogAccessDiagnosticState enum values. Both switches convert the same enum values to strings/messages. Consider extracting a helper function that converts LogAccessDiagnosticState to a string representation to reduce duplication and improve maintainability. This would also ensure consistency if new states are added to the enum.

esp/services/ws_logaccess/WsLogAccessService.cpp

Copilot · 2026-01-06T03:57:11Z

system/jlib/jlog.cpp

+void diagnoseLogAccessPluginLoad(LogAccessPluginDiagnostics & diagnostics)
+{
+    constexpr const char * methodName = "diagnoseLogAccessPluginLoad";
+    constexpr const char * instFactoryName = "createInstance";
+
+    try
+    {
+        Owned<IPropertyTree> logAccessPluginConfig = getGlobalConfigSP()->getPropTree("logAccess");
+
+        if (!logAccessPluginConfig)
+        {
+            diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::ConfigNotFound;
+            diagnostics.statusMessage.setf("%s: No logaccess configuration found in global config", methodName);
+            return;
+        }
+
+        logAccessPluginConfig->getProp("@type", diagnostics.pluginType);
+        if (diagnostics.pluginType.isEmpty())
+        {
+            diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::ConfigFoundNoType;
+            diagnostics.statusMessage.setf("%s: RemoteLogAccess plugin type not specified in configuration", methodName);
+            return;
+        }
+
+        StringBuffer pluginName;
+        pluginName.append(diagnostics.pluginType.str()).append("logaccess");
+        diagnostics.libName.set(pluginName.str());
+
+        //Attempt to load the DLL/SO
+        HINSTANCE logAccessPluginLib = nullptr;
+        try
+        {
+            logAccessPluginLib = LoadSharedObject(pluginName.str(), false, false); // raiseOnError=false to capture error
+            if (!logAccessPluginLib)
+            {
+                diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadFailed;
+                diagnostics.statusMessage.setf("%s: Failed to load plugin '%s' (%s%s%s): Library not found or load failed", methodName, pluginName.str(), SharedObjectPrefix, pluginName.str(), SharedObjectExtension);
+                return;
+            }
+
+            // Try to get the factory procedure
+            void * xproc = GetSharedProcedure(logAccessPluginLib, instFactoryName);
+            if (xproc == nullptr)
+            {
+                diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadFailed;
+                diagnostics.statusMessage.setf("%s: Cannot locate procedure '%s' in library '%s'", methodName, instFactoryName, diagnostics.libName.str());
+                FreeSharedObject(logAccessPluginLib);
+                return;
+            }
+
+            diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadSucceeded;
+            diagnostics.statusMessage.setf("%s: Plugin library and factory procedure verified successfully", methodName);
+            FreeSharedObject(logAccessPluginLib);
+        }
+        catch (IException *e)
+        {
+            diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadFailed;
+            StringBuffer errorMsg;
+            e->errorMessage(errorMsg);
+            diagnostics.statusMessage.setf("%s: Failed to load or verify plugin library '%s': %s", methodName, diagnostics.libName.str(), errorMsg.str());
+            if (logAccessPluginLib)
+                FreeSharedObject(logAccessPluginLib);
+            e->Release();
+            return;
+        }
+        catch (...)
+        {
+            diagnostics.logAccessPluginLoadState = LogAccessDiagnosticState::LoadFailed;
+            diagnostics.statusMessage.setf("%s: Failed to load or verify plugin library '%s': Unknown error", methodName, diagnostics.libName.str());
+            if (logAccessPluginLib)
+                FreeSharedObject(logAccessPluginLib);
+            return;
+        }
+    }
+    catch (IException *e)
+    {
+        StringBuffer msg;
+        e->errorMessage(msg);
+        diagnostics.statusMessage.setf("%s: Exception during plugin diagnostics: %s", methodName, msg.str());
+        e->Release();
+    }
+    catch (...)
+    {
+        diagnostics.statusMessage.setf("%s: Unknown exception during plugin diagnostics", methodName);
+    }
+}


The new diagnoseLogAccessPluginLoad function provides diagnostic capabilities for plugin loading but lacks unit test coverage. The codebase has comprehensive unit tests in testing/unittests/jlibtests.cpp for jlib functionality. Consider adding tests that verify the diagnostic function correctly identifies various failure scenarios (ConfigNotFound, ConfigFoundNoType, LoadFailed, LoadSucceeded) and properly handles exceptions without leaking resources. This is especially important given the complexity of the error handling paths and the need to ensure diagnostic state is properly set in all cases.

rpastrana · 2026-01-08T00:32:51Z

blocked by #20753

- Adds plugin/configuration issue analysis - Add enum values: ConfigNotFound, ConfigFoundNoType, LoadFailed, LoadSucceeded - Update WsLogAccessService.cpp to use clean switch-based logic instead of nested if-else Co-authored-by: rodrigo pastrana <[email protected]>

- Standardizes the shared object loading logic for logaccess - Updates the HealthReport logic to mimic loading logic Signed-off-by: Rodrigo Pastrana <[email protected]>

Copilot AI review requested due to automatic review settings January 5, 2026 23:37

Copilot started reviewing on behalf of rpastrana January 5, 2026 23:38 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

system/jlib/jlog.cpp Outdated Show resolved Hide resolved

system/jlib/jlog.cpp Outdated Show resolved Hide resolved

system/jlib/jlog.cpp Outdated Show resolved Hide resolved

rpastrana changed the title ~~HPCC-35558 Standadize logaccess plugin load logic~~ HPCC-35558 Standardize logaccess plugin load logic Jan 6, 2026

rpastrana force-pushed the HPCC-35558-logaccesssharedobject-10.0.x branch 2 times, most recently from eb4323b to ab2aad6 Compare January 6, 2026 02:00

rpastrana requested a review from Copilot January 6, 2026 03:52

Copilot started reviewing on behalf of rpastrana January 6, 2026 03:53 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

Copilot AI and others added 2 commits January 8, 2026 15:06

HPCC-35558 Standardize logaccess plugin load logic

ac53be6

- Standardizes the shared object loading logic for logaccess - Updates the HealthReport logic to mimic loading logic Signed-off-by: Rodrigo Pastrana <[email protected]>

rpastrana force-pushed the HPCC-35558-logaccesssharedobject-10.0.x branch from ab2aad6 to ac53be6 Compare January 8, 2026 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HPCC-35558 Standardize logaccess plugin load logic #20792

HPCC-35558 Standardize logaccess plugin load logic #20792

rpastrana commented Jan 5, 2026

Uh oh!

github-actions bot commented Jan 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

rpastrana commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

HPCC-35558 Standardize logaccess plugin load logic #20792

Are you sure you want to change the base?

HPCC-35558 Standardize logaccess plugin load logic #20792

Conversation

rpastrana commented Jan 5, 2026

Type of change:

Checklist:

Smoketest:

Testing:

Uh oh!

github-actions bot commented Jan 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

rpastrana commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant