HPCC-32281 Grafana/Loki logaccess 2nd phase improvements #18900

rpastrana · 2024-07-20T02:36:44Z

Ensures proper log output format
Utilizes context log fields available
Updates grafana logaccess sample config
Replaces unnecessary funtion w/ inline logic
Removes invalid wildcard filter restriction
Trims log data at server due to added json newlines
Reenables several stream columns

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-07-20T02:37:02Z

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

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

rpastrana · 2024-07-22T20:32:48Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+            fieldValues->setProp(m_messageColumn.name, stream->queryProp(m_messageColumn.name));
+            break;
+        }
+        case RETURNCOLS_MODE_custom: //not supported yet


We need to fail and let user know

rpastrana · 2024-07-22T21:15:50Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

@@ -605,14 +647,14 @@ bool GrafanaLogAccessCurlClient::fetchLog(LogQueryResultDetails & resultDetails,
        //from https://grafana.com/docs/loki/latest/query/log_queries/
        //Adding | json to your pipeline will extract all json properties as labels if the log line is a valid json document. Nested properties are flattened into label keys using the _ separator.
        logLineParser.set(" | json log"); //this parses the log entry and extracts the log field into a label
-        logLineParser.append(" | line_format \"{{.log}}\""); //Formats output line to only contain log label
+        logLineParser.append(" | line_format \"{{.log | trim}}\""); //Formats output line to only contain log label


the line_format output doesn't clean up all json new lines

rpastrana · 2024-07-22T21:18:37Z

@jeclrsg this change implements the log output format I provided the other day. Let me know if there are any changes needed so I can move forward w/ this PR. Thanks.

rpastrana · 2024-07-22T23:37:25Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

                                                             //This drops the stream, and various insert timestamps

        //we're always going to get a stream container, and a the log line...
        //the stream container contains unnecessary, and redundant lines
        //there's documentation of a 'drop' command whch doesn't work in practice
        //online recomendation is to clear those stream entries...
-        logLineParser.append(" | label_format log=\"\", filename=\"\", namespace=\"\", node_name=\"\", job=\"\"");// app=\"\", component=\"\", container=\"\", instance=\"\");
+        logLineParser.append(" | label_format log=\"\", filename=\"\"");//, namespace=\"\", job=\"\""// app=\"\", component=\"\", container=\"\", instance=\"\");


some of the "stream" values are useful, don't clear them

rpastrana · 2024-07-22T23:38:07Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.hpp

@@ -73,22 +73,24 @@ class GrafanaLogAccessCurlClient : public CInterfaceOf<IRemoteLogAccess>
    LogField m_instanceColumn = LogField("instance", true);
    LogField m_podColumn = LogField("pod", true);
    LogField m_containerColumn = LogField("container", true);
-    LogField m_messageColumn = LogField("MSG");
+    LogField m_messageColumn = LogField("log");


we'll stuff the entire result log line in the message column.

rpastrana · 2024-07-22T23:38:29Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.hpp

-    LogField m_logProcIDColumn = LogField("PID");
-    LogField m_logThreadIDColumn = LogField("TID");
+    LogField m_logDateTimstampColumn = LogField("tsNs");
+    //LogField m_logTimestampColumn = LogField("TIME");


don't need these until we extract all columns

jeclrsg

Yeah, that column mapping in the yaml looks correct to me based on the conversation we had last week.

rpastrana · 2024-07-25T18:59:34Z

Yeah, that column mapping in the yaml looks correct to me based on the conversation we had last week.

Thanks @jeclrsg can you also review the newly proposed response format? Keep in mind the timestamp we receive from Grafana is an epoch value which can be converted into human readable timestamp:

<GetLogsResponse>
<LogLines>
{
      "lines": [
            {
                  "fields": [
                        {"tsNs": "1721343553880265366"},
                        {"pod": "sasha-thor-qmon-57cc47dc78-tcks5"},
                        {"log": "00000038 AUD INF 2024-07-18 22:59:13.872 1 22 UNK \",ThorQueueMonitor,thor,0,0,1,1,49,---,---\" sasha-thor-qmon-57cc47dc78-tcks5"}
                  ]
            },
            {
                  "fields": [
                        {"tsNs": "1721343561154324789"},
                        {"pod": "eclwatch-775b856c5b-qzwgl"},
                        {"log": "00000141 USR PRO 2024-07-18 22:59:21.027 7 23 UNK \"CInfoCacheReaderThread K8s Resource Reader: InfoCache collected (0 seconds).\" eclwatch-775b856c5b-qzwgl"}
                  ]
            }
      ]
}
</LogLines>
<LogLineCount>10</LogLineCount>
<TotalLogLinesAvailable>0</TotalLogLinesAvailable>
</GetLogsResponse>

ghalliday

Some issues spotted - see comments.

ghalliday · 2024-07-26T12:15:39Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+            Owned<IPropertyIterator> fieldsIter = resultLine->getIterator();
+            ForEach(*fieldsIter)
+            {
+                const char * prop = resultLine->queryProp(fieldsIter->getPropKey());


const char * prop = fieldsIter>queryPropValue() is more efficient/simpler.

same comments for the other formats below.

ghalliday · 2024-07-26T12:17:06Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+                encodeXML(prop, returnbuf);
+                returnbuf.appendf("</%s>", fieldsIter->getPropKey());
+            }
+            returnbuf.appendf("</lin>");


typo: "line" - would generate invalid xml

ghalliday · 2024-07-26T12:18:13Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+                    firstField = false;
+
+                StringBuffer prop;
+                resultLine->getProp(fieldsIter->getPropKey(), prop);


also should use queryPropValue()

ghalliday · 2024-07-26T12:21:50Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+            fieldValues->setProp(m_nodeColumn.name, stream->queryProp(m_nodeColumn.name));
+            fieldValues->setProp(m_containerColumn.name, stream->queryProp(m_containerColumn.name));
+            fieldValues->setProp(m_instanceColumn.name, stream->queryProp(m_instanceColumn.name));
+        }


