HPCC-32734 Add ElasticSearch metric sink configuration #19247

kenrowland · 2024-10-29T18:07:07Z

Added configuration options for the ElasticSearch sink Updated metrics readme with information on adding the sink to the cluster Added example ElasticSearch configuraion yaml file

Signed-Off-By: Kenneth Rowland kenneth.rowland@lexisnexisrisk.com

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:

Added configuration options for the ElasticSearch sink Updated metrics readme with information on adding the sink to the cluster Added example ElasticSearch configuraion yaml file Signed-Off-By: Kenneth Rowland kenneth.rowland@lexisnexisrisk.com

github-actions · 2024-10-29T19:39:56Z

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

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

rpastrana

@kenrowland looks good, but I do have some comments for you to ponder. They're all meant to be constructive, let me know if should clarify any points. Thanks.

rpastrana · 2024-10-29T21:28:43Z

helm/examples/metrics/README.md

+data types to be stored in the index in their native types. Without dynamic mapping, the ElasticSearch
+default mapping does not properly map value to unsigned 64-bit integers. 
+
+To create an index with dynamic mapping, use the following object when creating the index:


this write up seems very useful.
Is the goal still to handle the creation of the indices programmatically in later versions of the sink?

Currently no plans to create the index from the sink. If that was something we added, there are several considerations that must be taken into account. These are

If the cluster is using the same index for all metric reporting, creating and configuring the index must be coordinated somehow. If each component creates their own, there still needs to be coordination.

The sink would also need to add mapping templates in order to map values to the correct type. This could still use wildcard dynamic templates, or could be specific to the metrics registered for each metric framework instance

If the sink creates the index, we need to worry about life cycle management.

I felt like just telling the sink what index to use allows the policy of the ElasticSearch instance to remain there instead of pushing it down to the sink.

If we decide to push that responsibility to the sink, we can add it as an improvement addressing the concerns from above

rpastrana · 2024-10-29T21:32:21Z

helm/examples/metrics/README.md

+<Environment>
+    <Software>
+        <metrics name="mymetricsconfig">
+            <sinks name="elastic" type="elastic">


trivial but changing the name to 'myelasticsink' would drive to point further and would follow the convention from the metrics name attribute above.

Sounds reasonable. I'll change that.

rpastrana · 2024-10-30T12:54:10Z

helm/examples/metrics/README.md

+    <Software>
+        <metrics name="mymetricsconfig">
+            <sinks name="elastic" type="elastic">
+                <settings period="30" 


The challenge of adding the configuration before the logic is higher potential for downstream restructure of configuration which can be very disruptive to end-users.

It's obviously hard to know what we don't know we'll need to support in later versions, but let's scrutinize each element of your config layout more than usual...
for instance, indexName, if we're planning on expanding the index creation, we might need a pattern rather than a name. with that in mind it might be worth supporting a structured element for the index, for now the only active attribute for the index element could be "name", later adding more attributes under the index element

Index config changed accordingly to allow sub items.

rpastrana · 2024-10-30T12:55:30Z

helm/examples/metrics/elasticsearch_metrics.yaml

+#   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


to illustrate what I meant above, this could be settings.index.name

Reasonable. Change made. It also would allow keeping the index configuration separate in case we decide to push more policy into the sink.

rpastrana · 2024-10-30T12:58:25Z

system/metrics/sinks/elastic/elasticSink.cpp

