Addressed review comments

hpcc-systems · Nov 4, 2024 · a3dd106 · a3dd106
1 parent 015cf20
commit a3dd106
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 27 deletions.
diff --git a/helm/examples/metrics/README.md b/helm/examples/metrics/README.md
@@ -250,7 +250,9 @@ Make a copy and modify as needed for your installation.
 ##### Configuration Settings
 The ElasticSearch sink defines the following settings:
 
-* elasticHost - The host name or IP address of the ElasticSearch server. (required)
+* hostProtocol - The protocol used to connect to the ElasticSearch server. (default: https)
+* hostName - The host name or IP address of the ElasticSearch server. (required)
+* hostPort - The port number of the ElasticSearch server. (default: 9200)
 * indexName - The name of the index to which metrics are reported. (required)
 * countMetricSuffix - The suffix used to identify count metrics. (default: count)
 * gaugeMetricSuffix - The suffix used to identify gauge metrics. (default: gauge)
@@ -271,11 +273,11 @@ settings are shown):
 <Environment>
     <Software>
         <metrics name="mymetricsconfig">
-            <sinks name="elastic" type="elastic">
-                <settings period="30" 
-                          ignoreZeroMetrics="1" 
-                          elasticHost="<url>"
-                          indexName="<index>" />
+            <sinks name="myelasticsink" type="elastic">
+                <settings period="30" ignoreZeroMetrics="1">
+                    <host name="<hostname>" port="<port>" protocol="http|htps"/>
+                    <index name="<index>"/> 
+                <settings/>
             </sinks>
         </metrics>
     </Software>

diff --git a/helm/examples/metrics/elasticsearch_metrics.yaml b/helm/examples/metrics/elasticsearch_metrics.yaml
@@ -3,11 +3,15 @@
 # Settings:
 #   type                                - sink type (must be elastic for ElasticSearch support)
 #   name                                - name for the sink instance
-#   settings.elasticHost                - url for the ElasticSearch instance
-#   settings.indexName                  - ElasticSearch index name where metrics are indexed
-#   settings.countMetricSuffix          - suffix for count metrics
-#   settings.gaugeMetricSuffix          - suffix for gauge metrics
-#   settings.histogramMetricSuffix      - suffix for histogram metrics
+#   settings.countMetricSuffix          - suffix for count metrics (default: count)
+#   settings.gaugeMetricSuffix          - suffix for gauge metrics (default: gauge)
+#   settings.histogramMetricSuffix      - suffix for histogram metrics (default: histogram)
+#   settings.host                       - ElasticSearch host settings
+#   settings.host.protocol               - protocol to use, http or https (default)
+#   settings.host.name                   - host name
+#   settings.host.port                   - port number (default 9200)
+#   settings.index                       - ElasticSearch index settings
+#   settings.index.name                  - index name
 #
 # If not overridden, the following suffixes are used by default:
 #   countMetricSuffix: count
@@ -18,10 +22,15 @@ global:
   metrics:
     sinks:
     - type: elastic
-      name: elasticsink
+      name: myelasticsink
       settings:
-        elasticHost: http://localhost:9200,
-        indexName: hpccmetrics,
         countMetricSuffix: count,
         gaugeMetricSuffix: gauge,
         histogramMetricSuffix: histogram
+        host:
+          protocol: https
+          name: hostname
+          port: 9200
+        index:
+          name: hpccmetrics
+
diff --git a/system/metrics/sinks/elastic/elasticSink.cpp b/system/metrics/sinks/elastic/elasticSink.cpp
@@ -12,7 +12,6 @@
 ############################################################################## */
 
 #include "elasticSink.hpp"
-
 #include "nlohmann/json.hpp"
 
 //including cpp-httplib single header file REST client
@@ -43,13 +42,48 @@ ElasticMetricSink::ElasticMetricSink(const char *name, const IPropertyTree *pSet
     PeriodicMetricSink(name, "elastic", pSettingsTree)
 {
     ignoreZeroMetrics = pSettingsTree->getPropBool("@ignoreZeroMetrics", true);
-    pSettingsTree->getProp("@elasticHost", elasticHost);
-    pSettingsTree->getProp("@indexName", indexName);
+
+    StringBuffer hostName;
+    StringBuffer hostProtocol;
+    StringBuffer hostPort;
+
+    Owned<IPropertyTree> pHostConfigTree = pSettingsTree->getPropTree("host");
+
+    pHostConfigTree->getProp("@name", hostName);
+
+    if (!pHostConfigTree->getProp("@protocol", hostProtocol))
+    {
+        hostProtocol.append("https");
+    }
+
+    if (!pHostConfigTree->getProp("@port", hostPort))
+    {
+        hostPort.append("9200");
+    }
+
+    if (!hostName.isEmpty() && !hostPort.isEmpty() && !hostProtocol.isEmpty())
+    {
+        elasticHostUrl.append(hostProtocol).append("://").append(hostName).append(":").append(hostPort);
+    }
+    else
+    {
+        WARNLOG("ElasticMetricSink: Host configuration is invalid");
+    }
+
+    Owned<IPropertyTree> pIndexConfigTree = pSettingsTree->getPropTree("index");
+    pSettingsTree->getProp("@name", indexName);
+    if (indexName.isEmpty())
+    {
+        WARNLOG("ElasticMetricSink: Index configuration is invalid");
+    }
+
+    // Both a host url and an index name are required
+    configurationValid = !elasticHostUrl.isEmpty() && !indexName.isEmpty();
 
     // Initialize standard suffixes
-    countMetricSuffix.append("_count");
-    gaugeMetricSuffix.append("_gauge");
-    histogramMetricSuffix.append("_histogram");
+    countMetricSuffix.append("count");
+    gaugeMetricSuffix.append("gauge");
+    histogramMetricSuffix.append("histogram");
 
     // See if any are overridden
     pSettingsTree->getProp("@countMetricSuffix", countMetricSuffix);
@@ -60,11 +94,6 @@ ElasticMetricSink::ElasticMetricSink(const char *name, const IPropertyTree *pSet
 
 void ElasticMetricSink::prepareToStartCollecting()
 {
-    if (elasticHost.isEmpty())
-        throw MakeStringException(-1, "ElasticMetricSink: ElasticSearch host missing");
-
-    if (indexName.isEmpty())
-        throw MakeStringException(-1, "ElasticMetricSink: Index name missing");
 
 }
 
@@ -77,6 +106,6 @@ void ElasticMetricSink::doCollection()
 
 void ElasticMetricSink::collectingHasStopped()
 {
-    ;
+
 }
 
diff --git a/system/metrics/sinks/elastic/elasticSink.hpp b/system/metrics/sinks/elastic/elasticSink.hpp
@@ -38,8 +38,9 @@ class ELASTICSINK_API ElasticMetricSink : public hpccMetrics::PeriodicMetricSink
 protected:
     StringBuffer indexName;
     bool ignoreZeroMetrics = false;
-    StringBuffer elasticHost;
+    StringBuffer elasticHostUrl;
     StringBuffer countMetricSuffix;
     StringBuffer gaugeMetricSuffix;
     StringBuffer histogramMetricSuffix;
+    bool configurationValid = false;
 };