From b18c15a26bd47c1b7dac7e7fe667fd93b8905791 Mon Sep 17 00:00:00 2001 From: Rodrigo Pastrana Date: Tue, 30 Jul 2024 13:47:25 -0400 Subject: [PATCH] HPCC-32281 Code review 1 - Adds epoch log field type - Ensures logfield type default is set to string - Fixes invalid XML tag - Uses queryPropValues rather than queryprop(key) - Uses queryProp(key) rather than queryProp - Protects null reference to interatro in processValues method - Utilizes cpp17 switch case [[fallthrough]] - Generalizes logMap configuration logic Signed-off-by: Rodrigo Pastrana --- esp/scm/ws_logaccess.ecm | 3 +- .../ws_logaccess/WsLogAccessService.cpp | 4 + .../Grafana/CurlClient/GrafanaCurlClient.cpp | 119 ++++++------------ 3 files changed, 44 insertions(+), 82 deletions(-) diff --git a/esp/scm/ws_logaccess.ecm b/esp/scm/ws_logaccess.ecm index bd214a668fa..5232f983275 100644 --- a/esp/scm/ws_logaccess.ecm +++ b/esp/scm/ws_logaccess.ecm @@ -79,7 +79,8 @@ ESPenum LogColumnValueType : string str("string"), numeric("numeric"), datetime("datetime"), - enum("enum") + enum("enum"), + epoch("epoch") }; ESPStruct [nil_remove] LogColumn diff --git a/esp/services/ws_logaccess/WsLogAccessService.cpp b/esp/services/ws_logaccess/WsLogAccessService.cpp index 0e8b12c329d..98e4a98ad90 100644 --- a/esp/services/ws_logaccess/WsLogAccessService.cpp +++ b/esp/services/ws_logaccess/WsLogAccessService.cpp @@ -71,6 +71,10 @@ bool Cws_logaccessEx::onGetLogAccessInfo(IEspContext &context, IEspGetLogAccessI WARNLOG("Invalid col type found in logaccess logmap config"); } } + else + { + espLogColumn->setColumnType("string"); + } logColumns.append(*espLogColumn.getClear()); } else diff --git a/system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp b/system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp index c9db448f9fe..b973f3f1258 100644 --- a/system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp +++ b/system/logaccess/Grafana/CurlClient/GrafanaCurlClient.cpp @@ -203,12 +203,12 @@ void formatResultLine(StringBuffer & returnbuf, const IProperties * resultLine, Owned fieldsIter = resultLine->getIterator(); ForEach(*fieldsIter) { - const char * prop = resultLine->queryProp(fieldsIter->getPropKey()); + const char * prop = fieldsIter->queryPropValue(); returnbuf.appendf("<%s>", fieldsIter->getPropKey()); encodeXML(prop, returnbuf); returnbuf.appendf("", fieldsIter->getPropKey()); } - returnbuf.appendf(""); + returnbuf.appendf(""); isFirstLine = false; break; } @@ -227,7 +227,7 @@ void formatResultLine(StringBuffer & returnbuf, const IProperties * resultLine, else firstField = false; - const char * prop = resultLine->queryProp(fieldsIter->getPropKey()); + const char * prop = fieldsIter->queryPropValue(); returnbuf.appendf("{\"%s\":\"", fieldsIter->getPropKey()); encodeJSON(returnbuf,prop); returnbuf.append("\"}"); @@ -248,9 +248,8 @@ void formatResultLine(StringBuffer & returnbuf, const IProperties * resultLine, else firstField = false; - StringBuffer prop; - resultLine->getProp(fieldsIter->getPropKey(), prop); - encodeCSVColumn(returnbuf, prop); + const char * fieldValue = resultLine->queryProp(fieldsIter->getPropKey()); + encodeCSVColumn(returnbuf, fieldValue); } returnbuf.newline(); @@ -269,6 +268,9 @@ void formatResultLine(StringBuffer & returnbuf, const IProperties * resultLine, */ void GrafanaLogAccessCurlClient::processValues(StringBuffer & returnbuf, IPropertyTreeIterator * valuesIter, IPropertyTree * stream, LogAccessLogFormat format, const LogAccessReturnColsMode retcolmode, bool & isFirstLine) { + if (!valuesIter) + return; + Owned fieldValues = createProperties(true); //extract the requested fields from the stream if it's available @@ -281,10 +283,12 @@ void GrafanaLogAccessCurlClient::processValues(StringBuffer & returnbuf, IProper 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]]; } case RETURNCOLS_MODE_default: { fieldValues->setProp(m_podColumn.name, stream->queryProp(m_podColumn.name)); + [[fallthrough]]; } case RETURNCOLS_MODE_min: { @@ -416,10 +420,12 @@ void GrafanaLogAccessCurlClient::processQueryJsonResp(LogQueryResultDetails & re if (result.hasProp("values")) { - logLineIter.setown(result.getElements("values")); - } + logLineIter.setown(result.getElements("values")); // if no values elements found, will get NullPTreeIterator + if (!logLineIter) + IERRLOG("%s: Grafana response does not contain expected 'values' elements", COMPONENT_NAME); - processValues(returnbuf, logLineIter, result.queryPropTree("stream"), format, retcolmode, isFirstLine); + processValues(returnbuf, logLineIter, result.queryPropTree("stream"), format, retcolmode, isFirstLine); + } } //Adds the format postfix to the return buffer @@ -717,6 +723,19 @@ bool GrafanaLogAccessCurlClient::fetchLog(LogQueryResultDetails & resultDetails, return false; } +void processLogMapConfig(const IPropertyTree * logMapConfig, LogField * targetField) +{ + if (!logMapConfig || !targetField) + return; + + if (logMapConfig->hasProp(logMapIndexPatternAtt)) + if (strcmp(logMapConfig->queryProp(logMapIndexPatternAtt), "stream")==0) + targetField->isStream = true; + + if (logMapConfig->hasProp(logMapSearchColAtt)) + targetField->name = logMapConfig->queryProp(logMapSearchColAtt); +} + GrafanaLogAccessCurlClient::GrafanaLogAccessCurlClient(IPropertyTree & logAccessPluginConfig) { m_pluginCfg.set(&logAccessPluginConfig); @@ -810,91 +829,29 @@ GrafanaLogAccessCurlClient::GrafanaLogAccessCurlClient(IPropertyTree & logAccess IPropertyTree & logMap = logMapIter->query(); const char * logMapType = logMap.queryProp("@type"); if (streq(logMapType, "global")) - { - if (logMap.hasProp(logMapIndexPatternAtt)) - if (strcmp(logMap.queryProp(logMapIndexPatternAtt), "stream")==0) - m_globalSearchCol.isStream = true; - - if (logMap.hasProp(logMapSearchColAtt)) - m_globalSearchCol.name = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_globalSearchCol); else if (streq(logMapType, "workunits")) - { - if (logMap.hasProp(logMapSearchColAtt)) - m_workunitsColumn = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_workunitsColumn); else if (streq(logMapType, "components")) - { - if (logMap.hasProp(logMapIndexPatternAtt)) - if (strcmp(logMap.queryProp(logMapIndexPatternAtt), "stream")==0) - m_componentsColumn.isStream = true; - - if (logMap.hasProp(logMapSearchColAtt)) - m_componentsColumn.name = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_componentsColumn); else if (streq(logMapType, "class")) - { - if (logMap.hasProp(logMapSearchColAtt)) - m_classColumn = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_classColumn); else if (streq(logMapType, "audience")) - { - if (logMap.hasProp(logMapSearchColAtt)) - m_audienceColumn = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_audienceColumn); else if (streq(logMapType, "instance")) - { - if (logMap.hasProp(logMapIndexPatternAtt)) - if (strcmp(logMap.queryProp(logMapIndexPatternAtt), "stream")==0) - m_instanceColumn.isStream = true; - - if (logMap.hasProp(logMapSearchColAtt)) - m_instanceColumn.name = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_instanceColumn); else if (streq(logMapType, "node")) - { - if (logMap.hasProp(logMapIndexPatternAtt)) - if (strcmp(logMap.queryProp(logMapIndexPatternAtt), "stream")==0) - m_nodeColumn.isStream = true; - - if (logMap.hasProp(logMapSearchColAtt)) - m_nodeColumn.name = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_nodeColumn); else if (streq(logMapType, "host")) - { OWARNLOG("%s: 'host' LogMap entry is NOT supported!", COMPONENT_NAME); - } else if (streq(logMapType, "pod")) - { - if (logMap.hasProp(logMapIndexPatternAtt)) - if (strcmp(logMap.queryProp(logMapIndexPatternAtt), "stream")==0) - m_podColumn.isStream = true; - - if (logMap.hasProp(logMapSearchColAtt)) - m_podColumn.name = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_podColumn); else if (streq(logMapType, "message")) - { - if (logMap.hasProp(logMapIndexPatternAtt)) - if (strcmp(logMap.queryProp(logMapIndexPatternAtt), "stream")==0) - m_messageColumn.isStream = true; - - if (logMap.hasProp(logMapSearchColAtt)) - m_messageColumn.name = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_messageColumn); else if (streq(logMapType, "timestamp")) - { - if (logMap.hasProp(logMapIndexPatternAtt)) - if (strcmp(logMap.queryProp(logMapIndexPatternAtt), "stream")==0) - m_logDateTimstampColumn.isStream = true; - - if (logMap.hasProp(logMapSearchColAtt)) - m_logDateTimstampColumn.name = logMap.queryProp(logMapSearchColAtt); - } + processLogMapConfig(&logMap, &m_logDateTimstampColumn); else - { ERRLOG("Encountered invalid LogAccess field map type: '%s'", logMapType); - } } DBGLOG("%s: targeting: '%s' - datasource: '%s'", COMPONENT_NAME, m_grafanaConnectionStr.str(), m_dataSourcesAPIURI.str());