Add ts::Metrics::StaticString and remove RecRegisterStatString (#12591)

* Add ts::Metrics::StaticString and remove RecRegisterStatString

* include optional

* only check static strings for certain rec types

* Use std::unordered_map for string storage

Author: cmcfarlen

include/records/RecCore.h

@@ -84,10 +84,6 @@ RecErrT _RecRegisterStatFloat(RecT rec_type, const char *name, RecFloat data_def
 #define RecRegisterStatFloat(rec_type, name, data_default, persist_type) \
   _RecRegisterStatFloat((rec_type), (name), (data_default), REC_PERSISTENCE_TYPE(persist_type))
 
-RecErrT _RecRegisterStatString(RecT rec_type, const char *name, RecStringConst data_default, RecPersistT persist_type);
-#define RecRegisterStatString(rec_type, name, data_default, persist_type) \
-  _RecRegisterStatString((rec_type), (name), (data_default), REC_PERSISTENCE_TYPE(persist_type))
-
 RecErrT _RecRegisterStatCounter(RecT rec_type, const char *name, RecCounter data_default, RecPersistT persist_type);
 #define RecRegisterStatCounter(rec_type, name, data_default, persist_type) \
   _RecRegisterStatCounter((rec_type), (name), (data_default), REC_PERSISTENCE_TYPE(persist_type))

include/tsutil/Metrics.h

@@ -33,6 +33,7 @@
 #include <string>
 #include <string_view>
 #include <variant>
+#include <optional>
 
 #include "swoc/MemSpan.h"
 
@@ -535,6 +536,41 @@ class Metrics
 
   }; // class Counter
 
+  class StaticString
+  {
+  public:
+    using StringStorage = std::unordered_map<std::string, std::string>;
+    using iterator      = StringStorage::iterator;
+
+    static void
+    createString(const std::string &name, const std::string_view value)
+    {
+      auto &instance = Metrics::StaticString::instance();
+      return instance._createString(name, value);
+    }
+
+    static StaticString &instance();
+
+    iterator
+    begin()
+    {
+      return _strings.begin();
+    }
+    iterator
+    end()
+    {
+      return _strings.end();
+    };
+
+    std::optional<std::string_view> lookup(const std::string &name);
+
+  private:
+    void _createString(const std::string &name, const std::string_view value);
+
+    StringStorage      _strings;
+    mutable std::mutex _mutex;
+  };
+
   /**
    * Derive metrics by summing a set of other metrics.
    *

src/proxy/logging/LogStandalone.cc

@@ -33,6 +33,7 @@
 #include "tscore/ink_sys_control.h"
 #include "tscore/signals.h"
 #include "tscore/Layout.h"
+#include "tsutil/Metrics.h"
 #include "proxy/shared/DiagsConfig.h"
 
 // Needs LibRecordsConfigInit()
@@ -89,13 +90,13 @@ initialize_records()
   // Define version info records
   //
   auto &version = AppVersionInfo::get_version();
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.short", version.version(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.long", version.full_version(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_number", version.build_number(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_time", version.build_time(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_date", version.build_date(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_machine", version.build_machine(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_person", version.build_person(), RECP_NON_PERSISTENT);
+  ts::Metrics::StaticString::createString("proxy.process.version.server.short", version.version());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.long", version.full_version());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_number", version.build_number());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_time", version.build_time());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_date", version.build_date());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_machine", version.build_machine());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_person", version.build_person());
 }
 
 /*-------------------------------------------------------------------------

src/records/P_RecCore.cc

@@ -66,15 +66,6 @@ _RecRegisterStatFloat(RecT rec_type, const char *name, RecFloat data_default, Re
   REC_REGISTER_STAT_XXX(rec_float, RECD_FLOAT);
 }
 
-RecErrT
-_RecRegisterStatString(RecT rec_type, const char *name, RecStringConst data_in, RecPersistT persist_type)
-{
-  // NOTE(cmcfarlen): RecRegisterState calls RecDataSet which call strdup on the string data.
-  // therefore, this const cast will not be modified nor escape the stack past here.
-  char *data_default = const_cast<char *>(data_in);
-  REC_REGISTER_STAT_XXX(rec_string, RECD_STRING);
-}
-
 RecErrT
 _RecRegisterStatCounter(RecT rec_type, const char *name, RecCounter data_default, RecPersistT persist_type)
 {

src/records/RecCore.cc

@@ -547,6 +547,23 @@ RecLookupRecord(const char *name, void (*callback)(const RecRecord *, void *), v
     if (lock) {
       ink_rwlock_unlock(&g_records_rwlock);
     }
+
+    // Also check for StaticString metrics
+    if (err == REC_ERR_FAIL) {
+      auto &strings = ts::Metrics::StaticString::instance();
+
+      if (auto m = strings.lookup(std::string{name}); m) {
+        RecRecord r;
+        r.rec_type                = RECT_PLUGIN;
+        r.data_type               = RECD_STRING;
+        r.name                    = name;
+        r.data.rec_string         = const_cast<char *>(m->data());
+        r.data_default.rec_string = const_cast<char *>(m->data());
+
+        callback(&r, data);
+        err = REC_ERR_OKAY;
+      }
+    }
   }
 
   return err;
@@ -556,7 +573,6 @@ RecErrT
 RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)(const RecRecord *, void *), void *data,
                          bool /* lock ATS_UNUSED */)
 {
-  int   num_records;
   Regex regex;
 
   if (!regex.compile(match, RE_CASE_INSENSITIVE | RE_UNANCHORED)) {
@@ -566,22 +582,36 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)(
   if ((rec_type & (RECT_PROCESS | RECT_NODE | RECT_PLUGIN))) {
     // First find the new metrics, this is a bit of a hack, because we still use the old
     // librecords callback with a "pseudo" record.
-    RecRecord tmp;
-
-    tmp.rec_type = RECT_PROCESS;
-
     for (auto &&[name, type, val] : ts::Metrics::instance()) {
       if (regex.exec(name.data())) {
+        RecRecord tmp;
+
+        tmp.rec_type = RECT_PROCESS;
+
         tmp.name         = name.data();
         tmp.data_type    = type == ts::Metrics::MetricType::COUNTER ? RECD_COUNTER : RECD_INT;
         tmp.data.rec_int = val;
         callback(&tmp, data);
       }
     }
-    // Fall through to return any matching string metrics
+    // Finally check string metrics
+    for (auto &&[name, value] : ts::Metrics::StaticString::instance()) {
+      if (regex.exec(name)) {
+        RecRecord tmp;
+
+        tmp.rec_type = RECT_PROCESS;
+
+        tmp.name      = name.data();
+        tmp.data_type = RECD_STRING;
+        // NOTE(cmcfarlen): unfortunate relic here that the callbacks expect a non-const rec_string
+        // This should be temp until traffic_ctl uses ts::Metrics directly
+        tmp.data.rec_string = const_cast<char *>(value.c_str());
+        callback(&tmp, data);
+      }
+    }
   }
 
-  num_records = g_num_records;
+  int num_records = g_num_records;
   for (int i = 0; i < num_records; i++) {
     RecRecord *r = &(g_records[i]);
 

src/records/RecordsConfigUtils.cc

@@ -102,10 +102,6 @@ initialize_record(const RecordElement *record, void *)
       RecRegisterStatFloat(type, record->name, tempFloat, RECP_NON_PERSISTENT);
       break;
 
-    case RECD_STRING:
-      RecRegisterStatString(type, record->name, (RecString)record->value, RECP_NON_PERSISTENT);
-      break;
-
     case RECD_COUNTER:
       tempCounter = static_cast<RecCounter>(ink_atoi64(record->value));
       RecRegisterStatCounter(type, record->name, tempCounter, RECP_NON_PERSISTENT);

src/records/test_RecordsConfig.cc

@@ -47,8 +47,6 @@ RecordsConfigRegister()
   RecRegisterConfigCounter(RECT_CONFIG, "proxy.config.link_test_3", 0, RECU_DYNAMIC, RECC_NULL, nullptr);
 
   // NODE
-  RecRegisterStatString(RECT_NODE, "proxy.node.cb_test_1", "cb_test_1__original", RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_NODE, "proxy.node.cb_test_2", "cb_test_2__original", RECP_NON_PERSISTENT);
   RecRegisterStatInt(RECT_NODE, "proxy.node.cb_test_int", 0, RECP_NON_PERSISTENT);
   RecRegisterStatFloat(RECT_NODE, "proxy.node.cb_test_float", 0.0f, RECP_NON_PERSISTENT);
   RecRegisterStatCounter(RECT_NODE, "proxy.node.cb_test_count", 0, RECP_NON_PERSISTENT);

src/traffic_server/traffic_server.cc

@@ -48,6 +48,7 @@
 
 #include "ts/ts.h" // This is sadly needed because of us using TSThreadInit() for some reason.
 #include "swoc/swoc_file.h"
+#include "tsutil/Metrics.h"
 
 #include <syslog.h>
 #include <algorithm>
@@ -712,13 +713,13 @@ initialize_records()
   // Define version info records
   //
   auto &version = AppVersionInfo::get_version();
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.short", version.version(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.long", version.full_version(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_number", version.build_number(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_time", version.build_time(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_date", version.build_date(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_machine", version.build_machine(), RECP_NON_PERSISTENT);
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.build_person", version.build_person(), RECP_NON_PERSISTENT);
+  ts::Metrics::StaticString::createString("proxy.process.version.server.short", version.version());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.long", version.full_version());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_number", version.build_number());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_time", version.build_time());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_date", version.build_date());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_machine", version.build_machine());
+  ts::Metrics::StaticString::createString("proxy.process.version.server.build_person", version.build_person());
 }
 
 void
@@ -2030,8 +2031,7 @@ main(int /* argc ATS_UNUSED */, const char **argv)
     Machine::init(hostname, &machine_addr.sa);
   }
 
