feat(metrics): Add label for main and other listeners #4739

abhijat · 2025-03-10T10:59:09Z

The stats collected per connection are divided according to main or other listener.

Metrics are decorated with labels listener= main or other.

The behavior of INFO command is unchanged, it still displays the sum of connections and commands instead of separate main and other counts.

src/facade/dragonfly_connection.cc

The stats collected per connection are divided according to main or other listener. Metrics are decorated with labels listener= main or other. Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>

kostasrim · 2025-03-11T12:59:43Z

src/server/dragonfly_test.cc

+  EXPECT_EQ(metrics.facade_stats.conn_stats.num_conns_main, 0);
+  EXPECT_EQ(metrics.facade_stats.conn_stats.num_conns_other, 0);


This is zero because we don't use an actual connection right ?

yeah, in BaseFamilyTest::Run we seem to directly call DispatchCommand on the test connection wrapper, the HandleRequests method isn't called which would increment or decrement these.

kostasrim · 2025-03-11T13:01:52Z

tests/dragonfly/server_family_test.py

+@dfly_args({"memcached_port": 11211})
+async def test_metric_labels(
+    df_server: DflyInstance, async_client: aioredis.Redis, memcached_client: Client
+):


We do not need a test for the prometheous metrics but I guess it won't hurt ? You can just open your browser and check via http://localhost:6379/metrics or just rely on the unit test. Not strongly opinionated about this tbh

Also you don't need memcached_client or memcached_port

I used memcached client to get a count of other connection and command as I see it goes via a listener other than main. Do you recommend to remove this test?

it's actually an interesting point. We want to count the memcache listener as "main" as well. it is customer facing as well.

I added a new method which returns true if the role is MAIN or the protocol is MEMCACHED and adjusted the metrics to use this. The test is also changed to account for this and use the admin port for counting other metrics.

This will metric will be incremented for any memcached listener (or main role listener), I hope this is correct behavior.

kostasrim · 2025-03-11T13:03:33Z

src/server/server_family.cc

+  AppendMetricHeader("connected_clients", "", MetricType::GAUGE, &resp->body());
+  AppendMetricValue("connected_clients", conn_stats.num_conns_main, {"listener"}, {"main"},
+                    &resp->body());
+  AppendMetricValue("connected_clients", conn_stats.num_conns_other, {"listener"}, {"other"},
+                    &resp->body());


@adiholden Won't this break our dashboards ? Since now we need to use the label as well on our queries ?

Double checking here that this is ok for us.

double checked with @Pothulapati we should be ok with the label. It won't break our dashboards.

kostasrim · 2025-03-11T13:05:10Z

src/facade/dragonfly_connection.cc

+uint32_t& Connection::NumConns() {
+  return IsMain() ? stats_->num_conns_main : stats_->num_conns_other;
+}


I really do not like to return references especially when we only want to increment what is being referenced. I know that Stefan suggested this but I would also suggest a slightly change: Rename NumConns() to IncrNumConns() and do the increment here:

void Connection::IncrNumConnStats() { IsMain() ? ++stats_->blah blah.... }

kostasrim · 2025-03-11T13:06:25Z

src/facade/dragonfly_connection.cc

+  if (const Listener* ls = dynamic_cast<Listener*>(listener()); ls) {
+    is_main_ = ls->IsMainInterface();
+  }


I know that the static_cast you replaced looks weird and wrong but it isn't.

We do not have other Listener types so if you static_cast here is perfectly fine and the downcast is safe to do so at compile time.

When you use dynamic_cast you are actually doing a runtime cast which queries the RTTI information of the object. There is an associated overhead with this cast. It's needed when "you are not sure" about the object type. Here, in this context and for our data types, we only have Listener so the static_cast will always succeed and be correct.

IMO this is our bad design. C++ has solved this problem and it's called CRTP (curiously recurring template pattern) which is a way to express static polymoprphism in a safe way (avoiding the explicit static_cast we used above by delegating that to the base class via the compiler). I never had a change to refactor our class hierarchies and that's why we got static_cast here. It's not wrong but it could be expressed better. However, I don't have the time to push such a refactor (it's not hard to do so but it does require time because you need to substitute virtual functions with static casts done by the base class).

p.s. you probably have encounter CRTP in seastar. All of the WRITE/READ DMA functions in there should use CRTP internally to decouple interface/impl requirements

Do you recommend switching back to static_cast here to avoid paying the type query cost?

@abhijat we usually do not rely on rtti in our code. Matter of taste and we do not use it.

@romange I wrote a paragraph 🤣 But yeah rtti has a small overhead as well :)

moved back to static_cast

kostasrim · 2025-03-11T13:24:12Z

src/server/main_service.cc

@@ -1318,7 +1318,8 @@ bool Service::InvokeCmd(const CommandId* cid, CmdArgList tail_args, SinkReplyBui
    DispatchMonitor(cntx, cid, tail_args);
  }

-  ServerState::tlocal()->RecordCmd();
+  const bool is_main = cntx->conn() ? cntx->conn()->IsMain() : false;


Cn cntx->conn() be null here ? If so maybe we can store the IsMain in ConnectionContext instead ?

I saw this was null during a unit test which runs the populate debug command, because it called InvokeCmd with a locally created context which had owner set to nullptr https://github.com/dragonflydb/dragonfly/blob/main/src/server/debugcmd.cc#L196

yep this is a stub context and we have a bug. So, why don't we use the context to check if it's main or not ?

and since we ConnectionContext local_cntx{cntx, stub_tx.get()}; we will also copy the IsMain information and we will count the right thing. Otherwise we would count those commands as non main even if they actually are

