HPCC-32874 Add WsLogAcces Health Report Method #19307

rpastrana · 2024-11-20T05:05:44Z

HPCC-32874 Add WsLogAcces Health Report Method

Adds healthreport method to logaccess interface
Implements healthreport method for all logaccess plugins
Adds general rest request support in grafana client

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-11-20T05:06:02Z

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

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

rpastrana · 2024-11-20T17:20:58Z

Sample Grafana/Loki output:

Add WsLogAcces Health Report Method - Adds healthreport method to logaccess interface - Implements healthreport method for all logaccess plugins Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexisrisk.com>

rpastrana · 2024-12-13T18:01:01Z

@jeclrsg I added this method to help developers debug config/connection logaccess issues, but thought ECLWatch might be able to make use of it as well. As of now, the different plugins (Elastic Stack, Azure, loki) generate their own reports with unique content related to the particular plugin, but we could enforce certain entries if you think ECLWatch could use them. Let me know what you think.

asselitx

The biggest item in my mind is a preference for a common structured output. Second are questions about encoding output for security- I'm in that mindset now, but not sure what would be best in this case.

asselitx · 2024-12-13T21:03:40Z

esp/scm/ws_logaccess.ecm

+    bool IncludeSampleQuery(true);
+};
+
+ESPResponse GetHealthReportResponse


this should have [exceptions_inline] also

asselitx · 2024-12-13T21:06:14Z

esp/scm/ws_logaccess.ecm

