Track internode msg bytes #1081

aymkhalil · 2024-04-12T19:32:53Z

Addresses: https://github.com/riptano/cndb/issues/9194

Tracking internode message bytes.

More generic implementation (i.e. leveraging InboundMessageHandlers like InternodeInboundMetrics have the following complications:

The work if we want to track internode messages globally. However per keyspace and table dimensions would require access the the deserialzed message and inverting the payload at runtime.
The life cycle of RequestSensors today (from sensor registration to synchronizing the sensors registry) begins on VerbHandler#doVerb. Trying to register and synchronise outside the verb handler boundaries to increment internode sensors would require major changes (probably where the verbs are dispatched in InboundSink)

aymkhalil · 2024-04-17T23:22:17Z

Update internode sensors tracking to happen at the outboundSink level
Encoded KEYSPACE header to simplify encoding internode sensors in the message header. Please note that it is not possible to link the message to a keyspace in the Mutation case because the payload is empty. Even for reads, the message doesn't naturally hold keyspace info in the LocalDataResponse and even if it does, extracting will require inspecting the payload at runtime. I found the header approach to be simpler.
Internode sensors are now tracking at the keyspace level only, consequently, there are only tracking in the SensorsRegistry
Added SensorsInternodeMessageTest unit tests

919d1cf

sbtourist · 2024-05-13T08:07:40Z

src/java/org/apache/cassandra/net/SensorsCustomParams.java

@@ -28,6 +29,12 @@
 */
 public final class SensorsCustomParams
 {
+    /**


This looks a bit unclean, what about passing the keyspace via the existing executor locals mechanism? I.e. we could add a RequestTracker method to register/retrieve keyspaces involved in the current request, wdyt?

Makes sense. AFAIK RequestTracker would map 1:1 with the keyspace so will add it to the constructor.

sbtourist · 2024-05-13T08:14:19Z

src/java/org/apache/cassandra/sensors/Context.java

+     */
+    public Context(String keyspace)
+    {
+        this(keyspace, null, null);


Apologies for not being great at remembering things (one more reason I guess to recap our discussions on github), but can you remind me why don't we also track table information?

Batch writes (one idea to mitigate this is to divide bytes and counts equally between tables)

The life cycle of RequestSensors today (from sensor registration to synchronizing the sensors registry) begins on VerbHandler#doVerb and ends on the callback. To track per table, two complexities arise:

There will be need to synchronize sensors values outside the verb handler boundary

We embed sensor value in the response message, that would require somehow injecting the sensor values into the message at the OutboundSink level which is not trivial.

I built on the idea you proposed of adding keyspace to RequestTracker to add all tables too in order to track table info. The caveat is:

We don't track internode messages per table per request bc the message size is only known after it's been built

The track per table across requests using SensorsRegistry - the very first message belong to an table will not contain internode sensors.

Let me know caa569f

I had a quick look and I'm not too convinced, it seems internode tracking adds quite a bit of confusion :/ Let me think if we can make it a bit better and more similar to the other sensors...

sbtourist · 2024-05-13T12:24:01Z

src/java/org/apache/cassandra/sensors/SensorsRegistry.java

+    /**
+     * Tracks outbound messages size and count in Sensors Regsitry
+     */
+    private boolean trackOutboundMessages(Message<?> message, InetAddressAndPort ignored)


Adding internode tracking responsibility to the registry doesn't look right to me: what about moving this code to MessagingService#listen() and rewrite it slightly to use the "standard" sensors API? It would probably be slightly more verbose, but overall cleaner IMO. WDYT?

It is cleaner but requires a SensorsRegistry#syncAllSensors if we were to use the standard API. If there is need, we can expose an async variant or a public api to sync a single sensor directly.

aymkhalil · 2024-05-13T19:34:30Z

Fixed PR diff cc52860

aymkhalil · 2024-05-13T23:56:39Z

@sbtourist PR is ready for second round of review. PTAL.

sbtourist · 2024-05-14T17:19:15Z

src/java/org/apache/cassandra/net/MessagingService.java

@@ -620,10 +625,32 @@ boolean isConnected(InetAddressAndPort address, Message<?> messageOut)
    public void listen()
    {
        inboundSockets.open();
+        outboundSink.add(this::trackOutboundMessages);


Maybe a nit, but this would be best invoked first, so as soon as we start listening it's all setup.

sbtourist · 2024-05-14T18:32:52Z

src/java/org/apache/cassandra/sensors/Context.java

+     */
+    public Context(String keyspace)
+    {
+        this(keyspace, null, null);


I had a quick look and I'm not too convinced, it seems internode tracking adds quite a bit of confusion :/ Let me think if we can make it a bit better and more similar to the other sensors...

* Opinionated changes to internode tracking: * Removed message count as bytes should really be the most important/accurate. * Tracked both inbound and outbound bytes, as writes take up inbound bytes, while reads take up outbound bytes. * Added internode sensors to custom params in the same place where we add the other sensors. * Added internode sensors for request. * Added SensorsInternodeTest + bug fixes. * * Use only the payload size as internode bytes. * Cache the payload size where possible.

sbtourist · 2024-05-28T09:00:52Z

src/java/org/apache/cassandra/db/CounterMutationVerbHandler.java

+        tables.forEach(tm -> {
+            Context context = Context.from(tm);
+            requestSensors.registerSensor(context, Type.INTERNODE_BYTES);
+            requestSensors.incrementSensor(context, Type.INTERNODE_BYTES, message.serializedSize(MessagingService.current_version) / tables.size());


message.serializedSize

I realized for inbound messages we use the whole message serialized size, should we use the payload size for consistency?

+1 8543504

* Collect counter mutation WRITE_BYTES from replicas * Rely on replica count to accommodate for mutation replica sensor values * Empty-Commit * Track internode msg bytes and counts * Track internode sensors at the outbound sink level * Fix NPE for ks only sensors * Fix more NPEs in unit tests * Rebase on top of origin/cc-ucu * Remove ks from headers & track internode messages in MessagingService * Add internode sensors per ks and table * Opinionated changes to internode tracking (#1121) * Opinionated changes to internode tracking: * Removed message count as bytes should really be the most important/accurate. * Tracked both inbound and outbound bytes, as writes take up inbound bytes, while reads take up outbound bytes. * Added internode sensors to custom params in the same place where we add the other sensors. * Added internode sensors for request. * Added SensorsInternodeTest + bug fixes. * * Use only the payload size as internode bytes. * Cache the payload size where possible. * Use payload size for inbound messages --------- Co-authored-by: Sergio Bossa <sergio.bossa@gmail.com>

aymkhalil requested a review from sbtourist April 12, 2024 19:33

aymkhalil marked this pull request as ready for review April 17, 2024 23:22

Base automatically changed from cc-ucu-counter to cc-ucu May 10, 2024 23:15

sbtourist reviewed May 13, 2024

View reviewed changes

aymkhalil force-pushed the cc-ucu-intenode branch from a5a34c1 to fa835cc Compare May 13, 2024 19:12

aymkhalil added 7 commits May 13, 2024 12:15

Collect counter mutation WRITE_BYTES from replicas

620c8ad

Rely on replica count to accommodate for mutation replica sensor values

cc348c0

Empty-Commit

d719a2b

Track internode msg bytes and counts

1d57505

Track internode sensors at the outbound sink level

4b1f0ce

Fix NPE for ks only sensors

c70e27f

Fix more NPEs in unit tests

014041a

aymkhalil force-pushed the cc-ucu-intenode branch from fa835cc to 9936c02 Compare May 13, 2024 19:31

Rebase on top of origin/cc-ucu

cc52860

aymkhalil force-pushed the cc-ucu-intenode branch from 9936c02 to cc52860 Compare May 13, 2024 19:33

Remove ks from headers & track internode messages in MessagingService

36499a2

aymkhalil force-pushed the cc-ucu-intenode branch 3 times, most recently from e59190b to 442dbff Compare May 13, 2024 23:44

Add internode sensors per ks and table

caa569f

aymkhalil force-pushed the cc-ucu-intenode branch from 442dbff to caa569f Compare May 13, 2024 23:50

sbtourist reviewed May 14, 2024

View reviewed changes

aymkhalil mentioned this pull request May 23, 2024

CNDB-9193 Track sensor bytes for LWT #1074

Merged

aymkhalil changed the title ~~Track internode msg bytes and counts~~ Track internode msg bytes May 24, 2024

Merge branch 'cc-ucu' into cc-ucu-intenode

c17a353

sbtourist reviewed May 28, 2024

View reviewed changes

Use payload size for inbound messages

8543504

sbtourist approved these changes May 30, 2024

View reviewed changes

aymkhalil merged commit ad66f41 into cc-ucu May 30, 2024
433 of 444 checks passed

aymkhalil deleted the cc-ucu-intenode branch May 30, 2024 17:05

aymkhalil restored the cc-ucu-intenode branch May 30, 2024 17:09

aymkhalil deleted the cc-ucu-intenode branch May 30, 2024 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track internode msg bytes #1081

Track internode msg bytes #1081

aymkhalil commented Apr 12, 2024 •

edited

Loading

aymkhalil commented Apr 17, 2024

sbtourist May 13, 2024

aymkhalil May 13, 2024

sbtourist May 13, 2024

aymkhalil May 13, 2024

aymkhalil May 13, 2024

sbtourist May 14, 2024

sbtourist May 13, 2024

aymkhalil May 13, 2024

aymkhalil commented May 13, 2024

aymkhalil commented May 13, 2024

sbtourist May 14, 2024

sbtourist May 14, 2024

sbtourist May 28, 2024

aymkhalil May 28, 2024

Track internode msg bytes #1081

Track internode msg bytes #1081

Conversation

aymkhalil commented Apr 12, 2024 • edited Loading

aymkhalil commented Apr 17, 2024

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

aymkhalil commented May 13, 2024

aymkhalil commented May 13, 2024

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

aymkhalil commented Apr 12, 2024 •

edited

Loading