Skip to content

Commit 10ea86c

Browse files
zwoopzwoop
committed
Fixes from Walt's review comments
1 parent 84e4892 commit 10ea86c

File tree

5 files changed

+95
-65
lines changed

5 files changed

+95
-65
lines changed

‎include/api/Metrics.h‎

Lines changed: 53 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
#pragma once
2525

2626
#include <array>
27-
#include <optional>
2827
#include <unordered_map>
2928
#include <tuple>
3029
#include <mutex>
@@ -50,8 +49,7 @@ class Metrics
5049
friend class Metrics;
5150

5251
public:
53-
AtomicType() = default;
54-
virtual ~AtomicType() = default;
52+
AtomicType() = default;
5553

5654
int64_t
5755
load() const
@@ -113,18 +111,8 @@ class Metrics
113111
// Yes, we don't return objects here, but rather ID's and atomic's directly. Treat
114112
// the std::atomic<int64_t> as the underlying class for a single metric, and be happy.
115113
IdType lookup(const std::string_view name) const;
116-
AtomicType *lookup(IdType id, std::string_view *name = nullptr) const;
117-
118-
std::optional<AtomicType *>
119-
lookupPtr(const std::string_view name) const
120-
{
121-
IdType id = lookup(name);
122-
if (id != NOT_FOUND) {
123-
return lookup(id);
124-
}
125-
return std::nullopt;
126-
}
127-
114+
AtomicType *lookup(const std::string_view name, IdType *out_id) const;
115+
AtomicType *lookup(IdType id, std::string_view *out_name = nullptr) const;
128116
bool rename(IdType id, const std::string_view name);
129117

130118
AtomicType &
@@ -296,6 +284,30 @@ class Metrics
296284
{
297285
};
298286

287+
static IdType
288+
lookup(const std::string_view name)
289+
{
290+
auto &instance = Metrics::instance();
291+
292+
return instance.lookup(name);
293+
}
294+
295+
static AtomicType *
296+
lookup(const IdType id, std::string_view *out_name = nullptr)
297+
{
298+
auto &instance = Metrics::instance();
299+
300+
return reinterpret_cast<AtomicType *>(instance.lookup(id, out_name));
301+
}
302+
303+
static AtomicType *
304+
lookup(const std::string_view name, IdType *id)
305+
{
306+
auto &instance = Metrics::instance();
307+
308+
return reinterpret_cast<AtomicType *>(instance.lookup(name, id));
309+
}
310+
299311
static Metrics::IdType
300312
create(const std::string_view name)
301313
{
@@ -335,7 +347,7 @@ class Metrics
335347
}
336348

337349
static int64_t
338-
load(AtomicType *metric)
350+
load(const AtomicType *metric)
339351
{
340352
ink_assert(metric);
341353
return metric->_value.load();
@@ -360,6 +372,30 @@ class Metrics
360372
{
361373
};
362374

375+
static IdType
376+
lookup(const std::string_view name)
377+
{
378+
auto &instance = Metrics::instance();
379+
380+
return instance.lookup(name);
381+
}
382+
383+
static AtomicType *
384+
lookup(const IdType id, std::string_view *out_name = nullptr)
385+
{
386+
auto &instance = Metrics::instance();
387+
388+
return reinterpret_cast<AtomicType *>(instance.lookup(id, out_name));
389+
}
390+
391+
static AtomicType *
392+
lookup(const std::string_view name, IdType *id)
393+
{
394+
auto &instance = Metrics::instance();
395+
396+
return reinterpret_cast<AtomicType *>(instance.lookup(name, id));
397+
}
398+
363399
static Metrics::IdType
364400
create(const std::string_view name)
365401
{
@@ -392,7 +428,7 @@ class Metrics
392428
}
393429

394430
static int64_t
395-
load(AtomicType *metric)
431+
load(const AtomicType *metric)
396432
{
397433
ink_assert(metric);
398434
return metric->_value.load();

‎src/api/Metrics.cc‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ Metrics::lookup(const std::string_view name) const
8585
}
8686

8787
Metrics::AtomicType *
88-
Metrics::lookup(Metrics::IdType id, std::string_view *name) const
88+
Metrics::lookup(Metrics::IdType id, std::string_view *out_name) const
8989
{
9090
auto [blob_ix, offset] = _splitID(id);
9191
Metrics::NamesAndAtomics *blob = _blobs[blob_ix];
@@ -96,13 +96,30 @@ Metrics::lookup(Metrics::IdType id, std::string_view *name) const
9696
offset = 0;
9797
}
9898

99-
if (name) {
100-
*name = std::get<0>(std::get<0>(*blob)[offset]);
99+
if (out_name) {
100+
*out_name = std::get<0>(std::get<0>(*blob)[offset]);
101101
}
102102

103103
return &((std::get<1>(*blob)[offset]));
104104
}
105105

106+
Metrics::AtomicType *
107+
Metrics::lookup(const std::string_view name, Metrics::IdType *out_id) const
108+
{
109+
Metrics::IdType id = lookup(name);
110+
Metrics::AtomicType *result = nullptr;
111+
112+
if (id != NOT_FOUND) {
113+
result = lookup(id);
114+
}
115+
116+
if (nullptr != out_id) {
117+
*out_id = id;
118+
}
119+
120+
return result;
121+
}
122+
106123
std::string_view
107124
Metrics::name(Metrics::IdType id) const
108125
{

‎src/api/unit_tests/test_Metrics.cc‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,12 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]")
9191

9292
SECTION("lookup")
9393
{
94-
auto nm = m.lookupPtr("notametric");
95-
REQUIRE(!nm);
96-
97-
auto mid = Metrics::Counter::create("ametric");
98-
auto fm = m.lookupPtr("ametric");
99-
REQUIRE(fm.has_value());
100-
REQUIRE(fm.value());
101-
REQUIRE(fm.value() == m.lookup(mid));
102-
REQUIRE(m.lookup("ametric") == mid);
94+
auto nm = m.lookup("notametric");
95+
REQUIRE(nm == ts::Metrics::NOT_FOUND);
96+
97+
auto mid = Metrics::Counter::create("ametric");
98+
auto fmid = m.lookup("ametric");
99+
100+
REQUIRE(mid == fmid);
103101
}
104102
}

