KAFKA-16115: Adding missing heartbeat metrics by philipnee · Pull Request #15216 · apache/kafka

philipnee · 2024-01-17T18:47:15Z

Add a HeartbeatMetrics module to measure various metrics related to the heartbeat. Here is the highlight of the changes:

HeartMetrics encapsulate a heartbeat sensor
Add metrics suffix types to the AbstractConsumerMetrics so that it can be extended to different metric groups

Non-related refactor:

Rename the metric classes to "MetricManager" the reason being "Metrics" seems to relate to the Metric class however these managers are merely containers holding sensor references for recording purposes.
Created OffsetCommitMetricsManager so that we are more consistent with the metrics management
Extended KafkaConsumerMetrics to the AbstractConsumerMetrics so that the groupName is consistently created (without random concatenation)

Some follow ups:

Refactor commit sensor by introducing a CommitMetrics module so that we can keep most of the metrics in one place and let it be a bit more consistent stylistically
Possibly refactor fetch manager metrics

eee wip

clean up remove bad code

clean up test

clean up

eee wip

clean up remove bad code

clean up test

clean up

…kafka into ctr-heartbeat-metrics

philipnee · 2024-01-29T18:54:16Z

    private long pollStartMs;
    private long timeSinceLastPollMs;

    public KafkaConsumerMetrics(Metrics metrics, String metricGrpPrefix) {


i'm not even sure why we have metricGrpPrefix from the beginning - it is not specified by the user and it seems to always be the default.

philipnee · 2024-01-29T18:58:34Z

        return new ConsumerConfig(prop);
    }

-    private HeartbeatRequestManager createHeartbeatRequestManager() {


philipnee · 2024-01-29T19:03:07Z

Hi @lucasbru - This PR is ready for review. I did a bit of refactor in this PR so not everything here is in the scope. Let me know what do you think!

lucasbru · 2024-01-30T14:56:27Z

Thanks, @philipnee. I will not get to it today, but I will review it tomorrow.

lucasbru

@philipnee. Thanks for the PR, I left some comments

lucasbru · 2024-01-31T10:59:15Z

     * sending heartbeat until the next poll.
     */
    private final Timer pollTimer;
+    private final HeartbeatMetricsManager metricsManager;


nit: I'd either group this with the field below or separate this from pollTimer with a newline. Looks like the javadoc is referring to both fields like this.

lucasbru · 2024-01-31T11:06:17Z

    final PendingRequests pendingRequests;
    private boolean closing = false;
-    private Sensor commitSensor;
+    private OffsetCommitMetricsManager metricsManager;


lucasbru · 2024-01-31T11:09:15Z

        Map<TopicPartition, OffsetAndMetadata> offsets = new HashMap<>();
        offsets.put(new TopicPartition("t1", 0), new OffsetAndMetadata(0));
        commitRequestManger.addOffsetCommitRequest(offsets, Optional.empty(), false);
-        assertPoll(1, commitRequestManger);


Did you remove this on purpose?

lucasbru · 2024-01-31T11:17:51Z

+        assertHeartbeat(heartbeatRequestManager, DEFAULT_HEARTBEAT_INTERVAL_MS);
+        assertEquals(0.06d, (double) getMetric("heartbeat-rate").metricValue(), 0.005d);
+        assertEquals(2.0, getMetric("heartbeat-total").metricValue());
+        time.sleep(DEFAULT_HEARTBEAT_INTERVAL_MS);


I think here we can test some other value, since we don't really need this to be the heartbeat interval, right?

lucasbru · 2024-01-31T11:22:32Z

+
+        // test poll
+        assertHeartbeat(heartbeatRequestManager, 0);
+        time.sleep(DEFAULT_HEARTBEAT_INTERVAL_MS);


The mixed use of DEFAULT_HEARTBEAT_INTERVAL_MS and heartbeatIntervalMs is confusing. Can use heartbeatIntervalMs consistently (if we need the extra alias) or just remove the alias and always use DEFAULT_HEARTBEAT_INTERVAL_MS ?

lucasbru · 2024-01-31T11:25:07Z

-            heartbeatRequestState,
-            backgroundEventHandler);
+    private KafkaMetric getMetric(final String name) {
+        return metrics.metrics().get(metrics.metricName(name, metricGroupPrefix + COORDINATOR_METRICS_SUFFIX));


I'd suggest using the full metric name (including literal suffix) in these tests, like they appear in the documentation, and not reuse the constants from the prod code (otherwise we won't catch if those constants get modified incorrectly).

lucasbru · 2024-01-31T13:07:53Z

