From accbec5adeccc134531ed2c98795fb1aa5b2ac5d Mon Sep 17 00:00:00 2001
From: Yingge He <yinggeh@nvidia.com>
Date: Wed, 30 Oct 2024 12:55:03 -0700
Subject: [PATCH] Address comments

---
 src/metric_model_reporter.cc |  9 +++++----
 src/metric_model_reporter.h  |  4 +++-
 src/metrics.cc               | 14 ++++++++------
 src/metrics.h                |  6 ++++--
 src/model_config_utils.cc    | 14 +++++++-------
 5 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/src/metric_model_reporter.cc b/src/metric_model_reporter.cc
index 4bd3e4ed3..62533392e 100644
--- a/src/metric_model_reporter.cc
+++ b/src/metric_model_reporter.cc
@@ -80,12 +80,13 @@ MetricReporterConfig::ParseConfig(
   for (const auto& metric_control : model_metrics.metric_control()) {
     const std::string& family_name =
         metric_control.metric_identifier().family();
-    // Copy protobuf RepeatedField to std::vector
-    const auto& buckets_proto = metric_control.histogram_options().buckets();
-    const prometheus::Histogram::BucketBoundaries buckets(
-        buckets_proto.begin(), buckets_proto.end());
 
+    // If family name exists, override with new options.
     if (metric_map_.find(family_name) != metric_map_.end()) {
+      // Copy protobuf RepeatedField to std::vector
+      const auto& buckets_proto = metric_control.histogram_options().buckets();
+      const prometheus::Histogram::BucketBoundaries buckets(
+          buckets_proto.begin(), buckets_proto.end());
       histogram_options_[metric_map_.at(family_name)] = buckets;
     }
   }
diff --git a/src/metric_model_reporter.h b/src/metric_model_reporter.h
index 9620ffdc9..5ca1e7d9b 100644
--- a/src/metric_model_reporter.h
+++ b/src/metric_model_reporter.h
@@ -79,7 +79,9 @@ struct MetricReporterConfig {
   bool is_decoupled_ = false;
 
  private:
-  // Maps the metric fullname to its lookup key.
+  // Maps the metric family fullname to its lookup key. This field is required
+  // to find the lookup key given its fullname when users specify in the custom
+  // metric configuration "ModelMetrics".
   // All new histograms must update the map.
   const std::unordered_map<std::string, std::string> metric_map_ = {
       {"nv_inference_first_response_histogram_ms", kFirstResponseHistogram}};
diff --git a/src/metrics.cc b/src/metrics.cc
index f9b141b73..ed66d7781 100644
--- a/src/metrics.cc
+++ b/src/metrics.cc
@@ -109,12 +109,6 @@ Metrics::Metrics()
                     "execution per-model.")
               .Register(*registry_)),
 
-      inf_first_response_histogram_ms_family_(
-          prometheus::BuildHistogram()
-              .Name("nv_inference_first_response_histogram_ms")
-              .Help("Duration from request to first response in milliseconds")
-              .Register(*registry_)),
-
       model_load_time_family_(prometheus::BuildGauge()
                                   .Name("nv_model_load_duration_secs")
                                   .Help("Model load time in seconds")
@@ -155,6 +149,14 @@ Metrics::Metrics()
                     "microseconds")
               .Register(*registry_)),
 
+      // Histograms
+      // New histograms must be added to MetricReporterConfig.metric_map_
+      inf_first_response_histogram_ms_family_(
+          prometheus::BuildHistogram()
+              .Name("nv_inference_first_response_histogram_ms")
+              .Help("Duration from request to first response in milliseconds")
+              .Register(*registry_)),
+
       // Summaries
       inf_request_summary_us_family_(
           prometheus::BuildSummary()
diff --git a/src/metrics.h b/src/metrics.h
index af983cdca..ac04ebebc 100644
--- a/src/metrics.h
+++ b/src/metrics.h
@@ -312,8 +312,6 @@ class Metrics {
   prometheus::Family<prometheus::Counter>&
       inf_compute_output_duration_us_family_;
   prometheus::Family<prometheus::Gauge>& inf_pending_request_count_family_;
-  prometheus::Family<prometheus::Histogram>&
-      inf_first_response_histogram_ms_family_;
   prometheus::Family<prometheus::Gauge>& model_load_time_family_;
 
   prometheus::Family<prometheus::Gauge>& pinned_memory_pool_total_family_;
@@ -330,6 +328,10 @@ class Metrics {
   prometheus::Family<prometheus::Counter>& cache_num_misses_model_family_;
   prometheus::Family<prometheus::Counter>& cache_miss_duration_us_model_family_;
 
+  // Histograms
+  prometheus::Family<prometheus::Histogram>&
+      inf_first_response_histogram_ms_family_;
+
   // Summaries
   prometheus::Family<prometheus::Summary>& inf_request_summary_us_family_;
   prometheus::Family<prometheus::Summary>& inf_queue_summary_us_family_;
diff --git a/src/model_config_utils.cc b/src/model_config_utils.cc
index ef915c785..cfb2368b3 100644
--- a/src/model_config_utils.cc
+++ b/src/model_config_utils.cc
@@ -1624,18 +1624,18 @@ ValidateModelConfig(
   if (config.has_model_metrics()) {
 #ifdef TRITON_ENABLE_METRICS
     for (const auto& metric_control : config.model_metrics().metric_control()) {
-      if (metric_control.has_metric_identifier()) {
-        if (metric_control.metric_identifier().family().empty()) {
-          return Status(
-              Status::Code::INVALID_ARG,
-              "metric_identifier must specify 'family'");
-        }
-      } else {
+      if (!metric_control.has_metric_identifier()) {
         return Status(
             Status::Code::INVALID_ARG,
             "model_control must specify 'metric_identifier'");
       }
 
+      if (metric_control.metric_identifier().family().empty()) {
+        return Status(
+            Status::Code::INVALID_ARG,
+            "metric_identifier must specify 'family'");
+      }
+
       if (!metric_control.has_histogram_options()) {
         return Status(
             Status::Code::INVALID_ARG,