‎src/iocore/aio/test_AIO.cc‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -221,10 +221,8 @@ dump_summary()
221221
printf("IO_URING results\n");
222222
printf("-----------------\n");
223223

224-
auto &m = Metrics::getInstance();
225-
226-
Metrics::Counter::AtomicType *completed = m.lookup(m.lookup("proxy.process.io_uring.completed"));
227-
Metrics::Counter::AtomicType *submitted = m.lookup(m.lookup("proxy.process.io_uring.submitted"));
224+
auto completed = Metrics::Counter::lookup("proxy.process.io_uring.completed", nullptr);
225+
auto completed = Metrics::Counter::lookup("proxy.process.io_uring.submitted", nullptr);
228226

229227
printf("submissions: %lu\n", Metrics::Gauge::load(submitted));
230228
printf("completions: %lu\n", Metrics::Gauge::load(completed));

‎src/proxy/http/PreWarmManager.cc‎

Lines changed: 13 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1152,8 +1152,7 @@ _makeName(const PreWarm::SPtrConstDst &dst, std::string_view statname, char *nam
11521152
void
11531153
PreWarmManager::_register_stats(const PreWarm::ParsedSNIConf &parsed_conf)
11541154
{
1155-
int stats_counter = 0;
1156-
ts::Metrics &metrics = ts::Metrics::instance();
1155+
int stats_counter = 0;
11571156

11581157
for (auto &entry : parsed_conf) {
11591158
const PreWarm::SPtrConstDst &dst = entry.first;
@@ -1166,24 +1165,15 @@ PreWarmManager::_register_stats(const PreWarm::ParsedSNIConf &parsed_conf)
11661165

11671166
_makeName(dst, COUNTER_STAT_ENTRIES[j], name, sizeof(name));
11681167

1169-
ts::Metrics::IdType stats_id = metrics.lookup(name);
1170-
Metrics::Counter::AtomicType *metric = nullptr;
1168+
auto metric = Metrics::Counter::createPtr(name); // This will do a lookup if it already exists
11711169

1172-
if (stats_id == ts::Metrics::NOT_FOUND) {
1173-
metric = Metrics::Counter::createPtr(name);
1174-
1175-
if (metric == nullptr) {
1176-
Error("couldn't register counter stat name=%s", name);
1177-
} else {
1178-
++stats_counter;
1179-
}
1170+
if (metric == nullptr) {
1171+
Error("couldn't register counter stat name=%s", name);
11801172
} else {
1181-
metric = dynamic_cast<Metrics::Counter::AtomicType *>(metrics.lookup(stats_id));
1173+
++stats_counter;
1174+
counters[j] = metric;
1175+
Debug("v_prewarm_init", "conter stat id=%d name=%s", Metrics::Counter::lookup(name), name);
11821176
}
1183-
1184-
counters[j] = metric;
1185-
1186-
Debug("v_prewarm_init", "conter stat id=%d name=%s", stats_id, name);
11871177
}
11881178

11891179
// Gauges next
@@ -1192,24 +1182,15 @@ PreWarmManager::_register_stats(const PreWarm::ParsedSNIConf &parsed_conf)
11921182

11931183
_makeName(dst, GAUGE_STAT_ENTRIES[j], name, sizeof(name));
11941184

1195-
ts::Metrics::IdType stats_id = metrics.lookup(name);
1196-
Metrics::Gauge::AtomicType *metric = nullptr;
1185+
auto metric = Metrics::Gauge::createPtr(name); // This will do a lookup if it already exists
11971186

1198-
if (stats_id == ts::Metrics::NOT_FOUND) {
1199-
metric = Metrics::Gauge::createPtr(name);
1200-
1201-
if (metric == nullptr) {
1202-
Error("couldn't register gauge stat name=%s", name);
1203-
} else {
1204-
++stats_counter;
1205-
}
1187+
if (metric == nullptr) {
1188+
Error("couldn't register gauge stat name=%s", name);
12061189
} else {
1207-
metric = dynamic_cast<Metrics::Gauge::AtomicType *>(metrics.lookup(stats_id));
1190+
++stats_counter;
1191+
gauges[j] = metric;
1192+
Debug("v_prewarm_init", "gauge stat id=%d name=%s", Metrics::Gauge::lookup(name), name);
12081193
}
1209-
1210-
gauges[j] = metric;
1211-
1212-
Debug("v_prewarm_init", "gauge stat id=%d name=%s", stats_id, name);
12131194
}
12141195

12151196
_stats_id_map[dst] = std::make_shared<const PreWarm::StatsIds>(ids);

0 commit comments

Comments
 (0)