+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+public class ConsumerMetricsManagerTest {


If you think it's worth it splitting each group of metrics in a separate class, I would also split the corresponding unit tests into separate classes in the same metrics package, instead of having a combined unit test for all classes.

lucasbru · 2024-01-31T13:20:36Z

+ * </ul>
+ * </p>
+ */
+public abstract class AbstractConsumerMetricsManager {


You don't have to change this, but personally, I don't think we need subclassing here. It seems all we get from it is a bunch of utilities, that could just be imported. Subclassing should be avoided if not necessary, and it creates complications (see, e.g. the current build failure).

are you suggesting making this a utility class?

See the latest commit - i removed this class to simplify the implementation a bit

As I said, you didn't have to change this, but yes, I felt that the class was adding complexity of inheritance with little benefits.

lucasbru · 2024-02-01T15:15:28Z

+import static org.apache.kafka.clients.consumer.internals.ConsumerUtils.CONSUMER_METRIC_GROUP_PREFIX;
+import static org.apache.kafka.clients.consumer.internals.ConsumerUtils.COORDINATOR_METRICS_SUFFIX;
+
+public class HeartbeatMetricsManager {


I was checking why the CI was misbehaving... it's probably not related to the PR, but while I was checking it, I noticed that other metrics containers like KafkaConsumerMetrics are AutoClosable and are explicitly closed inside LegacyKafkaConsumer and AsyncKafkaConsumer, removing all sensors from the Metrics class. Now, I'm not sure if this would cause any leaks, but it seems it would at least be good style to close metrics managers in the same way. What do you think? Sorry, didn't notice this on the first pass.

I thought about this during the implementation. However, the current RequestManager is not "closeable" nor do we try to close anything during shutdown. I assume these sensors and metric map will be GC'ed upon program termination/fatal failure, so it occur to be a strange thing to do to remove these sensors from the map before closing.

If we need to - we should make RequestManager closeable as well as the metrics, so that we can close these resources upon program termination.

GC should work fine with cyclic references. I suspect the point is that I can close my consumer, and I don't have to wait for GC to have the metrics cleaned up. You might even want to keep a reference to the consumer after it's closed, then GC will not kick in.

I'm not entirely sure if this is just a "nice-to-have" or if there is a strong reason to do this, but since adding a couple of close methods should be a quick thing, I would vote for cleaning the sensors up explicitly.

hey @lucasbru - The real issue is here: #7839

The PR claims it is only affecting KafkaConsumerMetrics: Both KafkaConsumerMetrics and SelectorMetrics are internally using the same mbean.

Okay, if we are not affected by this, not closing it is okay with me.

lucasbru · 2024-02-01T15:16:44Z

I have one last comment, otherwise the PR looks good to me.

philipnee · 2024-02-01T16:43:06Z

@lucasbru - Thank you for spending time reviewing the PR. I responded to your comment about resource closing. It is indeed a bit strange why we need to do this.

A weird thing I found while reviewing sensor code is that - there's a cyclic relationship between sensor and metrics. Sensor holds on to Metric's reference and Metrics holds on to sensor lookup map (which has Sensor as the val). See this

public class Metrics implements Closeable {

    private final MetricConfig config;
    private final ConcurrentMap<MetricName, KafkaMetric> metrics;
    private final ConcurrentMap<String, Sensor> sensors;

and

public final class Sensor {

    private final Metrics registry;

I wonder if this is why we should remove these sensors upon closing.

lucasbru

LGTM, thanks @philipnee. Restarting CI to see if the build failures are transient.

nizhikov · 2024-02-13T06:15:33Z

Hello @philipnee @lucasbru

Why you remove GroupState class in this commit?
This class will be used in java version of ConsoleGroupCommand when #14471 will be fully merged
Class was merged to trunk in ff25c35 to simplify further reviews.

lucasbru · 2024-02-13T08:40:02Z

@nizhikov Sorry, during review I thought @philipnee removed a left-over class from an earlier PR also called GroupState. https://github.com/apache/kafka/blob/6e164bb9ace3ea7a1a9542904d1a01c9fd3a1b48/clients/src/main/java/org/apache/kafka/clients/consumer/internals/GroupState.java but instead he deleted a different class. I should have checked. Please, bring the class back and sorry for the confusion.

nizhikov · 2024-02-13T08:43:40Z

@lucasbru No worries. I wanted to make it clear that class removed by mistake and didn't broke something.
Will return it in the next PR regarding Console Group Command.

Add a HeartbeatMetrics module to measure various metrics related to the heartbeat. Here is the highlight of the changes: HeartMetrics encapsulate a heartbeat sensor Add metrics suffix types to the AbstractConsumerMetrics so that it can be extended to different metric groups Non-related refactor: Rename the metric classes to "MetricManager" the reason being "Metrics" seems to relate to the Metric class however these managers are merely containers holding sensor references for recording purposes. Created OffsetCommitMetricsManager so that we are more consistent with the metrics management Extended KafkaConsumerMetrics to the AbstractConsumerMetrics so that the groupName is consistently created (without random concatenation) Some follow ups: Refactor commit sensor by introducing a CommitMetrics module so that we can keep most of the metrics in one place and let it be a bit more consistent stylistically Possibly refactor fetch manager metrics Reviewers: Lucas Brutschy <lbrutschy@confluent.io>

heartbeat metrics

0d6fcce

philipnee force-pushed the ctr-heartbeat-metrics branch from c26fc90 to 0d6fcce Compare January 19, 2024 18:12

Philip Nee added 3 commits January 19, 2024 10:14

clean up

112f6ce

remove lastHeartbeatsend and use the requestState var instead

946c740

eee wip

update test

6bd15e8

clean up remove bad code

philipnee force-pushed the ctr-heartbeat-metrics branch from e186171 to 6bd15e8 Compare January 25, 2024 22:17

clean up

6759da1

clean up test

philipnee force-pushed the ctr-heartbeat-metrics branch from 4e5db22 to 6759da1 Compare January 26, 2024 19:47

Philip Nee added 2 commits January 26, 2024 12:04

allow the manager to accept a group prefix

39c5239

clean up

c55dae2

philipnee force-pushed the ctr-heartbeat-metrics branch from bfc97f7 to c55dae2 Compare January 26, 2024 23:35

Update AbstractConsumerMetrics.java

df8ac28

philipnee force-pushed the ctr-heartbeat-metrics branch 2 times, most recently from 6fa1ff7 to a8631cb Compare January 29, 2024 00:28

Reconsolidate metrics into consumer coordinator metrics

6151964

clean up

philipnee force-pushed the ctr-heartbeat-metrics branch from a8631cb to 6151964 Compare January 29, 2024 00:55

Philip Nee added 14 commits January 28, 2024 17:04

clean up name

a84f83b

heartbeat metrics

4f39701

clean up

99d6fab

remove lastHeartbeatsend and use the requestState var instead

cf95499

eee wip

update test

631a755

clean up remove bad code

clean up

e96a06e

clean up test

allow the manager to accept a group prefix

6043316

clean up

d0e1728

Update AbstractConsumerMetrics.java

2e425a6

Reconsolidate metrics into consumer coordinator metrics

308daa2

clean up

clean up name

8824608

resolving conflict

b17455e

Merge branch 'ctr-heartbeat-metrics' of https://github.com/philipnee/…

31b9757

…kafka into ctr-heartbeat-metrics

resolve conflict

6285909

philipnee force-pushed the ctr-heartbeat-metrics branch from 867a10b to 6285909 Compare January 29, 2024 05:19

Philip Nee added 2 commits January 29, 2024 08:10

make naming consistent

1424ed9

clean up

7076ba5

philipnee commented Jan 29, 2024

View reviewed changes

Update HeartbeatRequestManagerTest.java

4eddba4

philipnee marked this pull request as ready for review January 29, 2024 19:02

Philip Nee added 2 commits January 29, 2024 11:24

privatize metric field

9c86077

Merge branch 'trunk' into ctr-heartbeat-metrics

336c443

philipnee requested a review from lucasbru January 29, 2024 23:47

lucasbru requested changes Jan 31, 2024

View reviewed changes

Philip Nee added 2 commits January 31, 2024 09:15

PR comments

8964a8d

clean up according to PR comments

22fcb39

lucasbru reviewed Feb 1, 2024

View reviewed changes

lucasbru approved these changes Feb 2, 2024

View reviewed changes

lucasbru merged commit f0087ac into apache:trunk Feb 2, 2024

lucasbru added the ctr Consumer Threading Refactor (KIP-848) label Jan 8, 2025

Uh oh!

Conversation

philipnee commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipnee commented Jan 29, 2024

Uh oh!

lucasbru commented Jan 30, 2024

Uh oh!

lucasbru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipnee Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasbru commented Feb 1, 2024

Uh oh!

philipnee commented Feb 1, 2024

Uh oh!

lucasbru left a comment

Choose a reason for hiding this comment

Uh oh!

nizhikov commented Feb 13, 2024

Uh oh!

lucasbru commented Feb 13, 2024

Uh oh!

nizhikov commented Feb 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

philipnee commented Jan 17, 2024 •

edited

Loading

philipnee Feb 1, 2024 •

edited

Loading