[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1385: MINIFICPP-1901 Add agent identity to Prometheus metrics

[GitBox <gi...@apache.org>]

fgerlits commented on code in PR #1385:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1385#discussion_r959739177


##########
METRICS.md:
##########
@@ -46,77 +46,88 @@ To use the publisher a port should also be configured where the metrics will be
 
 	nifi.metrics.publisher.PrometheusMetricsPublisher.port=9936
 
-The last option defines which metric classes should be exposed through the metrics publisher in configured with a comma separated value:
+The following option defines which metric classes should be exposed through the metrics publisher in configured with a comma separated value:
 
 	# in minifi.properties
 
 	nifi.metrics.publisher.metrics=QueueMetrics,RepositoryMetrics,GetFileMetrics,DeviceInfoNode,FlowInformation
 
+An agent identifier should also be defined to identify which agent the metric is exposed from. If not set, the hostname is used as the identifier.
+
+	# in minifi.properties
+
+	nifi.metrics.publisher.agent.identifier=Agent1
+
 ## Metrics
 
 The following section defines the currently available metrics to be published by the MiNiFi C++ agent.
 
 NOTE: In Prometheus all metrics are extended with a `minifi_` prefix to mark the domain of the metric. For example the `connection_name` metric is published as `minifi_connection_name` in Prometheus.
 
+## Generic labels

Review Comment:
   this should either be one level lower (i.e., `###`), or without a section title
   
   as it is, it looks as if `QueueMetrics`, etc were sub-sections of `Generic labels`



##########
libminifi/src/utils/OsUtils.cpp:
##########
@@ -320,9 +316,11 @@ std::string OsUtils::getMachineArchitecture() {
   return "unknown";
 }
 
-}  // namespace utils
-}  // namespace minifi
-}  // namespace nifi
-}  // namespace apache
-}  // namespace org
+std::string OsUtils::getHostName() {
+  char hostname[1024];
+  hostname[1023] = '\0';
+  gethostname(hostname, 1023);
+  return {hostname};
+}

Review Comment:
   I don't know how often that happens, probably not very often, but if `gethostname()` returns an error (non-zero) status, then the return value will be a string containing garbage.  We should handle errors in a better way.



##########
extensions/prometheus/tests/PrometheusMetricsPublisherTest.cpp:
##########
@@ -98,11 +100,12 @@ TEST_CASE_METHOD(PrometheusPublisherTestFixtureWithDummyExposer, "Test adding me
     auto collection = stored_metric->Collect();
     for (const auto& metric_family : collection) {
       for (const auto& prometheus_metric : metric_family.metric) {
-        for (const auto& label : prometheus_metric.label) {
-          if (label.name == "metric_class") {
-            REQUIRE(ranges::find(valid_metrics_without_flow, label.value) != ranges::end(valid_metrics_without_flow));
-          }
-        }
+        auto metric_class_label_it = ranges::find_if(prometheus_metric.label, [](const auto& label) { return label.name == "metric_class"; });
+        REQUIRE(metric_class_label_it != ranges::end(prometheus_metric.label));
+        REQUIRE(ranges::find(valid_metrics_without_flow, metric_class_label_it->value) != ranges::end(valid_metrics_without_flow));

Review Comment:
   slightly shorter:
   ```suggestion
           REQUIRE(ranges::contains(valid_metrics_without_flow, metric_class_label_it->value));
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org