Skip to content

Refactor new metrics with Counter vs Gauge types #10694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zwoop merged 6 commits into from
Nov 9, 2023
Merged

Refactor new metrics with Counter vs Gauge types #10694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
22229b2
Initial refactoring, cleanup
zwoop Oct 27, 2023
10040e4
Removes most of the instance use
zwoop Oct 27, 2023
3ce5ba3
Separate the metrics storage from metrics classes
zwoop Oct 27, 2023
41f2185
First phase of restricting the Counter class
zwoop Oct 27, 2023
d64a7e1
Switches to Gauge/Counter abstractions
zwoop Oct 30, 2023
8e6333d
Fixes from Walt's review comments
zwoop Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/developer-guide/api/functions/TSStat.en.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ A plugin can create statistics (metrics) that are accessible in the same way as
statistics. In general monitoring the behavior of plugins in production is easier to do in this way
in contrast to processing log files.

.. note::

These APIs are deprecated as of ATS v10.0.0, and will likely be removed in v11. Instead,
use the new APIs in `Metrics.h` diretly.

Synopsis
========

Expand Down
289 changes: 216 additions & 73 deletions include/api/Metrics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#pragma once

#include <array>
#include <optional>
#include <unordered_map>
#include <tuple>
#include <mutex>
Expand All @@ -45,23 +44,46 @@ class Metrics
private:
using self_type = Metrics;

class AtomicType
{
friend class Metrics;

public:
AtomicType() = default;

int64_t
load() const
{
return _value.load();
}

// ToDo: This is a little sketchy, but needed for the old InkAPI metrics.
void
store(int64_t val)
{
_value.store(val);
}

protected:
std::atomic<int64_t> _value{0};
};

public:
using IntType = std::atomic<int64_t>;
using IdType = int32_t; // Could be a tuple, but one way or another, they have to be combined to an int32_t.
using SpanIntType = swoc::MemSpan<IntType>;
using IdType = int32_t; // Could be a tuple, but one way or another, they have to be combined to an int32_t.
using SpanType = swoc::MemSpan<AtomicType>;

static constexpr uint16_t METRICS_MAX_BLOBS = 8192;
static constexpr uint16_t METRICS_MAX_SIZE = 2048; // For a total of 16M metrics
static constexpr IdType NOT_FOUND = std::numeric_limits<IdType>::min(); // <16-bit,16-bit> = <blob-index,offset>
static const auto MEMORY_ORDER = std::memory_order_relaxed;
static constexpr uint16_t MAX_BLOBS = 8192;
static constexpr uint16_t MAX_SIZE = 1024; // For a total of 8M metrics
static constexpr IdType NOT_FOUND = std::numeric_limits<IdType>::min(); // <16-bit,16-bit> = <blob-index,offset>
static const auto MEMORY_ORDER = std::memory_order_relaxed;

private:
using NameAndId = std::tuple<std::string, IdType>;
using NameContainer = std::array<NameAndId, METRICS_MAX_SIZE>;
using AtomicContainer = std::array<IntType, METRICS_MAX_SIZE>;
using MetricStorage = std::tuple<NameContainer, AtomicContainer>;
using MetricBlobs = std::array<MetricStorage *, METRICS_MAX_BLOBS>;
using LookupTable = std::unordered_map<std::string_view, IdType>;
using NameStorage = std::array<NameAndId, MAX_SIZE>;
using AtomicStorage = std::array<AtomicType, MAX_SIZE>;
using NamesAndAtomics = std::tuple<NameStorage, AtomicStorage>;
using BlobStorage = std::array<NamesAndAtomics *, MAX_BLOBS>;

public:
Metrics(const self_type &) = delete;
Expand All @@ -78,41 +100,22 @@ class Metrics

Metrics()
{
_blobs[0] = new MetricStorage();
_blobs[0] = new NamesAndAtomics();
ink_release_assert(_blobs[0]);
ink_release_assert(0 == newMetric("proxy.process.api.metrics.bad_id")); // Reserve slot 0 for errors, this should always be 0
ink_release_assert(0 == _create("proxy.process.api.metrics.bad_id")); // Reserve slot 0 for errors, this should always be 0
}

// Singleton
static Metrics &getInstance();
// The singleton instance, owned by the Metrics class
static Metrics &instance();

// 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);
SpanIntType newMetricSpan(size_t size, IdType *id = nullptr);
IdType lookup(const std::string_view name) const;
IntType *lookup(IdType id, std::string_view *name = nullptr) const;

std::optional<IntType *>
lookupPtr(const std::string_view name) const
{
IdType id = lookup(name);
if (id != NOT_FOUND) {
return lookup(id);
}
return std::nullopt;
}

