Refactor new metrics with Counter vs Gauge types by zwoop · Pull Request #10694 · apache/trafficserver

zwoop · 2023-10-30T19:42:27Z

This adds some more tooling around Counter vs Gauge type of metrics, assuring that Counters for example can only increment. There's no change in functionality here, other than the naming and creation details. But it will assure that the underlying APIs are used correctly.

Performance wise this should make no difference, at least in a release build. The compiler will optimize this down to just the fetch_add() just as before.

ywkaras · 2023-10-31T15:00:06Z

Maybe you can fix the Clang Analyzer defect by inserting ink_assert(d != nullptr); after line 1269.

zwoop · 2023-10-31T16:46:27Z

Maybe you can fix the Clang Analyzer defect by inserting ink_assert(d != nullptr); after line 1269.

I could, but the thing is, this is not my fault :). I asked @masaori335 to look at this, I'm suspecting that something in my PR makes clang-analyzer either confused, or brilliant, in detecting this old issue.

ywkaras · 2023-10-31T16:53:16Z

Clang Analyzer punishes for unconfessed sins and those from previous lifetimes.

zwoop · 2023-10-31T22:12:48Z

Clang Analyzer punishes for unconfessed sins and those from previous lifetimes.

True dat.

ywkaras · 2023-11-02T15:11:41Z

+
+    class AtomicType : public Metrics::AtomicType
+    {
+    };


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

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.

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

Ok, let me look tomorrow morning.

ywkaras · 2023-11-02T15:58:28Z

+
+  public:
+    AtomicType()          = default;
+    virtual ~AtomicType() = default;


Making this virtual doubles the size of instances of this type. Why is it necessary?

Same as above :/

above what?

It had to be virtual, otherwise I couldn't subclass it later as I did. And I had to sub class it, rather than aliases, to make the compiler detect bad usage. It's not considered polymorphic otherwise, e.g.

/Users/leif/apache/trafficserver/src/proxy/http/PreWarmManager.cc:1181:18: error: 'ts::Metrics::AtomicType' is not polymorphic metric = dynamic_cast<Metrics::Counter::AtomicType *>(metrics.lookup(stats_id));

Maybe we can fix the usage that's causing trouble here, I didn't write this code in the PreWarmManager.

Maybe the right solution here is to have

Metrics::Counter::lookup(stats_id); Metrics::Gauge::lookup(stats_id);

?

@ywkaras I added those static's, which allows us to eliminate the virtual. Let me know what you think, it's in the last commit that I lovingly made just for you.

ywkaras · 2023-11-02T16:11:04Z

+    {
+      auto &instance = Metrics::instance();
+
+      return reinterpret_cast<AtomicType *>(instance.lookup(instance._create(name)));


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

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.

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

ywkaras · 2023-11-02T17:26:21Z

+    }
+
+    static int64_t
+    load(AtomicType *metric)


This can't be const?

Maybe? I can try.

Ok, fixed, and uploaded with a new commit for your review suggestions (will add more as if we identify more things that needs fixing).

ywkaras · 2023-11-02T17:28:24Z

+      return metric->_value.load();
+    }
+
+  }; // class Counter


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

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

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

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.

ywkaras · 2023-11-02T17:43:07Z

@@ -103,16 +125,9 @@ class Metrics
    return std::nullopt;
  }


This function is not currently used.

It's used in the tests :). But it could be used, maybe even should be used. Certainly in plugins.

ywkaras · 2023-11-02T17:55:27Z

  increment_current_active_connections_stat() override
  {
-    Metrics::increment(http2_rsb.current_active_server_connection_count);
+    Metrics::Gauge::increment(http2_rsb.current_active_server_connection_count);


Since you had to change all of these anyway, why didn't you make increment and decrement non-static member functions:
http2_rsb.current_active_server_connection_count->increment(). It's shorter, and you would get a compiler error if you tried to decrement a counter rather than a gauge.

I thought of that, and even asked @cmcfarlen , we both preferred the static. I can change it if we think it's better. It just reads better with the static IMO.

ywkaras · 2023-11-07T01:18:22Z

+    {
+      auto &instance = Metrics::instance();
+
+      return reinterpret_cast<AtomicType *>(instance.lookup(instance._create(name)));


Another superfluous cast.

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

/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)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

ywkaras · 2023-11-07T01:24:51Z

-  Metrics::IntType *completed = m.lookup(m.lookup("proxy.process.io_uring.completed"));
-  Metrics::IntType *submitted = m.lookup(m.lookup("proxy.process.io_uring.submitted"));
+  Metrics::Counter::AtomicType *completed = m.lookup(m.lookup("proxy.process.io_uring.completed"));
+  Metrics::Counter::AtomicType *submitted = m.lookup(m.lookup("proxy.process.io_uring.submitted"));


You could have had two more useless casts here.

You implying we should have another lookup function, that avoids both lookups here/ This is an unusual use case honestly, the the IdType lookups are kept mostly because of the old APIs and use of "ids" instead of pointers.

We can probably eliminate some of this in v11 once we clean out more old stuff completely. Or, if we could convince Strostrup to allow overloading to take the return type into the resolution.

ywkaras · 2023-11-07T01:35:10Z


-  printf("submissions: %lu\n", Metrics::read(submitted));
-  printf("completions: %lu\n", Metrics::read(completed));
+  printf("submissions: %lu\n", Metrics::Gauge::load(submitted));


Really %lu quit slacking https://en.cppreference.com/w/cpp/types/integer

Yeh, not my code. I'm fairly certain we've agreed that you don't have to rewrite everyones code to make something "better".

ywkaras · 2023-11-07T03:52:05Z

+    }
+
+    static AtomicType *
+    createPtr(const std::string_view name)


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