I added a new field on ConnectionContext, it is false by default and copied from owner connection if the owner and owner's connection are valid.

Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>

kostasrim

minor comment LGTM

kostasrim · 2025-03-12T10:27:43Z

src/facade/dragonfly_connection.cc

+  if (is_main_) {
+    return true;
+  }
+
+  if (const Listener* lsnr = static_cast<Listener*>(listener()); lsnr) {
+    return lsnr->protocol() == Protocol::MEMCACHE;
+  }
+
+  return false;


Suggested change

if (is_main_) {

return true;

}

if (const Listener* lsnr = static_cast<Listener*>(listener()); lsnr) {

return lsnr->protocol() == Protocol::MEMCACHE;

}

return false;

const Listener* lsnr = static_cast<Listener*>(listener());

return is_main_ || lsnr->protocol() == Protocol::MEMCACHE;

and maybe rename because HasMainListener is kinda a lie if protocol is MEMCACHE. Maybe something like IsMainOrMemcached ?

yeah I'm not too happy with HasMainListener, will change this

The problem with accessing lsnr->protocol() without a null check is that I found it to be null during tests when we end up here with TestConnection, and it caused segfault. From what I could see the test connection doesn't have a listener pointer.

In that case return is_main_ || (lsnr && snr->protocol() == Protocol::MEMCACHE)

I made both the changes, I ordered it so the static_cast is only after checking for is_main_, but I can change it to make everything in one composite statement, in that case we always have to do the static cast.

Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>

kostasrim

LGTM

abhijat force-pushed the abhijat/feat/label-for-main-listener branch 4 times, most recently from 8139a83 to 6321b86 Compare March 10, 2025 15:20

BagritsevichStepan reviewed Mar 10, 2025

View reviewed changes

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

abhijat force-pushed the abhijat/feat/label-for-main-listener branch from 6321b86 to 7587220 Compare March 11, 2025 05:16

abhijat requested a review from BagritsevichStepan March 11, 2025 05:19

abhijat changed the title ~~[wip] feat(metrics): Add label for main and other listeners~~ feat(metrics): Add label for main and other listeners Mar 11, 2025

kostasrim requested review from kostasrim and removed request for BagritsevichStepan March 11, 2025 09:10

kostasrim reviewed Mar 11, 2025

View reviewed changes

abhijat added 4 commits March 12, 2025 10:16

switch back to static_cast

Verified

This commit was signed with the committer’s verified signature.

tshemsedinov Timur Shemsedinov

GPG key ID: 04D23537BDD66C59

Verified
Learn about vigilant mode

d0d495f

Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>

count memcached as main listener

Verified

This commit was signed with the committer’s verified signature.

tshemsedinov Timur Shemsedinov

GPG key ID: 04D23537BDD66C59

Verified
Learn about vigilant mode

a3d0e55

Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>

fix setter methods

Verified

This commit was signed with the committer’s verified signature.

tshemsedinov Timur Shemsedinov

GPG key ID: 04D23537BDD66C59

Verified
Learn about vigilant mode

99c85fc

Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>

add safety checks before accessing listner

Verified

This commit was signed with the committer’s verified signature.

tshemsedinov Timur Shemsedinov

GPG key ID: 04D23537BDD66C59

Verified
Learn about vigilant mode

42aa20b

Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>

abhijat requested a review from kostasrim March 12, 2025 08:23

kostasrim reviewed Mar 12, 2025

View reviewed changes

rename accessor

Verified

This commit was signed with the committer’s verified signature.

tshemsedinov Timur Shemsedinov

GPG key ID: 04D23537BDD66C59

Verified
Learn about vigilant mode

23680eb

Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>

abhijat requested a review from kostasrim March 12, 2025 10:58

kostasrim approved these changes Mar 12, 2025

View reviewed changes

abhijat merged commit ac33cd8 into main Mar 12, 2025
10 checks passed

abhijat deleted the abhijat/feat/label-for-main-listener branch March 12, 2025 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics): Add label for main and other listeners #4739

feat(metrics): Add label for main and other listeners #4739

abhijat commented Mar 10, 2025 •

edited

Loading

kostasrim Mar 11, 2025

abhijat Mar 12, 2025

kostasrim Mar 11, 2025

kostasrim Mar 11, 2025

abhijat Mar 11, 2025

romange Mar 11, 2025

abhijat Mar 12, 2025

kostasrim Mar 11, 2025

kostasrim Mar 11, 2025

kostasrim Mar 11, 2025

abhijat Mar 12, 2025

kostasrim Mar 11, 2025

abhijat Mar 11, 2025

romange Mar 11, 2025

kostasrim Mar 11, 2025

abhijat Mar 12, 2025

kostasrim Mar 11, 2025

abhijat Mar 11, 2025

kostasrim Mar 11, 2025

kostasrim Mar 11, 2025

abhijat Mar 12, 2025

kostasrim left a comment

kostasrim Mar 12, 2025

kostasrim Mar 12, 2025

abhijat Mar 12, 2025

abhijat Mar 12, 2025

kostasrim Mar 12, 2025 •

edited

Loading

abhijat Mar 12, 2025

kostasrim left a comment

		EXPECT_EQ(metrics.facade_stats.conn_stats.num_conns_main, 0);
		EXPECT_EQ(metrics.facade_stats.conn_stats.num_conns_other, 0);

feat(metrics): Add label for main and other listeners #4739

feat(metrics): Add label for main and other listeners #4739

Conversation

abhijat commented Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim left a comment

Choose a reason for hiding this comment

abhijat commented Mar 10, 2025 •

edited

Loading

kostasrim Mar 12, 2025 •

edited

Loading