CASSANDRA-17225 Add batch_metrics virtual table #1372

burmanm · 2021-12-23T19:23:29Z

Allows to retrieve unlogged,, logged and counter batch metrics from a VT.

I'm here for the (rare, collectible) t-shirt of course ;)

… logged and counter batch metrics

blerer

The patch looks good :-) I only have 2 suggestions.

blerer · 2022-01-11T17:02:00Z

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

+            if(r.getString("name").equals("partitions_per_logged_batch")) {
+                snapshot = metrics.partitionsPerLoggedBatch.getSnapshot();
+            } else if(r.getString("name").equals("partitions_per_unlogged_batch")) {
+                snapshot = metrics.partitionsPerUnloggedBatch.getSnapshot();
+            } else {
+                snapshot = metrics.partitionsPerCounterBatch.getSnapshot();
+            }


nit: It might make the code easier to read if we extract the logic in a separate method like:

private Histogram getExpectedHistogram(BatchMetrics metrics, String name) { if ("partitions_per_logged_batch".equals(name)) return metrics.partitionsPerLoggedBatch; if ("partitions_per_unlogged_batch".equals(name)) return metrics.partitionsPerUnloggedBatch; return metrics.partitionsPerCounterBatch; }

+1 on a new method. And I wanted to mention that the project follows some conventions you might want to get acquainted with - https://cassandra.apache.org/_/development/code_style.html (I noticed { are not on new lines)

I'm too used to go fmt doing everything automatically on save, I'll fix the indentation (I wonder if IntelliJ has something similar when saving to auto-reformat code..)

blerer · 2022-01-11T17:04:23Z

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

+            } else {
+                snapshot = metrics.partitionsPerCounterBatch.getSnapshot();
+            }
+            assertEquals(r.getDouble("p50"), snapshot.getMedian(), 1.0);


nit: I would switch the arguments as the expected value is the snapshot value and the actual value is what is returned by the resultset.

Yes, good catch (wonders of different test-libraries having these in different orders)

ekaterinadimitrova2 · 2022-01-13T23:03:54Z

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

+            if(r.getString("name").equals("partitions_per_logged_batch")) {
+                snapshot = metrics.partitionsPerLoggedBatch.getSnapshot();
+            } else if(r.getString("name").equals("partitions_per_unlogged_batch")) {
+                snapshot = metrics.partitionsPerUnloggedBatch.getSnapshot();
+            } else {
+                snapshot = metrics.partitionsPerCounterBatch.getSnapshot();
+            }


+1 on a new method. And I wanted to mention that the project follows some conventions you might want to get acquainted with - https://cassandra.apache.org/_/development/code_style.html (I noticed { are not on new lines)

ekaterinadimitrova2 · 2022-01-13T23:04:43Z

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

+    }
+
+    @Test
+    void testSelectAll() throws Throwable


I suspect you accidentally deleted the public accessor, otherwise you can't run the test. I noticed It throws a class cast exception though. Can you, please check it? Thanks. Please let me know if you need help

ekaterinadimitrova2 · 2022-01-13T23:16:51Z

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.codahale.metrics.Histogram;


Suggested change

import com.codahale.metrics.Histogram;

unused import

…shot in the test to be a separate function, fix indentation and MAX() uses LongType instead of DoubleType

ekaterinadimitrova2 · 2022-01-14T17:40:19Z

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

+        {
+            return metrics.partitionsPerLoggedBatch.getSnapshot();
+        }
+        else if ("partitions_per_unlogged_batch".equals(tableName))


I would probably skip the else :-) Just it reads better something like that:

if ("partitions_per_logged_batch".equals(tableName)) return metrics.partitionsPerLoggedBatch.getSnapshot(); if ("partitions_per_unlogged_batch".equals(tableName)) return metrics.partitionsPerUnloggedBatch.getSnapshot(); return metrics.partitionsPerCounterBatch.getSnapshot();

I see both approaches used in the code base, I'm personally on the "else" camp usually - to me it makes it easier to follow the flow, with the added bonus of being able to hide the whole conditional flow in the editor with a single click (does not matter in this case, but in general).

I also prefer avoiding any unecessary code as it can only create confusion and for simple condition braces are normally skipped.

ekaterinadimitrova2

One formatting comment, otherwise the latest changes LGTM. +1, we need to add NEWS.txt and CHANGES.txt, this can be done also on commit IMHO

blerer

The patch looks good to me

blerer · 2022-01-17T14:57:55Z

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

+        {
+            return metrics.partitionsPerLoggedBatch.getSnapshot();
+        }
+        else if ("partitions_per_unlogged_batch".equals(tableName))


I also prefer avoiding any unecessary code as it can only create confusion and for simple condition braces are normally skipped.

ekaterinadimitrova2 · 2022-01-24T22:42:26Z

Are we ready to wrap up this one? I can run CI when done

ekaterinadimitrova2 · 2022-01-26T14:52:36Z

In the meantime the docs migration was also finished. So we need to consider also updating the Virtual Tables page

