KAFKA-12469: Deprecated and corrected topic metrics for consumer (KIP-1109) #18232

apoorvmittal10 · 2024-12-17T15:17:13Z

The PR implements the behaviour defined in KIP-1109. It corrects the consumer topic and topic-partition metrics, while deprecating the incorrect ones.

https://cwiki.apache.org/confluence/display/KAFKA/KIP-1109%3A+Unifying+Kafka+Consumer+Topic+Metrics

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…-1109)

junrao

@apoorvmittal10 : Thanks for the PR. Left a few comments.

junrao · 2024-12-17T18:08:13Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManager.java

+                    // Remove deprecated metrics.
+                    metrics.removeSensor(deprecatedMetricName(partitionRecordsLagMetricName(tp)));
+                    metrics.removeSensor(deprecatedMetricName(partitionRecordsLeadMetricName(tp)));
+                    metrics.removeMetric(partitionPreferredReadReplicaMetricNameDeprecated(tp));


partitionPreferredReadReplicaMetricNameDeprecated => deprecatedPartitionPreferredReadReplicaMetricName to be consistent with other names?

junrao · 2024-12-17T18:57:36Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManagerTest.java

        metricsManager.recordBytesFetched(topicName2, 1);
+        // Another 8 metrics gets registered as deprecated metrics should be reported for topicName2.


gets => get

junrao · 2024-12-17T19:06:44Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManagerTest.java

+        // 5 new metrics shall be registered.
+        assertEquals(5, metrics.metrics().size() - additionalRegisteredMetricsSize);
+
+        // Remove 1 topic which has deprecated metrics as well.


1 topic => 1 partition

junrao · 2024-12-17T19:11:14Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManager.java

@@ -181,6 +195,67 @@ void maybeUpdateAssignment(SubscriptionState subscription) {
        }
    }

+    @Deprecated // To be removed in Kafka 5.0 release.
+    private void maybeRecordDeprecatedBytesFetched(String name, String topic, int bytes) {


It would be useful to follow the approach used in https://github.com/apache/kafka/pull/11302/files to add "deprecated" in the description of the metric name and update the ops doc about the deprecation.

As the metrics with a specfic tag has been deprectaed hence it's hard to specify same metric as deprecated. Hence I have added Note to consumer topic metrics. For consumer metrics ops need not to ne updated as ops.html uses metrics description to generate details on kafka-site, I have verified that the added Note can be viewed on kafka-site locally. Please let me know if you think it should be handled differently.

Generally speaking, we should always include a note explaining the deprecation. This should include the first version it was deprecated, when it's expected to be removed and what should be used instead.

Sure, so currently below Note has been added to metrics.

Current:

"Note: For topic names with periods (.), an additional metric with underscores is emitted. However, the periods replaced metric is deprecated. Please use the metric with actual topic name instead."

Example:

Should we change it to:

"Note: For topic names with periods (.), an additional metric with underscores is emitted. However, the periods replaced metric is deprecated since Kafka 4.1 and shall be removed in Kafka 5.0. Please use the metric with actual topic name instead."

wdyt?

chia7712

@apoorvmittal10 thanks for this patch!

chia7712 · 2024-12-18T09:52:09Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManager.java

-                .build();
+        maybeRecordDeprecatedPartitionLag(name, tp, lag);
+
+        Sensor recordsLag = new SensorBuilder(metrics, name, () -> Map.of("topic", tp.topic(), "partition", String.valueOf(tp.partition())))


nit: Could you please consider placing "topic" before "partition"? See the following screenshot:

That can be addressed by using Utils.mkMap

Make sense, I earlier considered it but didn't do as previous tags were not ordered. I have added the change.

AndrewJSchofield · 2024-12-18T15:20:35Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManagerTest.java

+        metricsManager.recordPartitionLag(tp3, 4);
+        metricsManager.recordPartitionLead(tp3, 2);
+
+        int additionalRegisteredMetricsSize = metrics.metrics().size();


Wouldn't it be sensible to assert the number of additional registered metrics too?

I haven't asserted them on purpose as above tests already validated them.

AndrewJSchofield · 2024-12-18T15:21:27Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManagerTest.java

+        metricsManager.maybeUpdateAssignment(subscriptionState);
+        // For tp2, 14 metrics will be unregistered. 3 for partition lag, 3 for partition lead, 1 for
+        // preferred read replica and similarly 7 deprecated metrics.
+        assertEquals(-9, metrics.metrics().size() - additionalRegisteredMetricsSize);


This -9 is needlessly obscure.

I have added a comment and toggled the check, please let me know if it's better now.

AndrewJSchofield · 2024-12-18T15:26:50Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManagerTest.java


 import static org.apache.kafka.clients.consumer.internals.FetchMetricsManager.topicPartitionTags;
 import static org.apache.kafka.clients.consumer.internals.FetchMetricsManager.topicTags;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;

 public class FetchMetricsManagerTest {


Personally, I would put @SuppressWarnings("deprecation") on this class because it is knowingly using deprecated methods.

I have a suspicion you put the suppression on FetchMetricsManager when I meant to put it on this test.

I have corrected it, also I have placed the suppression on individual tests so they can be removed while removing deprecated code.

junrao

@apoorvmittal10 : Thanks for the updated PR. One more comment. Also, is the test failure related to this PR?

junrao · 2024-12-19T19:10:33Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManager.java

-                .build();
+        maybeRecordDeprecatedPartitionLag(name, tp, lag);
+
+        Sensor recordsLag = new SensorBuilder(metrics, name, () -> mkMap(mkEntry("topic", tp.topic()), mkEntry("partition", String.valueOf(tp.partition()))))


Could we do the same for the deprecated metrics too for better consistency?

Is there a reason why you're still using the Utils.mkMap methods instead of Map.of from Java 9+?

@ijuma please see my previous comment #18232 (comment)

Hmm, if we want insertion ordering to be preserved, we should change SensorBuilder to use a LinkedHashMap (SequencedMap would be better, but that requires Java 21). Having this implicit requirement and hoping people get it right is very error prone.

Yeah agree a explicit requirement to send an ordered map would be better in builder itself.

I have opened https://issues.apache.org/jira/browse/KAFKA-18390 to address @ijuma's comment. The fix is targeted for version 4.1.0, as it may introduce significant changes to the codebase.

apoorvmittal10 · 2024-12-20T11:46:52Z

Also, is the test failure related to this PR?

It seems to be build scan publish failure. I ll keep an eye on current ongoing build to see if it passes.

junrao

@apoorvmittal10 : Thanks for the updated PR. LGTM. I will wait to see if other reviewers have more comments.

chia7712

LGTM

chia7712 · 2024-12-20T18:42:26Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManager.java

+        }
+    }
+
+    @Deprecated // To be removed in Kafka 5.0 release.


