Adding new metrics for SAI in nodetool tablestats command. #3742

sunil9977 · 2024-12-13T09:42:55Z

CASSANDRA-20026
Adding below metrics in nodetool tablestats command -

SAI local query latency (mean)
SAI post-filtering latency (mean)
SAI space used (bytes)
SAI SSTable indexes hit per query (mean)
SAI index segments hit per query (mean)
SAI rows filtered per query (mean)
SAI local total query timeouts
SAI queryable/total indexes

maedhroz · 2024-12-13T23:54:13Z

test/unit/org/apache/cassandra/tools/nodetool/stats/TableStatsPrinterTest.java

+        "\tSAI SSTable indexes hit per query (mean): 0.0\n" +
+        "\tSAI index segments hit per query (mean): 0.0\n" +
+        "\tSAI rows filtered per query (mean): 0.0\n" +
+        "\tSAI local total query timeouts: 0\n" +


nit: I guess we could just make this "local query timeouts" and leave out the "total". It's fairly obvious this is a count and not a rate...

(see also below if you do change it)

Corrected the name.

maedhroz · 2024-12-13T23:54:55Z

test/unit/org/apache/cassandra/tools/nodetool/stats/TableStatsPrinterTest.java

-                                 "        \"average_live_cells_per_slice_last_five_minutes\" : 5.0,\n" +
-                                 "        \"local_read_latency_ms\" : \"1.000\",\n" +
-                                 "        \"sstable_count\" : 1000,\n" +
+                                 "        \"sai_Index_Segments_Hit\" : 0.0,\n" +


nit: Any way we can keep things lower case for consistency?

maedhroz · 2024-12-14T00:00:35Z

test/unit/org/apache/cassandra/tools/nodetool/stats/TableStatsPrinterTest.java

+        "\tSAI index segments hit per query (mean): 0.0\n" +
+        "\tSAI rows filtered per query (mean): 0.0\n" +
+        "\tSAI local total query timeouts: 0\n" +
+        "\tSAI queryable/total indexes: null\n" +


We should have a few of these test tables present non-zero/null values. Here are some reasonable examples:

SAI local query latency (mean): 10.000 ms SAI post-filtering latency (mean): 1.000 ms SAI space used (bytes): 128000000 SAI SSTable indexes hit per query (mean): 3.5 SAI index segments hit per query (mean): 4.0 SAI rows filtered per query (mean): 55.0 SAI local total query timeouts: 4 SAI queryable/total indexes: 5/5

I added test table5 with SAI metrics.

maedhroz · 2024-12-16T19:19:59Z

test/unit/org/apache/cassandra/tools/nodetool/stats/TableStatsPrinterTest.java

+                                 "        \"sai_queryable_total_indexes\" : \"0/0\",\n" +
+                                 "        \"droppable_tombstone_ratio\" : \"0.66667\",\n" +
+                                 "        \"compression_metadata_off_heap_memory_used\" : \"1\",\n" +
+                                 "        \"sai_Rows_Filtered\" : \"NaN\",\n" +


nit: still mixed case?

maedhroz · 2024-12-16T19:27:34Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+                return "PerQuery";
+            case "PostFilteringReadLatency":
+            case "TotalQueryTimeouts":
+                return "TableQueryMetrics";


We already have TableQueryMetrics.TABLE_QUERY_METRIC_TYPE and TableStateMetrics.TABLE_STATE_METRIC_TYPE that we can reuse here, but I feel like it would be a good time to do something similar for "IndexGroupMetrics" (in IndexGroupMetrics) and "PerQuery" (in PerQueryMetrics).

I wouldn't say we have to extend this to the individual metric names, as we shouldn't be changing those anyway (with them being a user-facing thing).

maedhroz · 2024-12-16T19:27:48Z

src/java/org/apache/cassandra/tools/NodeProbe.java

@@ -1892,7 +1892,66 @@ public Object getThreadPoolMetric(String pathName, String poolName, String metri
      }
    }