// A bit of a convenience, since we use the ptr to the atomic frequently in the core
IntType *
newMetricPtr(const std::string_view name)
{
return lookup(newMetric(name));
}

AtomicType *lookup(const std::string_view name, IdType *out_id) const;
AtomicType *lookup(IdType id, std::string_view *out_name = nullptr) const;
bool rename(IdType id, const std::string_view name);

IntType &
AtomicType &
operator[](IdType id)
{
return *lookup(id);
Expand All @@ -129,16 +132,15 @@ class Metrics
{
auto metric = lookup(id);

return (metric ? metric->fetch_add(val, MEMORY_ORDER) : NOT_FOUND);
return (metric ? metric->_value.fetch_add(val, MEMORY_ORDER) : NOT_FOUND);
}

// ToDo: Do we even need these inc/dec functions?
int64_t
decrement(IdType id, uint64_t val = 1)
{
auto metric = lookup(id);

return (metric ? metric->fetch_sub(val, MEMORY_ORDER) : NOT_FOUND);
return (metric ? metric->_value.fetch_sub(val, MEMORY_ORDER) : NOT_FOUND);
}

std::string_view name(IdType id) const;
Expand All @@ -148,38 +150,10 @@ class Metrics
{
auto [blob, entry] = _splitID(id);

return (id >= 0 && ((blob < _cur_blob && entry < METRICS_MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off)));
return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off)));
}

// Static methods to encapsulate access to the atomic's
static void
increment(IntType *metric, uint64_t val = 1)
{
ink_assert(metric);
metric->fetch_add(val, MEMORY_ORDER);
}

static void
decrement(IntType *metric, uint64_t val = 1)
{
ink_assert(metric);
metric->fetch_sub(val, MEMORY_ORDER);
}

static int64_t
read(IntType *metric)
{
ink_assert(metric);
return metric->load();
}

static void
write(IntType *metric, int64_t val)
{
ink_assert(metric);
return metric->store(val);
}

class iterator
{
public:
Expand Down Expand Up @@ -215,7 +189,7 @@ class Metrics
std::string_view name;
auto metric = _metrics.lookup(_it, &name);

return std::make_tuple(name, metric->load());
return std::make_tuple(name, metric->_value.load());
}

bool
Expand All @@ -234,7 +208,7 @@ class Metrics
void next();

const Metrics &_metrics;
IdType _it;
Metrics::IdType _it;
};

iterator
Expand Down Expand Up @@ -267,6 +241,11 @@ class Metrics
}

private:
// These are private, to assure that we don't use them by accident creating naked metrics
IdType _create(const std::string_view name);
SpanType _createSpan(size_t size, IdType *id = nullptr);

