Remove TRITONSERVER_MetricCollect API

Author: yinggeh

include/triton/core/tritonserver.h

@@ -2693,7 +2693,6 @@ TRITONSERVER_DECLSPEC struct TRITONSERVER_Error* TRITONSERVER_MetricArgsDelete(
 /// \param family The metric family to add this new metric to.
 /// \param labels The array of labels to associate with this new metric.
 /// \param label_count The number of labels.
-/// \param buckets Monotonically increasing values representing the
 /// bucket boundaries. For histogram only.
 /// \return a TRITONSERVER_Error indicating success or failure.
 TRITONSERVER_DECLSPEC struct TRITONSERVER_Error* TRITONSERVER_MetricNew(
@@ -2769,18 +2768,6 @@ TRITONSERVER_DECLSPEC struct TRITONSERVER_Error* TRITONSERVER_MetricIncrement(
 TRITONSERVER_DECLSPEC struct TRITONSERVER_Error* TRITONSERVER_MetricSet(
     struct TRITONSERVER_Metric* metric, double value);
 
-/// Collect metrics.
-/// Supports metrics of kind TRITONSERVER_METRIC_KIND_COUNTER,
-/// TRITONSERVER_METRIC_KIND_GAUGE, TRITONSERVER_METRIC_KIND_HISTOGRAM and
-/// returns TRITONSERVER_ERROR_UNSUPPORTED for unsupported
-/// TRITONSERVER_MetricKind.
-///
-/// \param metric The metric object to collect.
-/// \param value Returns the current value of the metric object.
-/// \return a TRITONSERVER_Error indicating success or failure.
-TRITONSERVER_DECLSPEC struct TRITONSERVER_Error* TRITONSERVER_MetricCollect(
-    struct TRITONSERVER_Metric* metric, void* value);
-
 /// Get the TRITONSERVER_MetricKind of metric of its corresponding family.
 ///
 /// \param metric The metric object to query.

src/metric_family.cc

@@ -101,7 +101,11 @@ MetricFamily::Add(
     case TRITONSERVER_METRIC_KIND_HISTOGRAM: {
       if (args == nullptr) {
         throw std::invalid_argument(
-            "Bucket boundaries not found in Metric constructor args.");
+            "Bucket boundaries not found in Metric args.");
+      }
+      if (args->kind() != TRITONSERVER_METRIC_KIND_HISTOGRAM) {
+        throw std::invalid_argument(
+            "Incorrect Metric args kind in histogram Metric constructor.");
       }
       auto histogram_family_ptr =
           reinterpret_cast<prometheus::Family<prometheus::Histogram>*>(family_);
@@ -370,40 +374,6 @@ Metric::Set(double value)
   return nullptr;  // Success
 }
 
-TRITONSERVER_Error*
-Metric::Collect(prometheus::ClientMetric* value)
-{
-  if (metric_ == nullptr) {
-    return TRITONSERVER_ErrorNew(
-        TRITONSERVER_ERROR_INTERNAL,
-        "Could not collect metric value. Metric has been invalidated.");
-  }
-
-  switch (kind_) {
-    case TRITONSERVER_METRIC_KIND_COUNTER: {
-      auto counter_ptr = reinterpret_cast<prometheus::Counter*>(metric_);
-      *value = counter_ptr->Collect();
-      break;
-    }
-    case TRITONSERVER_METRIC_KIND_GAUGE: {
-      auto gauge_ptr = reinterpret_cast<prometheus::Gauge*>(metric_);
-      *value = gauge_ptr->Collect();
-      break;
-    }
-    case TRITONSERVER_METRIC_KIND_HISTOGRAM: {
-      auto histogram_ptr = reinterpret_cast<prometheus::Histogram*>(metric_);
-      *value = histogram_ptr->Collect();
-      break;
-    }
-    default:
-      return TRITONSERVER_ErrorNew(
-          TRITONSERVER_ERROR_UNSUPPORTED,
-          "Unsupported TRITONSERVER_MetricKind");
-  }
-
-  return nullptr;  // Success
-}
-
 }}  // namespace triton::core
 
 #endif  // TRITON_ENABLE_METRICS

src/metric_family.h

@@ -50,12 +50,12 @@ class TritonServerMetricArgs {
 
   void* SetHistogramArgs(const double* buckets, uint64_t bucket_count)
   {
-    kind_ = TRITONSERVER_MetricKind::TRITONSERVER_METRIC_KIND_HISTOGRAM;
+    kind_ = TRITONSERVER_METRIC_KIND_HISTOGRAM;
     buckets_.resize(bucket_count);
     std::memcpy(buckets_.data(), buckets, sizeof(double) * bucket_count);
     return nullptr;
   }
-
+  TRITONSERVER_MetricKind kind() const { return kind_; }
   const std::vector<double>& buckets() const { return buckets_; }
 
  private:
@@ -78,7 +78,7 @@ class MetricFamily {
 
   void* Add(
       std::map<std::string, std::string> label_map, Metric* metric,
-      const TritonServerMetricArgs* buckets);
+      const TritonServerMetricArgs* args);
   void Remove(void* prom_metric, Metric* metric);
 
   int NumMetrics()
@@ -125,7 +125,6 @@ class Metric {
   TRITONSERVER_Error* Increment(double value);
   TRITONSERVER_Error* Set(double value);
   TRITONSERVER_Error* Observe(double value);
-  TRITONSERVER_Error* Collect(prometheus::ClientMetric* value);
 
   // If a MetricFamily is deleted before its dependent Metric, we want to
   // invalidate the references so we don't access invalid memory.

src/test/metrics_api_test.cc

@@ -234,29 +234,61 @@ MetricAPIHelper(TRITONSERVER_Metric* metric, TRITONSERVER_MetricKind kind)
 }
 
 void
-HistogramAPIHelper(TRITONSERVER_Metric* metric)
+HistogramAPIHelper(
+    TRITONSERVER_Server* server, TRITONSERVER_Metric* metric,
+    std::vector<double> buckets, std::string metric_name,
+    std::string labels_str)
 {
-  // Observe
+  // Observe data
   std::vector<double> data{0.05, 1.5, 6.0};
-  std::vector<std::uint64_t> cumulative_counts = {1, 1, 2, 2, 3, 3};
   double sum = 0.0;
   for (auto datum : data) {
     FAIL_TEST_IF_ERR(
         TRITONSERVER_MetricSet(metric, datum), "observe metric value");
     sum += datum;
   }
+  std::vector<std::uint64_t> cumulative_counts = {1, 1, 2, 2, 3, 3};
+  ASSERT_EQ(buckets.size() + 1, cumulative_counts.size());
+
+  // Collect formatted output
+  std::string metrics_str;
+  GetMetrics(server, &metrics_str);
 
-  // Collect
-  prometheus::ClientMetric value;
-  FAIL_TEST_IF_ERR(
-      TRITONSERVER_MetricCollect(metric, &value),
-      "query metric value after observe");
-  auto hist = value.histogram;
-  ASSERT_EQ(hist.sample_count, data.size());
-  ASSERT_EQ(hist.sample_sum, sum);
-  ASSERT_EQ(hist.bucket.size(), cumulative_counts.size());
-  for (uint64_t i = 0; i < hist.bucket.size(); ++i) {
-    ASSERT_EQ(hist.bucket[i].cumulative_count, cumulative_counts[i]);
+  // All strings in this function are generated from ostringstream to make sure
+  // there is no trailing zeros. For example, std::to_string(7.55) is "7.550000"
+  // which is inconsistent with prometheus text_serializer output "7.55".
+
+  // custom_histogram_example_count{example1="histogram_label1",example2="histogram_label2"}
+  // 3
+  std::ostringstream count_ss;
+  count_ss << metric_name << "_count{" << labels_str << "} " << data.size();
+  ASSERT_EQ(NumMetricMatches(server, count_ss.str()), 1);
+
+  // custom_histogram_example_sum{example1="histogram_label1",example2="histogram_label2"}
+  // 7.55
+  std::ostringstream sum_ss;
+  sum_ss << metric_name << "_sum{" << labels_str << "} " << sum;
+  ASSERT_EQ(NumMetricMatches(server, sum_ss.str()), 1);
+
+  // custom_histogram_example_bucket{example1="histogram_label1",example2="histogram_label2"
+  std::ostringstream bucket_partial_ss;
+  bucket_partial_ss << metric_name << "_bucket{" << labels_str;
+  ASSERT_EQ(
+      NumMetricMatches(server, bucket_partial_ss.str()),
+      cumulative_counts.size());
+  for (uint64_t i = 0; i < cumulative_counts.size(); ++i) {
+    // custom_histogram_example_bucket{example1="histogram_label1",example2="histogram_label2",le="0.1"}
+    // 1
+    std::ostringstream le_ss;
+    if (i < buckets.size()) {
+      le_ss << "\"" << buckets[i] << "\"";
+    } else {
+      le_ss << "\"+Inf\"";
+    }
+    std::ostringstream bucket_ss;
+    bucket_ss << metric_name << "_bucket{" << labels_str
+              << ",le=" << le_ss.str() << "} " << cumulative_counts[i];
+    ASSERT_EQ(NumMetricMatches(server, bucket_ss.str()), 1);
   }
 }
 
@@ -431,7 +463,9 @@ TEST_F(MetricsApiTest, TestHistogramEndToEnd)
   FAIL_TEST_IF_ERR(TRITONSERVER_MetricArgsDelete(args), "delete metric args");
 
   // Run through metric APIs and assert correctness
-  HistogramAPIHelper(metric);
+  std::string labels_str =
+      "example1=\"histogram_label1\",example2=\"histogram_label2\"";
+  HistogramAPIHelper(server_, metric, buckets, name, labels_str);
 
   // Assert custom metric is reported and found in output
   ASSERT_EQ(NumMetricMatches(server_, description), 1);

src/tritonserver.cc

@@ -3525,18 +3525,6 @@ TRITONSERVER_MetricSet(TRITONSERVER_Metric* metric, double value)
 #endif  // TRITON_ENABLE_METRICS
 }
 
-TRITONSERVER_Error*
-TRITONSERVER_MetricCollect(TRITONSERVER_Metric* metric, void* value)
-{
-#ifdef TRITON_ENABLE_METRICS
-  return reinterpret_cast<tc::Metric*>(metric)->Collect(
-      reinterpret_cast<prometheus::ClientMetric*>(value));
-#else
-  return TRITONSERVER_ErrorNew(
-      TRITONSERVER_ERROR_UNSUPPORTED, "metrics not supported");
-#endif  // TRITON_ENABLE_METRICS
-}
-
 TRITONSERVER_Error*
 TRITONSERVER_GetMetricKind(
     TRITONSERVER_Metric* metric, TRITONSERVER_MetricKind* kind)