@@ -45,11 +45,26 @@ ElasticMetricSink::ElasticMetricSink(const char *name, const IPropertyTree *pSet
    ignoreZeroMetrics = pSettingsTree->getPropBool("@ignoreZeroMetrics", true);
    pSettingsTree->getProp("@elasticHost", elasticHost);
    pSettingsTree->getProp("@indexName", indexName);
+
+    // Initialize standard suffixes
+    countMetricSuffix.append("_count");


is there an advantage to use append rather than set? (I'm obviously assuming these suffix strings are empty before line 50)

Yes they are empty. Use of append is based on code review comments from a previous PR from others on the team.

rpastrana · 2024-10-30T13:00:00Z

helm/examples/metrics/elasticsearch_metrics.yaml

+    - type: elastic
+      name: elasticsink
+      settings:
+        elasticHost: http://localhost:9200,


perhaps too picky, but "Host" implies the dns entry only, no protocol, port, etc.

For this I wanted to provide the full string to which the specific endpoint is appended. This makes it a bit easier than having to read multiple configuration values just to put them together internally. Any individual part can be changed in the string as easily as it could be in a separate config value.

However, thinking ahead, I can see where something like the port or other portion of the route could be a cluster wide parameter. For that reason, separating them into separate values makes sense.

kenrowland

Pleas see comments

kenrowland · 2024-10-30T13:34:36Z

helm/examples/metrics/README.md

+data types to be stored in the index in their native types. Without dynamic mapping, the ElasticSearch
+default mapping does not properly map value to unsigned 64-bit integers. 
+
+To create an index with dynamic mapping, use the following object when creating the index:


Currently no plans to create the index from the sink. If that was something we added, there are several considerations that must be taken into account. These are

If the cluster is using the same index for all metric reporting, creating and configuring the index must be coordinated somehow. If each component creates their own, there still needs to be coordination.

The sink would also need to add mapping templates in order to map values to the correct type. This could still use wildcard dynamic templates, or could be specific to the metrics registered for each metric framework instance

If the sink creates the index, we need to worry about life cycle management.

I felt like just telling the sink what index to use allows the policy of the ElasticSearch instance to remain there instead of pushing it down to the sink.

If we decide to push that responsibility to the sink, we can add it as an improvement addressing the concerns from above

kenrowland · 2024-10-30T13:40:44Z

helm/examples/metrics/README.md

+<Environment>
+    <Software>
+        <metrics name="mymetricsconfig">
+            <sinks name="elastic" type="elastic">


Sounds reasonable. I'll change that.

kenrowland · 2024-10-30T13:47:46Z

helm/examples/metrics/elasticsearch_metrics.yaml

+    - type: elastic
+      name: elasticsink
+      settings:
+        elasticHost: http://localhost:9200,


For this I wanted to provide the full string to which the specific endpoint is appended. This makes it a bit easier than having to read multiple configuration values just to put them together internally. Any individual part can be changed in the string as easily as it could be in a separate config value.

However, thinking ahead, I can see where something like the port or other portion of the route could be a cluster wide parameter. For that reason, separating them into separate values makes sense.

kenrowland · 2024-10-30T13:49:35Z

system/metrics/sinks/elastic/elasticSink.cpp

@@ -45,11 +45,26 @@ ElasticMetricSink::ElasticMetricSink(const char *name, const IPropertyTree *pSet
    ignoreZeroMetrics = pSettingsTree->getPropBool("@ignoreZeroMetrics", true);
    pSettingsTree->getProp("@elasticHost", elasticHost);
    pSettingsTree->getProp("@indexName", indexName);
+
+    // Initialize standard suffixes
+    countMetricSuffix.append("_count");


Yes they are empty. Use of append is based on code review comments from a previous PR from others on the team.

kenrowland · 2024-10-31T19:40:05Z

helm/examples/metrics/elasticsearch_metrics.yaml

+#   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


Reasonable. Change made. It also would allow keeping the index configuration separate in case we decide to push more policy into the sink.

rpastrana

@kenrowland a few comments/questions

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

helm/examples/metrics/elasticsearch_metrics.yaml

@@ -0,0 +1,36 @@
+#
+# Defines a elastic sink for reporting metrics to an ElasticSearch instance


Trivial. Should be "Defines an..."

helm/examples/metrics/elasticsearch_metrics.yaml

rpastrana · 2024-11-05T14:18:41Z

system/metrics/sinks/elastic/elasticSink.cpp

+
+    Owned<IPropertyTree> pHostConfigTree = pSettingsTree->getPropTree("host");
+
+    pHostConfigTree->getProp("@name", hostName);


I don't remember the exact iptree behavior, but what happens if pSettingsTree does not contain a "host" branch? If we're not guaranteed accessing pHostConfigTree->getProp is safe, let's protect this line.

rpastrana · 2024-11-05T14:20:14Z

system/metrics/sinks/elastic/elasticSink.cpp

+
+    if (!pHostConfigTree->getProp("@protocol", hostProtocol))
+    {
+        hostProtocol.append("https");


this is a reasonable default, but perhaps not the expected default. If this default is not clearly declared to the admin/user somewhere, let's add something to make sure it is.

rpastrana · 2024-11-05T14:24:59Z

system/metrics/sinks/elastic/elasticSink.cpp

+    }
+    else
+    {
+        WARNLOG("ElasticMetricSink: Host configuration is invalid");


this commit doesn't show me how the sink will behave if we fall into this case. I see there's a "configurationValid" variable, but what happens when it's set to false? Does the sink simply refuse to report metrics?

Haven't quite decided how to handle this case yet. Since this is a periodic sink, it may be appropriate to add a boolean return code to the prepareToStartCollecting method. That would prevent the base class from starting the collection thread. Or, maybe it just returns each time the doCollection method is returned. This PR was only concentrating on configuration.

Understood. I think the question is, do we want to fail the load process and force admins to fix the misconfiguration immediately, or allow the components to load and report the error in logs. There's arguments for both, but if your sink cannot operate w/ the given misconfiguration we might consider error'ing out at load time.

I am of the belief that we do not fail the load process. We log the appropriate warning and just don't collect metrics. That support should be added when we actually start the collection process.

kenrowland · 2024-11-06T13:24:00Z

@ghalliday Please merge

ghalliday

A few minor comments, and one problem with the suffixes.

ghalliday · 2024-11-11T16:37:05Z

helm/examples/metrics/README.md

+* 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)
+* histogramMetricSuffix - The suffix used to identify histogram metrics. (default: histogram)


question: Does the elastic stack support mtls?

ghalliday · 2024-11-11T16:37:48Z

helm/examples/metrics/elasticsearch_metrics.yaml

+    - type: elastic
+      name: myelasticsink
+      settings:
+        countMetricSuffix: count,


I assume the comma here and next line are include by mistake.

ghalliday · 2024-11-11T16:40:59Z

system/metrics/sinks/elastic/elasticSink.cpp

+    Owned<IPropertyTree> pHostConfigTree = pSettingsTree->getPropTree("host");
+    if (pHostConfigTree)
+    {
+        pHostConfigTree->getProp("@name", hostName);


minor future point: If the tree is guaranteed to stay active then more common is to use queryProp() and avoid cloning the strings. In this case the code is only executed once, so it doesn't matter.

const char * protocol = pHostConfigTree->queryProp("@protocol"); if (!protocol) protocol = "https";

ghalliday · 2024-11-11T16:44:06Z

system/metrics/sinks/elastic/elasticSink.cpp

+    histogramMetricSuffix.append("histogram");
+
+    // See if any are overridden
+    pSettingsTree->getProp("@countMetricSuffix", countMetricSuffix);


This code does not work. getProp() appends to the existing value, it does not replace it. I am not sure why this code uses a different pattern from the code above that gets the hostname.

ghalliday · 2024-11-11T16:45:29Z

helm/examples/metrics/README.md

+    "dynamic_templates": [
+      {
+        "hpcc_metric_count_to_unsigned_long": {
+          "match": "*_count",


Will this cause problems if other, non-HPCC, components are publishing values with these suffixes to elastic stack?

ghalliday · 2024-11-11T16:45:50Z

system/metrics/sinks/elastic/elasticSink.cpp

+    configurationValid = !elasticHostUrl.isEmpty() && !indexName.isEmpty();
+
+    // Initialize standard suffixes
+    countMetricSuffix.append("count");


Is it likely that anyone is going to want to change these? Under what situation?

kenrowland requested a review from rpastrana October 29, 2024 19:23

rpastrana reviewed Oct 30, 2024

View reviewed changes

kenrowland commented Oct 31, 2024

View reviewed changes

kenrowland requested a review from rpastrana October 31, 2024 19:40

Addressed review comments

a3dd106

kenrowland force-pushed the HPCC-32734 branch from 885790b to a3dd106 Compare November 4, 2024 19:52

rpastrana reviewed Nov 5, 2024

View reviewed changes

Addressed additional comments

cd1fc60

kenrowland requested a review from rpastrana November 5, 2024 17:22

rpastrana approved these changes Nov 5, 2024

View reviewed changes

ghalliday requested changes Nov 11, 2024

View reviewed changes

		@@ -0,0 +1,36 @@
		#
		# Defines a elastic sink for reporting metrics to an ElasticSearch instance


		Owned<IPropertyTree> pHostConfigTree = pSettingsTree->getPropTree("host");

		pHostConfigTree->getProp("@name", hostName);

HPCC-32734 Add ElasticSearch metric sink configuration #19247

Are you sure you want to change the base?

HPCC-32734 Add ElasticSearch metric sink configuration #19247

Conversation

kenrowland commented Oct 29, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Oct 29, 2024

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

Choose a reason for hiding this comment

kenrowland 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

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

kenrowland Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

kenrowland commented Nov 6, 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

kenrowland commented Oct 29, 2024 •

edited

Loading

kenrowland Nov 5, 2024 •

edited

Loading