KAFKA-16731: Added share group metrics class. #16488

smjn · 2024-06-28T15:41:34Z

What

Added ShareGroupMetrics inner class to SharePartitionManager.
Added code to record metrics at various checkpoints in code.

Why

We want to record the shareAcknowledgement rate and count, partition load time max and average and record ack rate and count.

Testing

Updated tests to verify partition load metrics, record ack and share ack requests

AndrewJSchofield

Thanks for the PR. Just a couple of minor comments.

AndrewJSchofield · 2024-06-28T15:49:35Z

core/src/test/java/kafka/server/share/SharePartitionManagerTest.java

@@ -96,6 +101,7 @@
 import static org.mockito.Mockito.when;

 @Timeout(120)
+@SuppressWarnings({ "ClassDataAbstractionCoupling"})


nit: Superfluous space

AndrewJSchofield · 2024-06-28T15:52:37Z

core/src/main/java/kafka/server/share/SharePartitionManager.java

+            partitionLoadTimeSensor.add(metrics.metricName(
+                    PARTITION_LOAD_TIME_AVG,
+                    METRICS_GROUP_NAME,
+                    "Average time taken to load the share partitions."),


"The average time in milliseconds to load the share partitions".

AndrewJSchofield · 2024-06-28T15:53:01Z

core/src/main/java/kafka/server/share/SharePartitionManager.java

+            partitionLoadTimeSensor.add(metrics.metricName(
+                    PARTITION_LOAD_TIME_MAX,
+                    METRICS_GROUP_NAME,
+                    "Maximum time taken to load the share partitions."),


"The maximum time in milliseconds to load the share partitions."

apoorvmittal10

I ll take a look at it, early next week, just need to re-check the metrics definition once again around offset and records commit rate.

AndrewJSchofield

Thanks for the PR. A few minor comments.

AndrewJSchofield · 2024-07-05T08:42:17Z

core/src/main/java/kafka/server/share/SharePartitionManager.java

+        void recordAcknowledgement(byte ackType) {
+            if (recordAcksSensorMap.containsKey(ackType)) {
+                recordAcksSensorMap.get(ackType).record();
+            } else {


The ackType can be zero when a gap is being acknowledged. For example, when a batch of transactional records is received by the client, it responds with 0 for the offsets which correspond to the transactional control records. This will cause an error to be logged by the broker for each of the records.

@apoorvmittal10 I've seen this line being logged during integration tests. How do you think we should handle this? Either we need to ensure the metrics are only logged for ack types the metrics understands, or we need to permit the metrics to just ignore types which are not understood (which is my preferred option).

Sure, as discussed let's ignore the unknown ones (gaps).

AndrewJSchofield · 2024-07-05T08:43:34Z

core/src/main/java/kafka/server/share/SharePartitionManager.java

+                metrics.metricName(
+                    SHARE_ACK_RATE,
+                    METRICS_GROUP_NAME,
+                    "The rate of number of acknowledge requests."),


"Rate of number" sounds odd. "Rate of acknowledge requests" or "acknowledge request rate" would be better.

Probably "Rate of acknowledge requests" is best given my next comment.

AndrewJSchofield · 2024-07-05T08:44:01Z

core/src/main/java/kafka/server/share/SharePartitionManager.java

+                        metrics.metricName(
+                            RECORD_ACK_RATE,
+                            METRICS_GROUP_NAME,
+                            "The rate of number of records acknowledged per acknowledgement type.",


"Rate of number" sounds odd. "Rate of records acknowledged per acknowledgement type" sounds better I think.

apoorvmittal10

Thanks for the PR, LGTM!

AndrewJSchofield

Thanks for the PR. lgtm

AndrewJSchofield · 2024-07-05T10:36:01Z

core/src/main/java/kafka/server/share/SharePartitionManager.java

@@ -808,8 +808,6 @@ void shareAcknowledgement() {
        void recordAcknowledgement(byte ackType) {
            if (recordAcksSensorMap.containsKey(ackType)) {


I'd add a comment above the if test that says something like // unknown ack types (such as gaps for control records) are intentionally ignored

omkreddy

@smjn Thanks for the PR. LGTM

Added ShareGroupMetrics inner class to SharePartitionManager. Added code to record metrics at various checkpoints in code. Reviewers: Andrew Schofield <[email protected]>,Apoorv Mittal <[email protected]>, Manikumar Reddy <[email protected]>

AndrewJSchofield suggested changes Jun 28, 2024

View reviewed changes

apoorvmittal10 reviewed Jun 28, 2024

View reviewed changes

smjn requested a review from AndrewJSchofield June 28, 2024 16:26

smjn added 3 commits July 5, 2024 13:46

KAFKA-16731: Added share group metrics class.

c3bed75

Incorporated review comments.

512a51e

Incorporated comments.

98aa98d

smjn force-pushed the KAFKA-16731 branch from f14c580 to 98aa98d Compare July 5, 2024 08:18

smjn requested a review from apoorvmittal10 July 5, 2024 08:19

AndrewJSchofield suggested changes Jul 5, 2024

View reviewed changes

Reworded metric description.

7f73f85

smjn requested a review from AndrewJSchofield July 5, 2024 09:14

Removed log for unknown record type.

82597fb

apoorvmittal10 approved these changes Jul 5, 2024

View reviewed changes

AndrewJSchofield approved these changes Jul 5, 2024

View reviewed changes

Added comment for unknown ack types.

0ab7845

omkreddy approved these changes Jul 7, 2024

View reviewed changes

omkreddy merged commit 932759b into apache:trunk Jul 7, 2024
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-16731: Added share group metrics class. #16488

KAFKA-16731: Added share group metrics class. #16488

smjn commented Jun 28, 2024 •

edited

Loading

AndrewJSchofield left a comment

AndrewJSchofield Jun 28, 2024

AndrewJSchofield Jun 28, 2024

AndrewJSchofield Jun 28, 2024

apoorvmittal10 left a comment

AndrewJSchofield left a comment

AndrewJSchofield Jul 5, 2024

AndrewJSchofield Jul 5, 2024

apoorvmittal10 Jul 5, 2024

AndrewJSchofield Jul 5, 2024

AndrewJSchofield Jul 5, 2024

AndrewJSchofield Jul 5, 2024

apoorvmittal10 left a comment

AndrewJSchofield left a comment

AndrewJSchofield Jul 5, 2024

omkreddy left a comment

		@@ -808,8 +808,6 @@ void shareAcknowledgement() {
		void recordAcknowledgement(byte ackType) {
		if (recordAcksSensorMap.containsKey(ackType)) {

KAFKA-16731: Added share group metrics class. #16488

KAFKA-16731: Added share group metrics class. #16488

Conversation

smjn commented Jun 28, 2024 • edited Loading

What

Why

Testing

AndrewJSchofield 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

apoorvmittal10 left a comment

Choose a reason for hiding this comment

AndrewJSchofield 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

apoorvmittal10 left a comment

Choose a reason for hiding this comment

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omkreddy left a comment

Choose a reason for hiding this comment

smjn commented Jun 28, 2024 •

edited

Loading