+
+ESPResponse GetHealthReportResponse
+{
+    string Report;


I'd much rather see a structured response that has some common subset of useful values for ECLWatch.

Agreed, commented below for further discussion

asselitx · 2024-12-13T21:09:47Z

esp/services/ws_logaccess/WsLogAccessService.cpp

+    bool success = true;
+    if (!queryRemoteLogAccessor())
+    {
+        report.append("\"Error\": \"LogAccess plugin not available, review logAccess configuration!\"");


This should also be part of a structured response.

In agreement. what is the ESP standard for these types of messages? (ie, what's the expected structure format)

asselitx · 2024-12-13T21:10:06Z

esp/services/ws_logaccess/WsLogAccessService.cpp

+    resp.setReport(report.str());
+
+    return success;
+}


Missing final newline

asselitx · 2024-12-13T21:10:58Z

system/jlib/jlog.hpp

@@ -1675,6 +1710,106 @@ struct LogQueryResultDetails
    unsigned int totalReceived;
    unsigned int totalAvailable;
 };
+/*


I'm guessing before you finalize you'll remove these comments and those in WsLogAccessService.cpp.

Some, but kept the structure around as a mechanism to track structured responses for the ESP

asselitx · 2024-12-16T17:23:55Z

system/logaccess/ElasticStack/ElasticStackLogAccess.cpp

+        {
+            StringBuffer configJSON;
+            toJSON(m_pluginCfg, configJSON, 0, JSON_Format|JSON_HideRootArrayObject);
+            report.appendf("\"ConfigurationTree\": %s", configJSON.str()); //json encode


This is supposed to be more JSON appended to the report, not a string that contains the JSON configuration, correct? If it is the latter then it probably should be encoded.

asselitx · 2024-12-16T17:40:03Z

system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp

+        {
+            StringBuffer configJSON;
+            toJSON(m_pluginCfg, configJSON, 0);
+            report.appendf("\"ConfigurationTree\": %s", configJSON.str()); //json encode


Same question here as in ElasticStackLogAccess.cpp about JSON vs JSON in a string which needs encoding.

asselitx · 2024-12-16T17:40:13Z

system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp

+            StringBuffer configJSON;
+            toJSON(m_pluginCfg, configJSON, 0);
+            report.appendf("\"ConfigurationTree\": %s", configJSON.str()); //json encode
+        }


Is there any sensitive information in the configuration that shouldn't be exposed, like maybe tokens or passwords or internal URLs? On the other hand maybe this is a moot question since ECLWatch already gives access to component configs for roxie, esp etc.

Fair question, I don't think we support credentials in the config, but I'll make sure we don't, and if we do I'll mask it

HELM chart Schema doesn't call for any sensitive data, I think we're ok.

HPCC-Platform/helm/hpcc/values.schema.json

Line 3192 in 70ccefc

"logAccess": {

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

system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp

+                StringBuffer queryString, queryIndex;
+                populateKQLQueryString(queryString, queryIndex, queryOptions);
+
+                StringBuffer encodedValue;


Is this the only string that needs to be encoded, or are there other cases where user input could be part of any value returned in the report? Would it be a good idea to encode all strings returned?

Good question, within the sample query section, yes. But point taken, I'll look around and see if anything else could/should be encoded.

asselitx · 2024-12-16T17:54:17Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.hpp

@@ -107,4 +109,5 @@ class GrafanaLogAccessCurlClient : public CInterfaceOf<IRemoteLogAccess>
    virtual IRemoteLogAccessStream * getLogReader(const LogAccessConditions & options, LogAccessLogFormat format) override;
    virtual IRemoteLogAccessStream * getLogReader(const LogAccessConditions & options, LogAccessLogFormat format, unsigned int pageSize) override;
    virtual bool supportsResultPaging() const override { return false;}
+    virtual bool healthReport(StringBuffer & report, LogAccessHealthReportOptions options) override;
 };


Missing final newline

jeclrsg · 2024-12-17T14:11:59Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+        report.append("\"ConnectionInfo\": { ");
+        report.appendf("\"ConnectionString\": \"%s\"", m_grafanaConnectionStr.str());
+        report.appendf(", \"UserName\": \"%s\"", m_grafanaUserName.str());
+        report.appendf(", \"PasswordProvided\": %s", isEmptyString(m_grafanaPassword.str()) ? "true" : "false");


This was the only thing I noticed that Terrence hadn't mentioned already. Are these backwards? Seems like if isEmptyString(...) then PasswordProvided should be set to false.

rpastrana · 2024-12-17T15:08:16Z

Thanks for the review guys (@asselitx / @jeclrsg ) I'll work those suggestions in today.

I do want to discuss the suggestion to provide further structure, which I fully agree with, and I'm glad it was brought up. Since the method is not meant for client consumption (it's really just for developer/operations debugging) it doesn't
The problem we're faced with is that each of the plugins report unique values/structures/concepts and I found myself trying to force non-overlapping data into common sounding elements/structures and it didn't feel right.

Here's my thought, if ECLWatch commits to consuming some common values, we'll go through the effort of structuring the report. Something like:

Report
        Configuration
              ConfigTree <JSON configuration tree>
              Message <JSON connection related message(s)>    
        Connection
                Status OK | Failed
                Message <JSON connection related message(s)>
        Internals
                Plugin <JSON string containing any internal info the plugin chooses to report for debugging>
                Server <JSON string containing any internal info the server chose to report>
        SampleQuery
                 Query  <JSON string describing the sample query>
                 QueryString  ActualQueryStringSubmitted
                 Results
                       Count CountofRecordsReported
                       Logs  <JSON string raw results from server>

It feels to me like ECLWatch would only benefit from the Report/Conection/Status. Thoughts?

jeclrsg · 2024-12-17T15:59:05Z

@rpastrana That makes sense to me. I was looking at some of the docs yesterday, and one of them (either Grafana or Kibana) made it sound like they had an api status URI that only returned a 200 (no actual response body) if everything is ok. But I'd agree that ECL Watch probably only needs to get a status flag, and if the engine isn't available some message as to why.

rpastrana · 2024-12-18T14:08:48Z

helm/managed/logging/loki-stack/grafana-hpcc-logaccess.yaml

@@ -11,7 +11,7 @@ global:
      id: "1"
      name: "Loki"
    namespace:
-      name: "hpcc"


I need to revert this change

asselitx · 2024-12-18T21:32:30Z

A "status" and "message" field makes sense to me too- really whatever it is that the UI would need. Then if more details are needed in the future we could extend the interface. I think it still makes sense to return discrete fields that don't require further parsing and have a neat SOAP representation even though ECLWatch uses (exclusively?) JSON.

If there is also a need for the full raw "health report" for debugging/troubleshooting exposed through ESP I'd not be opposed to that as a separate method. Trying to make a common representation for that probably doesn't make sense and may even make it less useful.

rpastrana · 2024-12-19T13:44:40Z

A "status" and "message" field makes sense to me too- really whatever it is that the UI would need. Then if more details are needed in the future we could extend the interface. I think it still makes sense to return discrete fields that don't require further parsing and have a neat SOAP representation even though ECLWatch uses (exclusively?) JSON.

If there is also a need for the full raw "health report" for debugging/troubleshooting exposed through ESP I'd not be opposed to that as a separate method. Trying to make a common representation for that probably doesn't make sense and may even make it less useful.

@asselitx I think your comment makes a lot of sense. I'm wondering though if this single method could/should serve the debug report based on a flag. IE by default it only reports well established "status" and "message", but also optionally provides the debug report as a separate text field (in JSON format).

- Adds ESP respons structure - Introduces top level gree/yellow/red status concept - Introduces jlog structs to help status reporting - Utilizes JSON appending/encoding functionality - Various other minor changes Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexisrisk.com>

rpastrana · 2024-12-20T16:41:05Z

esp/scm/ws_logaccess.ecm

+    bool IncludeSampleQuery(false);
+};
+
+ESPenum LogAccessStatusCode : int


@jeclrsg this service can either provide a green/yellow/red type status, or more specific status codes as in the enum below. Let me know what you'd prefer.

rpastrana · 2024-12-20T16:42:44Z

@asselitx updated the ESP interface and some internal jlog interfaces to match the suggestions from code review. I only updated the Azure client, and will update the other clients once we finalize the ESP interface.

rpastrana · 2024-12-20T16:43:49Z

Sample report from Azure client:

rpastrana changed the title ~~HPCC-32874~~ HPCC-32874 Add WsLogAcces Health Report Method Nov 20, 2024

HPCC-32874

f77d122

Add WsLogAcces Health Report Method - Adds healthreport method to logaccess interface - Implements healthreport method for all logaccess plugins Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexisrisk.com>

rpastrana force-pushed the HPCC-32874-wslogaccesshealth branch from 9224ed6 to f77d122 Compare December 13, 2024 17:45

rpastrana requested review from asselitx and jeclrsg December 13, 2024 17:50

asselitx requested changes Dec 16, 2024

View reviewed changes

jeclrsg reviewed Dec 17, 2024

View reviewed changes

rpastrana commented Dec 18, 2024

View reviewed changes

rpastrana requested review from jeclrsg and asselitx December 20, 2024 16:39

rpastrana commented Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32874 Add WsLogAcces Health Report Method #19307

HPCC-32874 Add WsLogAcces Health Report Method #19307

rpastrana commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024

rpastrana commented Nov 20, 2024

rpastrana commented Dec 13, 2024 •

edited

Loading

asselitx left a comment

asselitx Dec 13, 2024

asselitx Dec 13, 2024

rpastrana Dec 17, 2024

asselitx Dec 13, 2024

rpastrana Dec 17, 2024

asselitx Dec 13, 2024

asselitx Dec 13, 2024

rpastrana Dec 17, 2024

asselitx Dec 16, 2024

asselitx Dec 16, 2024

asselitx Dec 16, 2024

rpastrana Dec 17, 2024

rpastrana Dec 18, 2024

asselitx Dec 16, 2024

rpastrana Dec 17, 2024

asselitx Dec 16, 2024

jeclrsg Dec 17, 2024

rpastrana commented Dec 17, 2024 •

edited

Loading

jeclrsg commented Dec 17, 2024

rpastrana Dec 18, 2024

asselitx commented Dec 18, 2024

rpastrana commented Dec 19, 2024

rpastrana Dec 20, 2024

rpastrana commented Dec 20, 2024

rpastrana commented Dec 20, 2024

HPCC-32874 Add WsLogAcces Health Report Method #19307

Are you sure you want to change the base?

HPCC-32874 Add WsLogAcces Health Report Method #19307

Conversation

rpastrana commented Nov 20, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Nov 20, 2024

rpastrana commented Nov 20, 2024

rpastrana commented Dec 13, 2024 • edited Loading

asselitx 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

rpastrana commented Dec 17, 2024 • edited Loading

jeclrsg commented Dec 17, 2024

Choose a reason for hiding this comment

asselitx commented Dec 18, 2024

rpastrana commented Dec 19, 2024

Choose a reason for hiding this comment

rpastrana commented Dec 20, 2024

rpastrana commented Dec 20, 2024

rpastrana commented Nov 20, 2024 •

edited

Loading

rpastrana commented Dec 13, 2024 •

edited

Loading

rpastrana commented Dec 17, 2024 •

edited

Loading