// These are little helpers around managing the ID's
static constexpr std::tuple<uint16_t, uint16_t>
_splitID(IdType value)
{
Expand All @@ -289,10 +268,174 @@ class Metrics

mutable std::mutex _mutex;
LookupTable _lookups;
MetricBlobs _blobs;
BlobStorage _blobs;
uint16_t _cur_blob = 0;
uint16_t _cur_off = 0;

public:
// These are sort of factory classes, using the Metrics singleton for all storage etc.
class Gauge
{
public:
using self_type = Gauge;
using SpanType = Metrics::SpanType;

class AtomicType : public Metrics::AtomicType
{
};
Copy link
Contributor

@ywkaras ywkaras Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just using AtomicType = Metrics::AtomicType; ?

Copy link
Contributor Author

@zwoop zwoop Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because then the compiler won't give errors or warnings about wrong types. Initially I did this all with "using", and then it became too easy to do the wrong things with the wrong types of metrics. Meaning, if you do

using Foo == Bar;

Foo and Bar are identical types, and can be used interchangeable. They truly are aliases. And I wanted to avoid people trying to do dangerous stuff such as sucking out a Counter out of the metrics storage, and then treat it as a Gauge.

Copy link
Contributor

@ywkaras ywkaras Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you give it the constructor explicit AtomicType(Metrics::AtomicType a) : Metrics::AtomicType(a) {} so you won't need the reinterpret_cast.

Copy link
Contributor Author

@zwoop zwoop Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me look tomorrow morning.


static IdType
lookup(const std::string_view name)
{
auto &instance = Metrics::instance();

return instance.lookup(name);
}

static AtomicType *
lookup(const IdType id, std::string_view *out_name = nullptr)
{
auto &instance = Metrics::instance();

return reinterpret_cast<AtomicType *>(instance.lookup(id, out_name));
}

static AtomicType *
lookup(const std::string_view name, IdType *id)
{
auto &instance = Metrics::instance();

return reinterpret_cast<AtomicType *>(instance.lookup(name, id));
}

static Metrics::IdType
create(const std::string_view name)
{
auto &instance = Metrics::instance();

return instance._create(name);
}

static AtomicType *
createPtr(const std::string_view name)
Copy link
Contributor

@ywkaras ywkaras Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const std::string_view is overkill, all the member functions are const I think.

Copy link
Contributor Author

@zwoop zwoop Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it hurt? Other than your eyes ?

Copy link
Contributor

@ywkaras ywkaras Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alan will sentence you to hard labor on his pumpkin farm for this.

{
auto &instance = Metrics::instance();

return reinterpret_cast<AtomicType *>(instance.lookup(instance._create(name)));
Copy link
Contributor

@ywkaras ywkaras Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the overload of lookup() that returns IdType/uint32_t. How can that work, to cast it to a64-bit pointer?

Copy link
Contributor Author

@zwoop zwoop Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a different lookup no? I'd have to look again, but remember there are two lookups, one to that returns the IdType, and one that returns a pointer directly to the atomic types.

Copy link
Contributor

@ywkaras ywkaras Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see but it shouldn't need the case then.

}

static Metrics::Gauge::SpanType
createSpan(size_t size, IdType *id = nullptr)
{
auto &instance = Metrics::instance();

return instance._createSpan(size, id);
}

static void
increment(AtomicType *metric, uint64_t val = 1)
{
ink_assert(metric);
metric->_value.fetch_add(val, MEMORY_ORDER);
}

static void
decrement(AtomicType *metric, uint64_t val = 1)
{
ink_assert(metric);
metric->_value.fetch_sub(val, MEMORY_ORDER);
}

static int64_t
load(const AtomicType *metric)
{
ink_assert(metric);
return metric->_value.load();
}

static void
store(AtomicType *metric, int64_t val)
{
ink_assert(metric);
return metric->_value.store(val);
}

}; // class Gauge

class Counter
{
public:
using self_type = Counter;
using SpanType = Metrics::SpanType;

class AtomicType : public Metrics::AtomicType
{
};

static IdType
lookup(const std::string_view name)
{
auto &instance = Metrics::instance();

return instance.lookup(name);
}

static AtomicType *
lookup(const IdType id, std::string_view *out_name = nullptr)
{
auto &instance = Metrics::instance();

return reinterpret_cast<AtomicType *>(instance.lookup(id, out_name));
}

static AtomicType *
lookup(const std::string_view name, IdType *id)
{
auto &instance = Metrics::instance();

return reinterpret_cast<AtomicType *>(instance.lookup(name, id));
}

static Metrics::IdType
create(const std::string_view name)
{
auto &instance = Metrics::instance();

return instance._create(name);
}

static AtomicType *
createPtr(const std::string_view name)
{
auto &instance = Metrics::instance();

return reinterpret_cast<AtomicType *>(instance.lookup(instance._create(name)));
Copy link
Contributor

@ywkaras ywkaras Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another superfluous cast.

Copy link
Contributor Author

@zwoop zwoop Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think it's superfluous, it won't compile otherwise I'm fairly certain.

Copy link
Contributor Author

@zwoop zwoop Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

/Users/leif/apache/trafficserver/include/api/Metrics.h:312:14: error: cannot initialize return object of type 'AtomicType *' (aka 'ts::Metrics::Gauge::AtomicType *') with an rvalue of type 'AtomicType *' (aka 'ts::Metrics::AtomicType *')
      return instance.lookup(instance._create(name));
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

}

static Metrics::Counter::SpanType
createSpan(size_t size, IdType *id = nullptr)
{
auto &instance = Metrics::instance();

return instance._createSpan(size, id);
}

static void
increment(AtomicType *metric, uint64_t val = 1)
{
ink_assert(metric);
metric->_value.fetch_add(val, MEMORY_ORDER);
}

static int64_t
load(const AtomicType *metric)
{
ink_assert(metric);
return metric->_value.load();
}

}; // class Counter
Copy link
Contributor

@ywkaras ywkaras Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected Counter to have load, increment and clear, and Gauge to have load, increment and decrement.

Copy link
Contributor Author

@zwoop zwoop Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear? Why clear? All counters starts at zero, and can always go up. You should not set it back to 0 IMO.

Copy link
Contributor

@ywkaras ywkaras Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there are pros and cons to having clear(). I was thinking we had set() to do a clear, so just have clear().

Copy link
Contributor Author

@zwoop zwoop Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'll probably argue that if you need to do clear(), you are using this wrong, and it should be a gauge. Counters are supposed to always increase, randomly setting it to 0 (or any value) is wrong, and why I even bothered adding this difference of Counters vs Gauges.


}; // class Metrics

} // namespace ts
Loading