This is an automated email from the ASF dual-hosted git repository.
cmcfarlen 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 1ecafd9236 iterators!
1ecafd9236 is described below
commit 1ecafd9236280eceec3f5915c2976ae844ec6ab7
Author: Chris McFarlen <[email protected]>
AuthorDate: Fri Jul 7 11:49:05 2023 -0500
iterators!
---
include/api/Metrics.h | 85 +++++++++++++++++++++++++++++++++++++++++++++++++
src/api/Metrics.cc | 48 ++++++++++++++--------------
src/api/test_Metrics.cc | 36 ++++++++++++++++++---
3 files changed, 141 insertions(+), 28 deletions(-)
diff --git a/include/api/Metrics.h b/include/api/Metrics.h
index e85f30033e..1146fa22c3 100644
--- a/include/api/Metrics.h
+++ b/include/api/Metrics.h
@@ -119,6 +119,8 @@ public:
return (metric ? metric->load() : NOT_FOUND);
}
+ std::string_view get_name(IdType id) const;
+
void
set(IdType id, int64_t val)
{
@@ -139,6 +141,83 @@ public:
void recordsDump(RecDumpEntryCb callback, void *edata) const;
+ class iterator
+ {
+ public:
+ using iterator_category = std::input_iterator_tag;
+ using value_type = std::pair<std::string_view,
AtomicType::value_type>;
+ using difference_type = ptrdiff_t;
+ using pointer = value_type *;
+ using reference = value_type &;
+
+ iterator(const Metrics &m, IdType pos) : metrics(m), it(pos) {}
+
+ iterator &
+ operator++()
+ {
+ next();
+ return *this;
+ }
+
+ iterator
+ operator++(int)
+ {
+ iterator result = *this;
+ next();
+ return result;
+ }
+
+ value_type
+ operator*() const
+ {
+ return std::make_pair(metrics.get_name(it), metrics.lookup(it)->load());
+ }
+
+ bool
+ operator==(const iterator &o) const
+ {
+ return std::addressof(metrics) == std::addressof(o.metrics) && it ==
o.it;
+ }
+ bool
+ operator!=(const iterator &o) const
+ {
+ return std::addressof(metrics) != std::addressof(o.metrics) || it !=
o.it;
+ }
+
+ private:
+ void
+ next()
+ {
+ auto [blob, offset] = metrics._splitID(it);
+
+ if (++offset == METRICS_MAX_SIZE) {
+ ++blob;
+ offset = 0;
+ }
+
+ it = _makeId(blob, offset);
+ }
+
+ const Metrics &metrics;
+ IdType it;
+ };
+
+ iterator
+ begin() const
+ {
+ return iterator(*this, 0);
+ }
+
+ iterator
+ end() const
+ {
+ _mutex.lock();
+ int16_t blob = _cur_blob;
+ int16_t offset = _cur_off;
+ _mutex.unlock();
+ return iterator(*this, _makeId(blob, offset));
+ }
+
private:
static constexpr std::tuple<uint16_t, uint16_t>
_splitID(IdType value)
@@ -146,6 +225,12 @@ private:
return std::make_tuple(static_cast<uint16_t>(value >> 16),
static_cast<uint16_t>(value & 0xFFFF));
}
+ static constexpr IdType
+ _makeId(uint16_t blob, uint16_t offset)
+ {
+ return (blob << 16 | offset);
+ }
+
void _addBlob();
mutable std::mutex _mutex;
diff --git a/src/api/Metrics.cc b/src/api/Metrics.cc
index 44bbb94657..0303433b0a 100644
--- a/src/api/Metrics.cc
+++ b/src/api/Metrics.cc
@@ -102,33 +102,33 @@ Metrics::lookup(IdType id) const
return &((std::get<1>(*blob)[offset]));
}
-// 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.
+std::string_view
+Metrics::get_name(Metrics::IdType id) const
+{
+ auto [blob_ix, offset] = _splitID(id);
+ Metrics::MetricStorage *blob = _blobs[blob_ix];
+
+ // Do a sanity check on the ID, to make sure we don't index outside of the
realm of possibility.
+ if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) {
+ blob = _blobs[0];
+ offset = 0;
+ }
+
+ const std::string &result = std::get<0>(std::get<0>(*blob)[offset]);
+ return result;
+}
+
+// ToDo: While this is using iterators now, there is this unfortunate use of
.data()
+// that should probably go away. I had problems using const std::string& in
the iterator
+// value_type so this needs to be revisited.
void
Metrics::recordsDump(RecDumpEntryCb callback, void *edata) const
{
- int16_t off_max = METRICS_MAX_SIZE;
-
- // Capture these under the lock guard
- _mutex.lock();
- int16_t blob_ix = _cur_blob;
- int16_t off_ix = _cur_off;
- _mutex.unlock();
-
- for (int i = 0; i <= blob_ix; ++i) {
- auto blob = _blobs[i];
- auto &names = std::get<0>(*blob);
- auto &metrics = std::get<1>(*blob);
- RecData datum;
-
- if (i == blob_ix) {
- off_max = off_ix;
- }
- for (int j = 0; j < off_max; ++j) {
- datum.rec_int = metrics[j].load();
- callback(RECT_PLUGIN, edata, 1, std::get<0>(names[j]).c_str(),
TS_RECORDDATATYPE_INT, &datum);
- }
+ RecData datum;
+
+ for (auto [name, metric] : *this) {
+ datum.rec_int = metric;
+ callback(RECT_PLUGIN, edata, 1, name.data(), TS_RECORDDATATYPE_INT,
&datum);
}
}
diff --git a/src/api/test_Metrics.cc b/src/api/test_Metrics.cc
index 0c28f91fb3..099a34f943 100644
--- a/src/api/test_Metrics.cc
+++ b/src/api/test_Metrics.cc
@@ -30,11 +30,39 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]")
{
ts::Metrics m;
- auto fooid = m.newMetric("foo");
+ SECTION("iterator")
+ {
+ auto [name, value] = *m.begin();
+ REQUIRE(value == 0);
+ REQUIRE(name == "proxy.node.bad_id.metrics");
- REQUIRE(fooid == 1);
+ REQUIRE(m.begin() != m.end());
+ REQUIRE(++m.begin() == m.end());
- m.increment(fooid);
+ auto it = m.begin();
+ it++;
+ REQUIRE(it == m.end());
+ }
- m.recordsDump([](RecT, void *, int, const char *name, int value, RecData *)
{ printf("Fooo: %s: %d\n", name, value); }, nullptr);
+ SECTION("New metric")
+ {
+ auto fooid = m.newMetric("foo");
+
+ REQUIRE(fooid == 1);
+ REQUIRE(m.get_name(fooid) == "foo");
+
+ REQUIRE(m.get(fooid) == 0);
+ m.increment(fooid);
+ REQUIRE(m.get(fooid) == 1);
+ }
+
+ SECTION("dump")
+ {
+ m.recordsDump([](RecT, void *, int, const char *name, int value, RecData
*) { printf("Fooo: %s: %d\n", name, value); },
+ nullptr);
+
+ for (auto [name, metric] : m) {
+ std::cout << name << ": " << metric << "\n";
+ }
+ }
}
