HPCC-32909 Retrieve index settings from ElasticSearch server #19377

kenrowland · 2025-01-03T15:44:40Z

Added code to query configured index on ElasticSearch server for dynamic mapping template match values for hpcc metric type suffixes.

Signed-Off-By: Kenneth Rowland [email protected]

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 · 2025-01-03T15:45:02Z

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

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

rpastrana · 2025-01-06T15:04:16Z

helm/examples/metrics/README.md

-In all cases, the configuration of the sink must match that of the dynamic mappings in the index.
+The object names beginning with "hpcc_metric_" represent the mappings for the three affected hpcc metrics types.
+The _match_ value for each defines the expected suffix for each HPCC metric value, based on type, when metrics
+are reported and indexed. The object names above are the default values useed by the sink. The index can be


rpastrana · 2025-01-06T15:08:40Z

helm/examples/metrics/README.md

@@ -306,10 +325,11 @@ Add the following to the environment.xml configuration file (note that some valu
        <metrics name="mymetricsconfig">
            <sinks name="myelasticsink" type="elastic">
                <settings period="30" ignoreZeroMetrics="1">
-                    <host domain="<domainname>" port="<port>" protocol="http|https">
+                    <host domain="<domainname>" port="<port>" protocol="http|https" 
+                            certificateFilePath="/home/krowland/elastic/cert/http_ca.crt">


I would avoid using your username from this example. I'd opt for something obvious such as "myusername"

Replaced. Question: is there a location on the node where the cert file can be placed so that the sink does not have to specify the path? Could be part of configuring a cluster to use ElasticSearch.

rpastrana · 2025-01-06T15:10:13Z

helm/examples/metrics/elasticsearch_metrics.yaml

-        countMetricSuffix: count
-        gaugeMetricSuffix: gauge
-        histogramMetricSuffix: histogram
+


this new line prob should be removed

rpastrana · 2025-01-06T15:57:17Z

system/metrics/sinks/elastic/elasticSink.cpp

@@ -1,5 +1,5 @@
 /*##############################################################################
-    HPCC SYSTEMS software Copyright (C) 2024 HPCC Systems®.
+    HPCC SYSTEMS software Copyright (C) 2025 HPCC Systems®.


if this was created last year, date shouldn't be updated

Restored to 2024

rpastrana · 2025-01-07T14:03:05Z

system/metrics/sinks/elastic/elasticSink.cpp

@@ -16,21 +16,6 @@
 #include "jsecrets.hpp"
 #include "jencrypt.hpp"

-//including cpp-httplib single header file REST client


is this change directly related to the issue addressed by this jira?

rpastrana · 2025-01-07T14:21:06Z

system/metrics/sinks/elastic/elasticSink.cpp

+    httplib::Result res = pClient->Get(endpoint.c_str(), elasticHeaders);
+    if (res->status != 200)
+    {
+        WARNLOG("ElasticMetricSink: Unable to retrieve index mapping for index %s", indexName.str());


if httplib::Result provides a message as well, it should be reported as well.

httplib result includes a response body (html), but that is likely too long to log. I added the return status code. If we find during testing that additional information from the body is needed, we can review and determine the best way to parse the body and add relevant data to the log.

rpastrana · 2025-01-07T14:24:41Z

system/metrics/sinks/elastic/elasticSink.cpp

+    auto indexConfig = data[indexName.str()];
+    if (indexConfig.is_null())
+    {
+        WARNLOG("ElasticMetricSink: Mapping section for Index '%s' does not exist", indexName.str());


how does one recover from this? Provide useful information to the target audience.

There is no recovery. The mapping section is required. "Unable to continue" can be added (or similar). This is where Ops would need to properly configure the index. Should a log message tell the target audience to refer to a specific document?

rpastrana · 2025-01-07T14:27:46Z

system/metrics/sinks/elastic/elasticSink.cpp

+                        WARNLOG("ElasticMetricSink: Invalid gauge suffix pattern");
+                        return false;
+                    }
+                }


else? report unexpected mapping if encountered

Other mappings can exist. We only look for the ones of interest.

Didn't see if this was addressed.

See above comment I forgot to release. It is not an error for other mappings to exist.

system/metrics/sinks/elastic/elasticSink.cpp

rpastrana · 2025-01-07T14:29:38Z

helm/examples/metrics/README.md

@@ -187,37 +187,41 @@ sink will report metrics from all components to the same index. Therefore, the
 index must be configured to receive metrics from all components. 

 #### Index Configuration
-The index must be created in ElasticSearch before metrics can be reported to it. The name is passed
-to the sink as a configuration setting. The index must be created with the following settings:
+The sink requires the index to be created and configured fully in ElasticSearch in order for the sink to report


The README updates are meant to be helpful, but I found the wording confusing. It might be worth seeking feedback from the doc guys.

IMO, the information here is beyond the intent of the README. I think we need to consider some separate documentation for each sink (could be in same doc). I'd like to address that with Stu and Jim. But for now, I wanted to try and capture the information.

rpastrana

follow up comments...

rpastrana · 2025-01-08T16:04:17Z

system/metrics/sinks/elastic/elasticSink.cpp

@@ -38,6 +37,10 @@ ElasticMetricSink::ElasticMetricSink(const char *name, const IPropertyTree *pSet
        configurationValid = true;
        PROGLOG("ElasticMetricSink: Loaded and configured");
    }
+    else
+    {
+        PROGLOG("ElasticMetricSink: Unable to complete initialization, sink not collecting");


This seems like a warning rather than just progress log. Also, if we know why the problem occurred (either host or index config invalid/missing) we should report it.

Changed to WARNLOG. If init fails, there will be at least one log message prior to this one with the specific reason.

rpastrana · 2025-01-08T16:08:02Z

system/metrics/sinks/elastic/elasticSink.cpp

@@ -175,9 +177,16 @@ bool ElasticMetricSink::getDynamicMappingSuffixesFromIndex(const IPropertyTree *
    std::string endpoint;
    endpoint.append("/").append(indexName.str()).append("/_mapping");
    httplib::Result res = pClient->Get(endpoint.c_str(), elasticHeaders);
+
+    if (res == nullptr)


per online search:
Error Handling:
If the Result is null, it indicates that the request failed. You can use httplib::Error enum to get more details about the error.
and I found this sample error handling pattern:
if (res && res->status == 200)
{
std::cout << res->body << "\n";
}
else
{
auto err = res.error();
std::cout << httplib::to_string(err) << "\n";
}

I added code to capture the error, but there is not httplib::to_string function, so just the number is displayed.

rpastrana · 2025-01-08T16:15:29Z

system/metrics/sinks/elastic/elasticSink.cpp

@@ -291,6 +298,10 @@ void ElasticMetricSink::intializeElasticClient()
    // Add cert path if needed
    if (!certificateFilePath.isEmpty())
        pClient->set_ca_cert_path(certificateFilePath.str());
+
+    pClient->set_connection_timeout(5);


default timeout values are always difficult and usually don't cover most situations. A couple of suggestions:

make these configurable

connect and read/write timeout values should be independent, 5 sec connection timeout might be reasonable, but successful reads/writes might take longer than 5 secs

Added configurable host parameters. Note that after the initial read, we will only be doing writes to index metrics.

rpastrana · 2025-01-08T16:17:26Z

system/metrics/sinks/elastic/elasticSink.cpp

+{
+    std::string endpoint;
+    endpoint.append("/").append(indexName.str());
+    auto res = pClient->Get(endpoint.c_str(), elasticHeaders);


is pClient only used to extract index info? if we intent to use it for other functionality, the above timeout values might not be reasonable.

rpastrana · 2025-01-08T16:17:55Z

system/metrics/sinks/elastic/elasticSink.cpp

@@ -318,9 +329,15 @@ bool ElasticMetricSink::validateIndex()
    endpoint.append("/").append(indexName.str());
    auto res = pClient->Get(endpoint.c_str(), elasticHeaders);

-    if (res->status != 200)
+    if (res == nullptr)


see above suggestion on libhttp err handling

Additional handling added.

rpastrana · 2025-01-08T16:19:21Z

system/metrics/sinks/elastic/elasticSink.cpp

 }


-bool ElasticMetricSink::prepareToStartCollecting()
+bool ElasticMetricSink::convertPatternToSuffix(const char *pattern, StringBuffer &suffix)


could this be static, or does it need to be a member? also, is it const?

rpastrana · 2025-01-08T16:20:06Z

system/metrics/sinks/elastic/elasticSink.cpp

+                        WARNLOG("ElasticMetricSink: Invalid gauge suffix pattern");
+                        return false;
+                    }
+                }


Didn't see if this was addressed.

rpastrana

@kenrowland

Added code to query configured index on ElasticSearch server for dynamic mapping template match values for hpcc metric type suffixes. Signed-Off-By: Kenneth Rowland [email protected]

kenrowland · 2025-01-08T22:59:12Z

@ghalliday please merge.

ghalliday

I don't see any problems.

ghalliday · 2025-01-10T09:37:57Z

system/metrics/sinks/elastic/elasticSink.cpp

+{
+    StringBuffer countSuffixMappingName;
+    if (!pIndexConfigTree->getProp("@countSuffixMappingName", countSuffixMappingName))
+        countSuffixMappingName.append("hpcc_metrics_count_suffix");


If you are concerned about efficiency, then the following is better since it avoids cloning the strings:

const char * countSuffixMappingName = pIndexConfigTree->queryProp("@countSuffixMappingName"); if (!countSuffixMappingName) countSuffixMappingName = "hpcc_metrics_count_suffix";

However this is only called once, so efficiency is not significant. Not something to change.

ghalliday · 2025-01-10T09:49:15Z

system/metrics/sinks/elastic/elasticSink.cpp

+                auto const &mappingName = it.first;
+                auto kvPairs = it.second;
+                auto const &matchValue = kvPairs["match"];
+                if (mappingName == countSuffixMappingName.str())


This code is confusing - because the RHS is a const char *, so this would intuitively be a pointer comparison. I think it probably is a string comparison - because the LHS is a std:string - but that isn't at all clear because of the use of auto.
I had thought this would mean each comparison is converting the const char * to a str::string, but the standard library does optimize that case.

Use strsame/streq would be clearer.

ghalliday · 2025-01-10T10:02:26Z

system/metrics/sinks/elastic/elasticSink.hpp

@@ -18,6 +18,22 @@
 #include "jptree.hpp"
 #include "jstring.hpp"

+//including cpp-httplib single header file REST client


A better solution would be to move the definition of the ElasticMetricSink class into the c++ file. If this was included from many places, then each of those places would now be processing the entire header file.

ghalliday · 2025-01-10T10:09:18Z

@kenrowland there is an (unrelated) build error which needs addressing - probably by disabling a warning - before this can be merged.

kenrowland requested a review from rpastrana January 3, 2025 15:45

rpastrana requested changes Jan 7, 2025

View reviewed changes

kenrowland force-pushed the HPCC-32909 branch from 06e88e2 to 9bcd8d3 Compare January 7, 2025 20:52

kenrowland requested a review from rpastrana January 7, 2025 22:41

kenrowland force-pushed the HPCC-32909 branch from 9bcd8d3 to 5808b32 Compare January 8, 2025 14:54

rpastrana requested changes Jan 8, 2025

View reviewed changes

kenrowland requested a review from rpastrana January 8, 2025 19:40

rpastrana approved these changes Jan 8, 2025

View reviewed changes

HPCC-32909 Retrieve index settings from ElasticSearch server

592be1a

Added code to query configured index on ElasticSearch server for dynamic mapping template match values for hpcc metric type suffixes. Signed-Off-By: Kenneth Rowland [email protected]

kenrowland force-pushed the HPCC-32909 branch from b646bf1 to 592be1a Compare January 8, 2025 22:58

ghalliday approved these changes Jan 10, 2025

View reviewed changes

HPCC-32909 Retrieve index settings from ElasticSearch server #19377

Are you sure you want to change the base?

HPCC-32909 Retrieve index settings from ElasticSearch server #19377

Conversation

kenrowland commented Jan 3, 2025 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jan 3, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana 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

rpastrana left a comment

Choose a reason for hiding this comment

kenrowland commented Jan 8, 2025

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

ghalliday commented Jan 10, 2025

kenrowland commented Jan 3, 2025 •

edited

Loading