azotcsit

@burmanm
In general the code looks good. I added a few minor comments. Please, take a look. Feel free to ask if there is any confusion! And thanks for your contribution! 😄

azotcsit · 2022-01-29T21:14:04Z

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

+        result.forEach(r -> {
+            Snapshot snapshot = getExpectedHistogramSnapshot(metrics, r.getString("name"));
+            assertEquals(snapshot.getMedian(), r.getDouble("p50th"), 0.0);
+            assertEquals(snapshot.get99thPercentile(), r.getDouble("p99th"), 0.0);


Shouldn't we have p999th and max here tested as well?

azotcsit · 2022-01-29T21:39:29Z

src/java/org/apache/cassandra/db/virtual/BatchMetricsTable.java

+    private static final String PARTITIONS_PER_COUNTER_BATCH = "partitions_per_counter_batch";
+    private final static String P50 = "p50th";
+    private final static String P99 = "p99th";
+    private final static String P999 = "p999th";


The ticket description mentions TableMetricTables as a reference. p999 is not used there. Do we really want to expose it?

@burmanm @ekaterinadimitrova2 WDYT?

In my opinion TableMetricTables was mentioned to give an idea where to look in order to understand the requirement. In that sense I think p999 is fine here

test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.java

azotcsit · 2022-01-29T21:55:33Z

src/java/org/apache/cassandra/db/virtual/BatchMetricsTable.java

+import org.apache.cassandra.metrics.BatchMetrics;
+import org.apache.cassandra.schema.TableMetadata;
+
+public class BatchMetricsTable extends AbstractVirtualTable


It would be nice to mention this new table in the documentation: https://github.com/apache/cassandra/blob/trunk/doc/modules/cassandra/pages/new/virtualtables.adoc

azotcsit · 2022-01-29T22:01:02Z

src/java/org/apache/cassandra/db/virtual/BatchMetricsTable.java

+    BatchMetricsTable(String keyspace)
+    {
+        super(TableMetadata.builder(keyspace, "batch_metrics")
+                           .kind(TableMetadata.Kind.VIRTUAL)


Let's add a table description. Example: https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/virtual/RolesCacheKeysTable.java#L33

Smth like: "metrics on batch statements"

blerer · 2022-02-10T08:57:15Z

Patch merged manually.

CASSANDRA-17225 Add batch_metrics virtual table to retrieve unlogged,…

de91431

… logged and counter batch metrics

blerer reviewed Jan 11, 2022

View reviewed changes

ekaterinadimitrova2 reviewed Jan 13, 2022

View reviewed changes

Modify test to write directly to BatchMetrics, separate HistogramSnap…

c53b87a

…shot in the test to be a separate function, fix indentation and MAX() uses LongType instead of DoubleType

ekaterinadimitrova2 reviewed Jan 14, 2022

View reviewed changes

ekaterinadimitrova2 approved these changes Jan 14, 2022

View reviewed changes

blerer reviewed Jan 17, 2022

View reviewed changes

yifan-c mentioned this pull request Jan 26, 2022

CASSANDRA-17224: Virtual table for exposing CQL metrics #1421

Closed

azotcsit reviewed Jan 29, 2022

View reviewed changes

blerer closed this Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSANDRA-17225 Add batch_metrics virtual table #1372

CASSANDRA-17225 Add batch_metrics virtual table #1372

burmanm commented Dec 23, 2021

blerer left a comment

blerer Jan 11, 2022

ekaterinadimitrova2 Jan 13, 2022

burmanm Jan 14, 2022

blerer Jan 11, 2022

burmanm Jan 14, 2022

ekaterinadimitrova2 Jan 13, 2022

ekaterinadimitrova2 Jan 13, 2022

ekaterinadimitrova2 Jan 13, 2022

ekaterinadimitrova2 Jan 13, 2022

ekaterinadimitrova2 Jan 14, 2022 •

edited

burmanm Jan 14, 2022

blerer Jan 17, 2022

ekaterinadimitrova2 left a comment

blerer left a comment

blerer Jan 17, 2022

ekaterinadimitrova2 commented Jan 24, 2022

ekaterinadimitrova2 commented Jan 26, 2022

azotcsit left a comment

azotcsit Jan 29, 2022

azotcsit Jan 29, 2022

ekaterinadimitrova2 Jan 31, 2022

azotcsit Jan 29, 2022

azotcsit Jan 29, 2022

blerer commented Feb 10, 2022

CASSANDRA-17225 Add batch_metrics virtual table #1372

CASSANDRA-17225 Add batch_metrics virtual table #1372

Conversation

burmanm commented Dec 23, 2021

blerer 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

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

ekaterinadimitrova2 Jan 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

blerer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekaterinadimitrova2 commented Jan 24, 2022

ekaterinadimitrova2 commented Jan 26, 2022

azotcsit 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

Choose a reason for hiding this comment

blerer commented Feb 10, 2022

ekaterinadimitrova2 Jan 14, 2022 •

edited