[trafficserver] branch NewAPIMetricsPOC updated: Adding some comments, because you will ask

[zwoop]

This is an automated email from the ASF dual-hosted git repository.

zwoop pushed a commit to branch NewAPIMetricsPOC
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/NewAPIMetricsPOC by this push:
     new 60d9fdd4d6 Adding some comments, because you will ask
60d9fdd4d6 is described below

commit 60d9fdd4d610397e7226aaf810382cf29ff0997f
Author: Leif Hedstrom <zw...@apache.org>
AuthorDate: Thu Jul 6 15:42:50 2023 -0600

    Adding some comments, because you will ask
---
 include/api/Metrics.h | 6 +++++-
 src/api/Metrics.cc    | 5 ++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/api/Metrics.h b/include/api/Metrics.h
index 0ab82324b8..1c6a7e95a3 100644
--- a/include/api/Metrics.h
+++ b/include/api/Metrics.h
@@ -40,7 +40,7 @@ class Metrics
 {
 private:
   using self_type  = Metrics;
-  using IdType     = int32_t;
+  using IdType     = int32_t; // Could be a tuple, but one way or another, 
they have to be combined to an int32_t.
   using AtomicType = std::atomic<int64_t>;
 
 public:
@@ -69,6 +69,10 @@ public:
   // Singleton
   static Metrics &getInstance();
 
+  // Yes, we don't return objects here, but rather ID's and atomic's directly. 
We could
+  // make these objects, but that's a lot of object instances to manage, and 
right now the
+  // ID in the containers is very small and sufficient to work with. But 
agreed, it's not
+  // very C++-like (but compatible with old librecords ATS code!).
   IdType newMetric(const std::string_view name);
   IdType lookup(const std::string_view name);
   AtomicType *lookup(IdType id) const;
diff --git a/src/api/Metrics.cc b/src/api/Metrics.cc
index c87239b61a..fc500df46a 100644
--- a/src/api/Metrics.cc
+++ b/src/api/Metrics.cc
@@ -97,6 +97,9 @@ Metrics::lookup(IdType id) const
   return &(atomics[std::get<1>(idx)]);
 }
 
+// ToDo: This is probably not great, and likely we should have some better
+// way exposing iterators over the Metrics. That avoids this ugly dependency
+// between librecords and this code.
 void
 Metrics::recordsDump(RecDumpEntryCb callback, void *edata)
 {
@@ -123,7 +126,7 @@ Metrics::recordsDump(RecDumpEntryCb callback, void *edata)
     for (int j = 0; j < off_max; ++j) {
       datum.rec_int = metrics[j].load();
       // ToDo: The recordtype here is fine for now, but we should probably 
make this generic
-      callback(RECT_PLUGIN, edata, 1, std::get<0>(names[i]).c_str(), 
TS_RECORDDATATYPE_INT, &datum);
+      callback(RECT_PLUGIN, edata, 1, std::get<0>(names[j]).c_str(), 
TS_RECORDDATATYPE_INT, &datum);
     }
   }
 }