does it hurt? Other than your eyes ?

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

ywkaras · 2023-11-07T03:53:50Z

-  this->rsb.refcountcache_last_sync_time       = intm.newMetricPtr((metrics_prefix + "last_sync.time").c_str());
-  this->rsb.refcountcache_last_total_items     = intm.newMetricPtr((metrics_prefix + "last_sync.total_items").c_str());
-  this->rsb.refcountcache_last_total_size      = intm.newMetricPtr((metrics_prefix + "last_sync.total_size").c_str());
+  this->rsb.refcountcache_current_items        = Metrics::Gauge::createPtr((metrics_prefix + "current_items").c_str());


Why are you doing string -> c string -> string_view instead of just string -> string_view directly?

Probably a remnant from the code as it was prior to changing to new metrics.

ywkaras · 2023-11-07T16:35:44Z

How about: https://godbolt.org/z/7Wv444EfE

bneradt · 2023-11-07T22:31:32Z

[approve ci]

bneradt · 2023-11-07T23:27:59Z

[approve ci]

masaori335 · 2023-11-08T02:23:07Z

-        std::string_view alpn_name = alpn_name_for_stat(dst->alpn_index);
+      _makeName(dst, COUNTER_STAT_ENTRIES[j], name, sizeof(name));
+
+      auto metric = Metrics::Counter::createPtr(name); // This will do a lookup if it already exists


This looks much convenient 👍

masaori335

This makes metrics type explicit. Looks good to me.

zwoop added TS API Metrics labels Oct 30, 2023

zwoop added this to the 10.0.0 milestone Oct 30, 2023

zwoop requested review from bneradt and bryancall October 30, 2023 19:42

zwoop self-assigned this Oct 30, 2023

zwoop force-pushed the RefactorNewMetrics branch from ae5feaa to a502f4c Compare October 30, 2023 21:41

bryancall requested a review from ywkaras October 30, 2023 22:13

zwoop force-pushed the RefactorNewMetrics branch from a502f4c to eaa3ff6 Compare October 31, 2023 16:49

masaori335 mentioned this pull request Oct 31, 2023

Add assert of CacheDisk is not nullptr #10704

Merged

zwoop force-pushed the RefactorNewMetrics branch 2 times, most recently from 8fc2b43 to 7688644 Compare November 1, 2023 16:18

ywkaras reviewed Nov 2, 2023

View reviewed changes

ywkaras reviewed Nov 7, 2023

View reviewed changes

ywkaras previously approved these changes Nov 7, 2023

View reviewed changes

zwoop dismissed ywkaras’s stale review via acdb96c November 7, 2023 05:20

zwoop force-pushed the RefactorNewMetrics branch from afa6c52 to acdb96c Compare November 7, 2023 05:20

zwoop force-pushed the RefactorNewMetrics branch 4 times, most recently from 774727b to e287f76 Compare November 7, 2023 21:04

zwoop force-pushed the RefactorNewMetrics branch 4 times, most recently from e56c764 to 10ea86c Compare November 8, 2023 00:01

ywkaras previously approved these changes Nov 8, 2023

View reviewed changes

masaori335 reviewed Nov 8, 2023

View reviewed changes

masaori335 previously approved these changes Nov 8, 2023

View reviewed changes

zwoop added 6 commits November 8, 2023 10:09

Initial refactoring, cleanup

22229b2

Removes most of the instance use

10040e4

Separate the metrics storage from metrics classes

3ce5ba3

First phase of restricting the Counter class

41f2185

Switches to Gauge/Counter abstractions

d64a7e1

Fixes from Walt's review comments

8e6333d

zwoop dismissed stale reviews from masaori335 and ywkaras via 8e6333d November 8, 2023 17:20

zwoop force-pushed the RefactorNewMetrics branch from 10ea86c to 8e6333d Compare November 8, 2023 17:20

masaori335 approved these changes Nov 8, 2023

View reviewed changes

ywkaras approved these changes Nov 8, 2023

View reviewed changes

zwoop merged commit 954cd74 into apache:master Nov 9, 2023

zwoop deleted the RefactorNewMetrics branch November 9, 2023 16:01

Uh oh!

Conversation

zwoop commented Oct 30, 2023

Uh oh!

ywkaras commented Oct 31, 2023

Uh oh!

zwoop commented Oct 31, 2023

Uh oh!

ywkaras commented Oct 31, 2023

Uh oh!

zwoop commented Oct 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwoop Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwoop Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwoop Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zwoop Nov 6, 2023 •

edited

Loading

zwoop Nov 7, 2023 •

edited

Loading

zwoop Nov 7, 2023 •

edited

Loading