You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2023/07/07 19:16:09 UTC
[trafficserver] branch NewAPIMetricsPOC updated: Use the iterators to dump metrics, and some cleanup
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 c6fc5fb987 Use the iterators to dump metrics, and some cleanup
c6fc5fb987 is described below
commit c6fc5fb987d8c027d705b8ae1cffebbe4a74c3f6
Author: Leif Hedstrom <zw...@apache.org>
AuthorDate: Fri Jul 7 13:15:58 2023 -0600
Use the iterators to dump metrics, and some cleanup
---
include/api/Metrics.h | 80 +++++++++++++++++---------------------------
src/api/Metrics.cc | 27 +++++++++------
src/records/RecCore.cc | 7 +++-
src/traffic_server/InkAPI.cc | 4 +--
4 files changed, 54 insertions(+), 64 deletions(-)
diff --git a/include/api/Metrics.h b/include/api/Metrics.h
index 1146fa22c3..733c815bb6 100644
--- a/include/api/Metrics.h
+++ b/include/api/Metrics.h
@@ -31,8 +31,7 @@
#include <atomic>
#include <cstdint>
-#include "records/P_RecDefs.h"
-#include "ts/apidefs.h"
+#include "tscore/ink_assert.h"
namespace ts
{
@@ -68,19 +67,17 @@ public:
{
_blobs[0] = new MetricStorage();
ink_release_assert(_blobs[0]);
- ink_release_assert(0 == newMetric("proxy.node.bad_id.metrics")); // Reserve slot 0 for errors, this should always be 0
+ ink_release_assert(0 == newMetric("proxy.node.api.metrics.bad_id")); // Reserve slot 0 for errors, this should always be 0
}
// 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!).
+ // Yes, we don't return objects here, but rather ID's and atomic's directly. Treat
+ // the std::atomic<int64_t> as the underlying class for a single metric, and be happy.
IdType newMetric(const std::string_view name);
IdType lookup(const std::string_view name) const;
- AtomicType *lookup(IdType id) const;
+ AtomicType *lookup(IdType id, std::string_view *name = nullptr) const;
AtomicType &
operator[](IdType id)
@@ -94,7 +91,6 @@ public:
return lookup(name);
}
- // Inlined, for performance
int64_t
increment(IdType id, uint64_t val = 1)
{
@@ -103,6 +99,7 @@ public:
return (metric ? metric->fetch_add(val) : NOT_FOUND);
}
+ // ToDo: Do we even need these inc/dec functions?
int64_t
decrement(IdType id, uint64_t val = 1)
{
@@ -111,51 +108,32 @@ public:
return (metric ? metric->fetch_sub(val) : NOT_FOUND);
}
- int64_t
- get(IdType id) const
- {
- auto metric = lookup(id);
-
- return (metric ? metric->load() : NOT_FOUND);
- }
-
- std::string_view get_name(IdType id) const;
-
- void
- set(IdType id, int64_t val)
- {
- auto metric = lookup(id);
-
- if (metric) {
- metric->store(val);
- }
- }
+ std::string_view name(IdType id) const;
bool
valid(IdType id) const
{
auto [blob, entry] = _splitID(id);
- return (id >= 0 && blob <= _cur_blob && entry < METRICS_MAX_SIZE);
+ return (id >= 0 && ((blob < _cur_blob && entry < METRICS_MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off)));
}
- 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 value_type = std::tuple<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(const Metrics &m, IdType pos) : _metrics(m), _it(pos) {}
iterator &
operator++()
{
next();
+
return *this;
}
@@ -163,43 +141,38 @@ public:
operator++(int)
{
iterator result = *this;
+
next();
+
return result;
}
value_type
operator*() const
{
- return std::make_pair(metrics.get_name(it), metrics.lookup(it)->load());
+ std::string_view name;
+ auto metric = _metrics.lookup(_it, &name);
+
+ return std::make_tuple(name, metric->load());
}
bool
operator==(const iterator &o) const
{
- return std::addressof(metrics) == std::addressof(o.metrics) && it == o.it;
+ return _it == o._it && std::addressof(_metrics) == std::addressof(o._metrics);
}
+
bool
operator!=(const iterator &o) const
{
- return std::addressof(metrics) != std::addressof(o.metrics) || it != o.it;
+ return _it != o._it || std::addressof(_metrics) != std::addressof(o._metrics);
}
private:
- void
- next()
- {
- auto [blob, offset] = metrics._splitID(it);
-
- if (++offset == METRICS_MAX_SIZE) {
- ++blob;
- offset = 0;
- }
+ void next();
- it = _makeId(blob, offset);
- }
-
- const Metrics &metrics;
- IdType it;
+ const Metrics &_metrics;
+ IdType _it;
};
iterator
@@ -215,6 +188,7 @@ public:
int16_t blob = _cur_blob;
int16_t offset = _cur_off;
_mutex.unlock();
+
return iterator(*this, _makeId(blob, offset));
}
@@ -231,6 +205,12 @@ private:
return (blob << 16 | offset);
}
+ static constexpr IdType
+ _makeId(std::tuple<uint16_t, uint16_t> id)
+ {
+ return _makeId(std::get<0>(id), std::get<1>(id));
+ }
+
void _addBlob();
mutable std::mutex _mutex;
diff --git a/src/api/Metrics.cc b/src/api/Metrics.cc
index 0303433b0a..f45d7a36d3 100644
--- a/src/api/Metrics.cc
+++ b/src/api/Metrics.cc
@@ -58,7 +58,7 @@ Metrics::newMetric(std::string_view name)
return it->second;
}
- Metrics::IdType id = (_cur_blob << 16 | _cur_off);
+ Metrics::IdType id = _makeId(_cur_blob, _cur_off);
Metrics::MetricStorage *blob = _blobs[_cur_blob];
Metrics::NameContainer &names = std::get<0>(*blob);
Metrics::AtomicContainer &atomics = std::get<1>(*blob);
@@ -88,7 +88,7 @@ Metrics::lookup(const std::string_view name) const
}
Metrics::AtomicType *
-Metrics::lookup(IdType id) const
+Metrics::lookup(IdType id, std::string_view *name) const
{
auto [blob_ix, offset] = _splitID(id);
Metrics::MetricStorage *blob = _blobs[blob_ix];
@@ -99,11 +99,15 @@ Metrics::lookup(IdType id) const
offset = 0;
}
+ if (name) {
+ *name = std::get<0>(std::get<0>(*blob)[offset]);
+ }
+
return &((std::get<1>(*blob)[offset]));
}
std::string_view
-Metrics::get_name(Metrics::IdType id) const
+Metrics::name(Metrics::IdType id) const
{
auto [blob_ix, offset] = _splitID(id);
Metrics::MetricStorage *blob = _blobs[blob_ix];
@@ -115,21 +119,22 @@ Metrics::get_name(Metrics::IdType id) const
}
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.
+// Iterator implementation
void
-Metrics::recordsDump(RecDumpEntryCb callback, void *edata) const
+Metrics::iterator::next()
{
- RecData datum;
+ auto [blob, offset] = _metrics._splitID(_it);
- for (auto [name, metric] : *this) {
- datum.rec_int = metric;
- callback(RECT_PLUGIN, edata, 1, name.data(), TS_RECORDDATATYPE_INT, &datum);
+ if (++offset == METRICS_MAX_SIZE) {
+ ++blob;
+ offset = 0;
}
+
+ _it = _makeId(blob, offset);
}
} // namespace ts
diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc
index 6dad8eb79e..358d0b90eb 100644
--- a/src/records/RecCore.cc
+++ b/src/records/RecCore.cc
@@ -1058,7 +1058,12 @@ RecDumpRecords(RecT rec_type, RecDumpEntryCb callback, void *edata)
}
// Also dump the new ts::Metrics if asked for
if (rec_type & TS_RECORDTYPE_PLUGIN) {
- ts::Metrics::getInstance().recordsDump(callback, edata);
+ RecData datum;
+
+ for (auto &&[name, val] : ts::Metrics::getInstance()) {
+ datum.rec_int = val;
+ callback(RECT_PLUGIN, edata, 1, name.data(), TS_RECORDDATATYPE_INT, &datum);
+ }
}
}
diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc
index b1c7805240..b574fd5c36 100644
--- a/src/traffic_server/InkAPI.cc
+++ b/src/traffic_server/InkAPI.cc
@@ -7520,14 +7520,14 @@ TSMgmtInt
TSStatIntGet(int id)
{
sdk_assert(sdk_sanity_check_stat_id(id) == TS_SUCCESS);
- return global_api_metrics.get(id);
+ return global_api_metrics[id].load();
}
void
TSStatIntSet(int id, TSMgmtInt value)
{
sdk_assert(sdk_sanity_check_stat_id(id) == TS_SUCCESS);
- global_api_metrics.set(id, value);
+ global_api_metrics[id].store(value);
}
TSReturnCode