KAFKA-19773: Include push interval in ClientTelemetryReceiver context #20672

see-quick · 2025-10-09T11:20:55Z

This PR implements include push internal by introducing a new
ClientTelemetryExporter API. More details can be found in the KIP [1].

[1] -
https://cwiki.apache.org/confluence/display/KAFKA/KIP-1217%3A+Include+push+interval+in+ClientTelemetryReceiver+context

server/src/main/java/org/apache/kafka/server/metrics/ClientTelemetryPlugin.java

mimaison

Thanks for the PR. I've made a first pass and left a few suggestions.

clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetry.java

clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryContext.java

clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetry.java

...src/test/java/org/apache/kafka/server/metrics/ClientTelemetryInterfaceCompatibilityTest.java

AndrewJSchofield

Thanks for the PR. I've done an initial review only so far.

clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryExporterProvider.java

core/src/main/scala/kafka/server/BrokerServer.scala

server/src/main/java/org/apache/kafka/server/metrics/ClientTelemetryPlugin.java

AndrewJSchofield · 2025-10-21T15:23:10Z

@see-quick Please can you merge latest changes from trunk into your branch. The failing test has been marked as flaky and your build should pass.

core/src/main/scala/kafka/server/DynamicBrokerConfig.scala

mimaison

Thanks for the updates, I made another pass and left a few comments.

core/src/test/scala/unit/kafka/server/DynamicBrokerConfigTest.scala

server/src/main/java/org/apache/kafka/server/metrics/ClientTelemetryExporterPlugin.java

server/src/main/java/org/apache/kafka/server/metrics/DefaultClientTelemetryContext.java

Signed-off-by: see-quick <[email protected]>

…lugin Signed-off-by: see-quick <[email protected]>

Signed-off-by: see-quick <[email protected]>

core/src/test/scala/unit/kafka/server/DynamicBrokerConfigTest.scala

Signed-off-by: see-quick <[email protected]>

mimaison

Thanks for the updates. Should we also update ClientTelemetryTest and KafkaStreamsTelemetryIntegrationTest to use the new (or maybe both) APIs?

server/src/main/java/org/apache/kafka/server/metrics/DefaultClientTelemetryContext.java

AndrewJSchofield

I have just one nit comment remaining. Apart from that, looks good to me.

clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryExporterProvider.java

Signed-off-by: see-quick <[email protected]>

...ents-integration-tests/src/test/java/org/apache/kafka/clients/admin/ClientTelemetryTest.java

see-quick · 2025-10-29T13:39:16Z

The failing test is not related to this PR more [1].

[1] - https://issues.apache.org/jira/browse/KAFKA-19840

AndrewJSchofield

Thanks for the PR. Looks good from my point of view.

mimaison

I'm really not sure the extensive changes made to KafkaStreamsTelemetryIntegrationTest are worth it. The new tests don't seem to be actually testing the new features from KIP-1217.

...tion-tests/src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryBase.java

server/src/test/java/org/apache/kafka/server/metrics/ClientTelemetryInterfaceTest.java

Signed-off-by: see-quick <[email protected]>

mimaison

LGTM

mimaison · 2025-11-04T14:37:24Z

The test failures in ShareConsumerTest > testShareGroupMaxSizeConfigExceeded are covered in https://issues.apache.org/jira/browse/KAFKA-19840

brandboat · 2025-11-05T01:26:18Z

server/src/test/java/org/apache/kafka/server/metrics/ClientMetricsTestUtils.java

            false);
    }

+    @SuppressWarnings("deprecation")


Hi all, this should be replaced with removal. I got below warnings when building kafka. fixed in #20830.