-    /**
+    public Object getSAIIndexMetrics(String ks, String cf, String metricName) {


nit: { on newline

maedhroz · 2024-12-16T19:27:52Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+        }
+    }
+
+    private Object getMetricValue(String metricName, ObjectName oName) throws IOException {


nit: { on newline

maedhroz · 2024-12-16T19:27:58Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+        }
+    }
+
+    private String getScopeForMetric(String metricName) {


nit: { on newline

maedhroz · 2024-12-16T19:28:56Z

src/java/org/apache/cassandra/tools/nodetool/stats/StatsTable.java

+    public long TotalQueryTimeouts;
+    public int TotalIndexCount;
+    public int TotalQueryableIndexCount;
+    public String TotalQueryableIndexRatio;


nit/style: For the above, make sure to start field names w/ lower-case letter

...or we could just prefix them all with sai, which I was going to suggest anyway, given some of them are pretty general, like disk used bytes.

Added prefix as sai.

maedhroz · 2024-12-16T19:31:18Z

src/java/org/apache/cassandra/tools/nodetool/stats/StatsTableComparator.java

+        }
+        else if(sortKey.equals("disk_used_bytes"))
+        {
+            return sign * ((String) stx.DiskUsedBytes).compareTo((String) sty.DiskUsedBytes);


Should we use compareFileSizes() here?

maedhroz · 2024-12-16T19:32:23Z

src/java/org/apache/cassandra/tools/nodetool/stats/StatsTableComparator.java

+        {
+            result = compareDoubles(stx.PostFilteringReadLatencyms, sty.PostFilteringReadLatencyms);
+        }
+        else if(sortKey.equals("disk_used_bytes"))


Should this be "sai_disk_used_bytes"?

maedhroz · 2024-12-16T19:34:52Z

src/java/org/apache/cassandra/tools/nodetool/stats/StatsTableComparator.java

+        }
+        else if(sortKey.equals("TotalQueryableIndexCount"))
+        {
+            return sign * ((int) stx.TotalQueryableIndexCount);


For TotalIndexCount and TotalQueryableIndexCount, are we actually comparing anything here?

maedhroz · 2024-12-16T19:36:21Z

src/java/org/apache/cassandra/tools/nodetool/stats/TableStatsHolder.java

+        mpTable.put("sai_Rows_Filtered",table.RowsFiltered);
+        mpTable.put("sai_total_query_timeouts",table.TotalQueryTimeouts);
+        mpTable.put("total_index_count",table.TotalIndexCount);
+        mpTable.put("queryable_index_count",table.TotalQueryableIndexCount);


nit: We should probably prefix these two with sai_ to be safe.

maedhroz · 2024-12-16T19:40:58Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+        }
+    }
+
+    private Object getMetricValue(String metricName, ObjectName oName) throws IOException {


Suggested change

private Object getMetricValue(String metricName, ObjectName oName) throws IOException {

private Object getSaiMetricValue(String metricName, ObjectName oName) throws IOException {

maedhroz · 2024-12-16T19:42:05Z

src/java/org/apache/cassandra/tools/NodeProbe.java

@@ -1892,7 +1892,66 @@ public Object getThreadPoolMetric(String pathName, String poolName, String metri
      }
    }

-    /**
+    public Object getSAIIndexMetrics(String ks, String cf, String metricName) {


Suggested change

public Object getSAIIndexMetrics(String ks, String cf, String metricName) {

public Object getSaiMetric(String ks, String cf, String metricName) {

maedhroz · 2024-12-16T19:42:27Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+        }
+    }
+
+    private String getScopeForMetric(String metricName) {


Suggested change

private String getScopeForMetric(String metricName) {

private String getSaiMetricScope(String metricName) {

maedhroz · 2024-12-16T19:43:26Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+            Set<ObjectName> matchingMBeans = mbeanServerConn.queryNames(oName, null);
+            if (matchingMBeans.isEmpty()) {
+                return null;
+            }


nit: might as well just avoid the braces for this if

maedhroz · 2024-12-16T19:43:46Z

src/java/org/apache/cassandra/tools/NodeProbe.java

@@ -1892,7 +1892,66 @@ public Object getThreadPoolMetric(String pathName, String poolName, String metri
      }
    }

-    /**
+    public Object getSAIIndexMetrics(String ks, String cf, String metricName) {
+        try {


nit: { on newline

maedhroz · 2024-12-16T19:43:53Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+            }
+
+            return getMetricValue(metricName, oName);
+        } catch (MalformedObjectNameException e) {


nit: { on newline

maedhroz · 2024-12-16T19:43:58Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+    }
+
+    private Object getMetricValue(String metricName, ObjectName oName) throws IOException {
+        switch (metricName) {


nit: { on newline

maedhroz · 2024-12-16T19:44:06Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+    }
+
+    private String getScopeForMetric(String metricName) {
+        switch (metricName) {


nit: { on newline

maedhroz · 2024-12-16T19:49:08Z

One thing I think we want to double-check on is that we don't show these stats for system tables, as they shouldn't have SAI indexes.

…ast review.

maedhroz · 2024-12-18T20:44:56Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+                return null;
+
+            return getSaiMetricValue(metricName, oName);
+        } catch (MalformedObjectNameException e)


nit/formatting: For everything below, newlines after } and before the catch

maedhroz · 2024-12-18T20:46:00Z

src/java/org/apache/cassandra/tools/nodetool/stats/TableStatsPrinter.java

            out.println("");
        }

+        private boolean isSystemKeyspaces(String keyspaceName) {
+            return SchemaConstants.isSystemKeyspace(keyspaceName);
+


nit: unnecessary newline

maedhroz · 2024-12-18T20:46:09Z

src/java/org/apache/cassandra/tools/nodetool/stats/TableStatsPrinter.java

            out.println("");
        }

+        private boolean isSystemKeyspaces(String keyspaceName) {


nit: newline for {

maedhroz · 2024-12-18T20:48:09Z

src/java/org/apache/cassandra/tools/nodetool/stats/TableStatsHolder.java

+                Object totalQueryableIndexCount = probe.getSaiMetric(keyspaceName, tableName, "TotalQueryableIndexCount");
+                int saiTotalQueryableIndexCount = (totalQueryableIndexCount != null) ? (int) totalQueryableIndexCount : 0;
+
+                statsTable.saiTotalQueryableIndexRatio = String.format("%d/%d", saiTotalIndexCount, saiTotalQueryableIndexCount);


Could we avoid the entire block above if we're looking at a system keyspace? We've got a check at print-time in TableStatsPrinter, but it seems like we might as well avoid hitting the probe here if we're not going to print the info anyway?

…w nit comments.

maedhroz · 2024-12-20T16:21:57Z

src/java/org/apache/cassandra/tools/nodetool/stats/TableStatsHolder.java

                statsKeyspace.tables.add(statsTable);
            }
            keyspaces.add(statsKeyspace);
        }
    }

+    private boolean isSystemKeyspaces(String keyspaceName)
+    {
+        return SchemaConstants.isSystemKeyspace(keyspaceName);


nit: Here and in TableStatsPrinter we could probably just inline the SchemaConstants.isSystemKeyspace() call. It's only used once in both classes anyway...

Adding new metrics for SAI in nodetool tablestats command.

f5e31ee

maedhroz self-requested a review December 13, 2024 21:48

maedhroz reviewed Dec 13, 2024

View reviewed changes

maedhroz reviewed Dec 14, 2024

View reviewed changes

Adding test table5 with SAI metrics.

8a322bc

maedhroz reviewed Dec 16, 2024

View reviewed changes

Added check for system keyspaces, and implemented nit comments from l…

e12e844

…ast review.

maedhroz reviewed Dec 18, 2024

View reviewed changes

Added check for skipping system tables from prob itself along with fe…

c383226

…w nit comments.

maedhroz reviewed Dec 20, 2024

View reviewed changes

maedhroz approved these changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new metrics for SAI in nodetool tablestats command. #3742

Adding new metrics for SAI in nodetool tablestats command. #3742

sunil9977 commented Dec 13, 2024

maedhroz Dec 13, 2024

sunil9977 Dec 16, 2024

maedhroz Dec 13, 2024

maedhroz Dec 14, 2024

sunil9977 Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

sunil9977 Dec 18, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz Dec 16, 2024

maedhroz commented Dec 16, 2024

maedhroz Dec 18, 2024

maedhroz Dec 18, 2024

maedhroz Dec 18, 2024

maedhroz Dec 18, 2024

maedhroz Dec 20, 2024

	private Object getMetricValue(String metricName, ObjectName oName) throws IOException {
	private Object getSaiMetricValue(String metricName, ObjectName oName) throws IOException {

	public Object getSAIIndexMetrics(String ks, String cf, String metricName) {
	public Object getSaiMetric(String ks, String cf, String metricName) {

	private String getScopeForMetric(String metricName) {
	private String getSaiMetricScope(String metricName) {

Adding new metrics for SAI in nodetool tablestats command. #3742

Are you sure you want to change the base?

Adding new metrics for SAI in nodetool tablestats command. #3742

Conversation

sunil9977 commented Dec 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

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

maedhroz commented Dec 16, 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