Could you please open the jira to ensure we will remove them from 5.0 :)

Done: https://issues.apache.org/jira/browse/KAFKA-18335

apoorvmittal10 · 2024-12-23T11:45:45Z

@AndrewJSchofield Please let us know if it's good to merge?

AndrewJSchofield

Just one final comment to do with suppression of deprecation warnings. Apart from that, this looks good to me.

AndrewJSchofield · 2024-12-27T10:43:51Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManager.java

 /**
 * The {@link FetchMetricsManager} class provides wrapper methods to record lag, lead, latency, and fetch metrics.
 * It keeps an internal ID of the assigned set of partitions which is updated to ensure the set of metrics it
 * records matches up with the topic-partitions in use.
 */
+@SuppressWarnings("deprecation")


I don't believe this suppression has any value because it's implementing deprecated code, as opposed to using it.

Yes you are right, I have corrected it.

AndrewJSchofield · 2024-12-27T10:44:45Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManagerTest.java


 import static org.apache.kafka.clients.consumer.internals.FetchMetricsManager.topicPartitionTags;
 import static org.apache.kafka.clients.consumer.internals.FetchMetricsManager.topicTags;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;

 public class FetchMetricsManagerTest {


I have a suspicion you put the suppression on FetchMetricsManager when I meant to put it on this test.

ijuma · 2025-01-01T16:09:46Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchMetricsManager.java

+        return this.metrics.metricInstance(metricsRegistry.partitionPreferredReadReplica, metricTags);
+    }
+
+    @Deprecated


We should never deprecate something without including a @deprecated javadoc with the details I outlined in a separate comment.

Sure, I have added details here #18232 (comment), please let me know your thoughts.

…-1109) (apache#18232) The PR implements the behaviour defined in KIP-1109. It corrects the consumer topic and topic-partition metrics, while deprecating the incorrect ones. Reviewers: Chia-Ping Tsai <[email protected]>, Jun Rao <[email protected]>, Andrew Schofield <[email protected]>

KAFKA-12469: Deprecated and corrected topic metrics for consumer (KIP…

edc209e

…-1109)

github-actions bot added triage PRs from the community consumer clients labels Dec 17, 2024

apoorvmittal10 requested review from junrao, chia7712 and AndrewJSchofield December 17, 2024 15:17

junrao reviewed Dec 17, 2024

View reviewed changes

github-actions bot removed the triage PRs from the community label Dec 17, 2024

chia7712 reviewed Dec 18, 2024

View reviewed changes

AndrewJSchofield requested changes Dec 18, 2024

View reviewed changes

apoorvmittal10 added 2 commits December 19, 2024 16:05

Addressing review comments

b42ffdf

Moving to ordered map for tags

5332c5c

apoorvmittal10 requested review from chia7712, junrao and AndrewJSchofield December 19, 2024 16:48

junrao reviewed Dec 19, 2024

View reviewed changes

Addressing review comments

8cea00e

apoorvmittal10 requested a review from junrao December 20, 2024 11:47

junrao approved these changes Dec 20, 2024

View reviewed changes

chia7712 approved these changes Dec 20, 2024

View reviewed changes

AndrewJSchofield requested changes Dec 27, 2024

View reviewed changes

apoorvmittal10 added 2 commits December 30, 2024 11:40

Correcting deprecation

05f30bc

Merge remote-tracking branch 'upstream/trunk' into KAFKA-12469

3fcb1f5

apoorvmittal10 requested a review from AndrewJSchofield December 30, 2024 11:42

empty for CI

ae9fa4d

AndrewJSchofield approved these changes Dec 31, 2024

View reviewed changes

AndrewJSchofield merged commit f88cf57 into apache:trunk Dec 31, 2024
8 of 9 checks passed

ijuma reviewed Jan 1, 2025

View reviewed changes

		metricsManager.recordBytesFetched(topicName2, 1);
		// Another 8 metrics gets registered as deprecated metrics should be reported for topicName2.

KAFKA-12469: Deprecated and corrected topic metrics for consumer (KIP-1109) #18232

KAFKA-12469: Deprecated and corrected topic metrics for consumer (KIP-1109) #18232

Conversation

apoorvmittal10 commented Dec 17, 2024

Committer Checklist (excluded from commit message)

junrao 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

apoorvmittal10 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorvmittal10 Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Current:

Should we change it to:

chia7712 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

Choose a reason for hiding this comment

junrao 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

ijuma Jan 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorvmittal10 commented Dec 20, 2024

junrao left a comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorvmittal10 commented Dec 23, 2024

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

apoorvmittal10 Dec 19, 2024 •

edited

Loading

apoorvmittal10 Jan 2, 2025 •

edited

Loading

ijuma Jan 1, 2025 •

edited

Loading