/Users/brandboat/Code/kafka/kafka/server/src/test/java/org/apache/kafka/server/metrics/ClientMetricsTestUtils.java:102: warning: [removal] ClientTelemetryReceiver in org.apache.kafka.server.telemetry has been deprecated and marked for removal public static class TestClientMetricsReceiver implements ClientTelemetryReceiver { ^ /Users/brandboat/Code/kafka/kafka/server/src/test/java/org/apache/kafka/server/metrics/ClientMetricsTestUtils.java:130: warning: [removal] ClientTelemetry in org.apache.kafka.server.telemetry has been deprecated and marked for removal public static class TestDualImplementation implements ClientTelemetry, ClientTelemetryExporterProvider { ^ /Users/brandboat/Code/kafka/kafka/server/src/test/java/org/apache/kafka/server/metrics/ClientMetricsTestUtils.java:140: warning: [removal] ClientTelemetryReceiver in org.apache.kafka.server.telemetry has been deprecated and marked for removal public ClientTelemetryReceiver clientReceiver() { ^

Suggested change

@SuppressWarnings("deprecation")

@SuppressWarnings("removal")

… (KIP-1217) (apache#20672) Reviewers: Mickael Maison <[email protected]>, Andrew Schofield <[email protected]>

github-actions bot added triage PRs from the community core Kafka Broker clients labels Oct 9, 2025

see-quick commented Oct 9, 2025

View reviewed changes

server/src/main/java/org/apache/kafka/server/metrics/ClientTelemetryPlugin.java Outdated Show resolved Hide resolved

github-actions bot removed the triage PRs from the community label Oct 10, 2025

see-quick marked this pull request as ready for review October 10, 2025 11:51

mimaison added the ci-approved label Oct 14, 2025

mimaison reviewed Oct 14, 2025

View reviewed changes

mimaison requested review from AndrewJSchofield and apoorvmittal10 October 14, 2025 15:04

see-quick force-pushed the kip-1217 branch from 021eda0 to f4d2bba Compare October 16, 2025 11:19

AndrewJSchofield requested changes Oct 20, 2025

View reviewed changes

AndrewJSchofield self-requested a review October 21, 2025 15:10

see-quick force-pushed the kip-1217 branch from 98528ab to a1a3b21 Compare October 21, 2025 17:58

see-quick commented Oct 22, 2025

View reviewed changes

core/src/main/scala/kafka/server/DynamicBrokerConfig.scala Show resolved Hide resolved

mimaison reviewed Oct 24, 2025

View reviewed changes

see-quick added 10 commits October 27, 2025 11:50

KAFKA-19773: Include push interval in ClientTelemetryReceiver context

f92152c

Signed-off-by: see-quick <[email protected]>

mickael review

276dfec

Signed-off-by: see-quick <[email protected]>

eof

33844ac

Signed-off-by: see-quick <[email protected]>

spotbugs issues

6fade3f

Signed-off-by: see-quick <[email protected]>

review from andrew

fff2f78

Signed-off-by: see-quick <[email protected]>

a nit with isTelemetryExporterConfigured

ce92468

Signed-off-by: see-quick <[email protected]>

update dynamicbrokerconfig naming to reflect clientTelemetryExporterP…

776ade2

…lugin Signed-off-by: see-quick <[email protected]>

add test case for dynamic configuration within client telemetry exporter

65f8677

Signed-off-by: see-quick <[email protected]>

remove prefix impornts and directly import it

59b29b6

Signed-off-by: see-quick <[email protected]>

be more strict on verify part with exactly once interactions

73f3348

Signed-off-by: see-quick <[email protected]>

see-quick force-pushed the kip-1217 branch from 764ed01 to 73f3348 Compare October 27, 2025 10:52

mimaison reviewed Oct 28, 2025

View reviewed changes

core/src/test/scala/unit/kafka/server/DynamicBrokerConfigTest.scala Outdated Show resolved Hide resolved

mickael review simplification

4b1fb23

Signed-off-by: see-quick <[email protected]>

mimaison reviewed Oct 28, 2025

View reviewed changes

server/src/main/java/org/apache/kafka/server/metrics/DefaultClientTelemetryContext.java Outdated Show resolved Hide resolved

AndrewJSchofield requested changes Oct 29, 2025

View reviewed changes

clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryExporterProvider.java Outdated Show resolved Hide resolved

github-actions bot added the streams label Oct 29, 2025

add also intergration tests for streams and clients + address Andrew

f16831e

Signed-off-by: see-quick <[email protected]>

see-quick force-pushed the kip-1217 branch from 09dd449 to f16831e Compare October 29, 2025 12:24

see-quick commented Oct 29, 2025

View reviewed changes

...ents-integration-tests/src/test/java/org/apache/kafka/clients/admin/ClientTelemetryTest.java Outdated Show resolved Hide resolved

see-quick commented Oct 29, 2025

View reviewed changes

...ents-integration-tests/src/test/java/org/apache/kafka/clients/admin/ClientTelemetryTest.java Outdated Show resolved Hide resolved

AndrewJSchofield approved these changes Oct 29, 2025

View reviewed changes

mimaison reviewed Oct 30, 2025

View reviewed changes

...tion-tests/src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryBase.java Outdated Show resolved Hide resolved

server/src/test/java/org/apache/kafka/server/metrics/ClientTelemetryInterfaceTest.java Outdated Show resolved Hide resolved

see-quick added 5 commits October 31, 2025 13:04

support just new API in streams tests

2279146

Signed-off-by: see-quick <[email protected]>

checkstyle

21f98bc

Signed-off-by: see-quick <[email protected]>

proper formatting

bf6e26f

Signed-off-by: see-quick <[email protected]>

adjust import control

e7f1ee2

Signed-off-by: see-quick <[email protected]>

adjust javadoc to mention just new API

2aa3bc4

Signed-off-by: see-quick <[email protected]>

mimaison approved these changes Nov 4, 2025

View reviewed changes

mimaison merged commit 680b871 into apache:trunk Nov 4, 2025
17 of 20 checks passed

brandboat reviewed Nov 5, 2025

View reviewed changes

	@SuppressWarnings("deprecation")
	@SuppressWarnings("removal")

KAFKA-19773: Include push interval in ClientTelemetryReceiver context #20672

KAFKA-19773: Include push interval in ClientTelemetryReceiver context #20672

Uh oh!

Conversation

see-quick commented Oct 9, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewJSchofield commented Oct 21, 2025

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

see-quick commented Oct 29, 2025

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison commented Nov 4, 2025

Uh oh!

Uh oh!

brandboat Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

see-quick commented Oct 9, 2025 •

edited by github-actions bot

Loading