Needs a comment //fallthrough whenever a case falls through to the next case.

I initially read the code incorrectly because it wasn't there, and coverity will also complain.

ghalliday · 2024-07-26T12:21:59Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+        case RETURNCOLS_MODE_default:
+        {
+            fieldValues->setProp(m_podColumn.name, stream->queryProp(m_podColumn.name));
+        }


//fallthrough

ghalliday · 2024-07-26T12:25:16Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+                logLineIter.setown(result.getElements("values"));
+            }
+
+            processValues(returnbuf, logLineIter, result.queryPropTree("stream"), format, retcolmode, isFirstLine);


This function will crash if logLineIter is null - either it should not be called, or processValues() needs protecting.

ghalliday · 2024-07-26T13:27:54Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

@@ -832,6 +873,24 @@ GrafanaLogAccessCurlClient::GrafanaLogAccessCurlClient(IPropertyTree & logAccess
            if (logMap.hasProp(logMapSearchColAtt))
                m_podColumn.name = logMap.queryProp(logMapSearchColAtt);
        }
+        else if (streq(logMapType, "message"))
+        {
+            if (logMap.hasProp(logMapIndexPatternAtt))


This code looks like it is duplicated multiple times. A candidate for cleaning up in a later PR.
(Not being commoned up makes it look as though each of the different branches are doing something different..)

jeclrsg · 2024-07-26T13:40:22Z

@rpastrana No, I don't see any problems in that response formatting either. It might be nice to have the timestamps be consistent coming out of all the engines, either dates or as ints. But I don't think there'd be any problem with adding some code on the client side to check the engine and be prepared to convert to a date.

rpastrana · 2024-07-26T13:55:35Z

Thanks

@rpastrana No, I don't see any problems in that response formatting either. It might be nice to have the timestamps be consistent coming out of all the engines, either dates or as ints. But I don't think there'd be any problem with adding some code on the client side to check the engine and be prepared to convert to a date.

Thanks for the feedback @jeclrsg.
I'll add a field mapping type of "epoch" and declare the tsNs field as such in the column map. This would let the UI know to map the value to a human readable TS

            <Column>
                  <Name>tsNs</Name>
                  <LogType>timestamp</LogType>
                  <ColumnMode>ALL</ColumnMode>
                  <ColumnType>epoch</ColumnType><!--is there a better type name-->
            </Column>

rpastrana · 2024-08-01T20:13:51Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+            fieldValues->setProp(m_nodeColumn.name, stream->queryProp(m_nodeColumn.name));
+            fieldValues->setProp(m_containerColumn.name, stream->queryProp(m_containerColumn.name));
+            fieldValues->setProp(m_instanceColumn.name, stream->queryProp(m_instanceColumn.name));
+            [[fallthrough]];


C17 feature, not sure if there's any concern using c17 specific constructs

Nice, I hadn't picked up on that feature. We require c++17, so no problem using the feature - it has the advantage it will be checked by the compiler.

rpastrana · 2024-08-01T20:14:26Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+            if (result.hasProp("values"))
+            {
+                logLineIter.setown(result.getElements("values")); // if no values elements found, will get NullPTreeIterator
+                if (!logLineIter)


This doesn't seem necessary

Correct, this will always be non-null. You have to check the result of logLineIter->first() to know if there are no elements found.

k, the goal was to protect processValues from null logLineIter, removing in favor of L271

ghalliday

@rpastrana looks good. Agree that one line is redundant. Please squash.

ghalliday · 2024-08-02T08:56:56Z

system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp

+            if (result.hasProp("values"))
+            {
+                logLineIter.setown(result.getElements("values")); // if no values elements found, will get NullPTreeIterator
+                if (!logLineIter)


Correct, this will always be non-null. You have to check the result of logLineIter->first() to know if there are no elements found.

rpastrana · 2024-08-02T19:09:03Z

@ghalliday squashed

rpastrana · 2024-08-02T19:09:48Z

@jeclrsg what needs to be done on the UI side to support these changes? The "epoch" column type is the only one I can think of.

ghalliday · 2024-08-05T12:17:35Z

@rpastrana looks good. Only question is should this target 9.6.x?

- Ensures proper log output format - Utilizes context log fields available - Updates grafana logaccess sample config - Replaces unnecessary funtion w/ inline logic - Removes invalid wildcard filter restriction - Trims log data at server due to added json newlines - Reenables several stream columns - Adds epoch log field type - Ensures logfield type default is set to string Signed-off-by: Rodrigo Pastrana <[email protected]>

rpastrana · 2024-08-05T14:05:12Z

@ghalliday rebased to 9.6.x but had trouble building locally, awaiting build results on gihubactions...

github-actions · 2024-08-08T16:01:17Z

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

Jirabot Action Result:

rpastrana force-pushed the HPCC-32281-NEW-Loki-context-columns branch 2 times, most recently from 8363a0e to 4590538 Compare July 22, 2024 19:44

rpastrana commented Jul 22, 2024

View reviewed changes

rpastrana assigned jeclrsg Jul 22, 2024

rpastrana commented Jul 22, 2024

View reviewed changes

rpastrana assigned ghalliday Jul 22, 2024

rpastrana requested review from ghalliday and jeclrsg July 25, 2024 13:07

rpastrana assigned rpastrana and unassigned ghalliday and jeclrsg Jul 25, 2024

jeclrsg approved these changes Jul 25, 2024

View reviewed changes

ghalliday requested changes Jul 26, 2024

View reviewed changes

rpastrana requested a review from ghalliday July 30, 2024 17:48

rpastrana force-pushed the HPCC-32281-NEW-Loki-context-columns branch from b18c15a to 9dfd13a Compare July 30, 2024 18:13

rpastrana commented Aug 1, 2024

View reviewed changes

ghalliday approved these changes Aug 2, 2024

View reviewed changes

rpastrana force-pushed the HPCC-32281-NEW-Loki-context-columns branch from e75057f to e0c6ada Compare August 2, 2024 19:08

rpastrana force-pushed the HPCC-32281-NEW-Loki-context-columns branch from e0c6ada to a0283eb Compare August 5, 2024 13:30

rpastrana changed the base branch from candidate-9.8.x to candidate-9.6.x August 5, 2024 14:03

ghalliday closed this Aug 8, 2024

ghalliday reopened this Aug 8, 2024

ghalliday merged commit 2755a2c into hpcc-systems:candidate-9.6.x Aug 8, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32281 Grafana/Loki logaccess 2nd phase improvements #18900

HPCC-32281 Grafana/Loki logaccess 2nd phase improvements #18900

rpastrana commented Jul 20, 2024 •

edited

Loading

github-actions bot commented Jul 20, 2024

rpastrana Jul 22, 2024

rpastrana Jul 22, 2024

rpastrana commented Jul 22, 2024

rpastrana Jul 22, 2024

rpastrana Jul 22, 2024

rpastrana Jul 22, 2024

jeclrsg left a comment

rpastrana commented Jul 25, 2024

ghalliday left a comment

ghalliday Jul 26, 2024

ghalliday Jul 26, 2024

ghalliday Jul 26, 2024

ghalliday Jul 26, 2024

ghalliday Jul 26, 2024

ghalliday Jul 26, 2024

ghalliday Jul 26, 2024

jeclrsg commented Jul 26, 2024

rpastrana commented Jul 26, 2024

rpastrana Aug 1, 2024

ghalliday Aug 2, 2024

rpastrana Aug 1, 2024

ghalliday Aug 2, 2024

rpastrana Aug 2, 2024

ghalliday left a comment

ghalliday Aug 2, 2024

rpastrana commented Aug 2, 2024

rpastrana commented Aug 2, 2024

ghalliday commented Aug 5, 2024

rpastrana commented Aug 5, 2024

github-actions bot commented Aug 8, 2024

HPCC-32281 Grafana/Loki logaccess 2nd phase improvements #18900

HPCC-32281 Grafana/Loki logaccess 2nd phase improvements #18900

Conversation

rpastrana commented Jul 20, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jul 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeclrsg left a comment

Choose a reason for hiding this comment

rpastrana commented Jul 25, 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

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

jeclrsg commented Jul 26, 2024

rpastrana commented Jul 26, 2024

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

rpastrana commented Aug 2, 2024

rpastrana commented Aug 2, 2024

ghalliday commented Aug 5, 2024

rpastrana commented Aug 5, 2024

github-actions bot commented Aug 8, 2024

rpastrana commented Jul 20, 2024 •

edited

Loading