-  RecRegisterStatString(RECT_PROCESS, "proxy.process.version.server.uuid", (char *)Machine::instance()->process_uuid.getString(),
-                        RECP_NON_PERSISTENT);
+  ts::Metrics::StaticString::createString("proxy.process.version.server.uuid", Machine::instance()->process_uuid.getString());
 
   res_track_memory = RecGetRecordInt("proxy.config.res_track_memory").value_or(0);
 

src/tsutil/Metrics.cc

@@ -24,6 +24,7 @@
 #include "tsutil/Assert.h"
 #include <memory>
 #include <mutex>
+#include <optional>
 #include <variant>
 #include <vector>
 #include "tsutil/Metrics.h"
@@ -292,4 +293,33 @@ Metrics::Derived::update_derived()
   details::DerivativeMetrics::instance().update();
 }
 
+Metrics::StaticString &
+Metrics::StaticString::instance()
+{
+  static Metrics::StaticString i{};
+  return i;
+}
+
+void
+Metrics::StaticString::_createString(const std::string &name, const std::string_view value)
+{
+  std::lock_guard l(_mutex);
+
+  _strings[name] = value;
+}
+
+std::optional<std::string_view>
+Metrics::StaticString::lookup(const std::string &name)
+{
+  std::lock_guard                 l(_mutex);
+  auto                            it = _strings.find(name);
+  std::optional<std::string_view> result{};
+
+  if (it != _strings.end()) {
+    result = it->second;
+  }
+
+  return result;
+}
+
 } // namespace ts