KAFKA-17248 - KIP 1076 implementation #17021

bbejeck · 2024-08-27T22:25:47Z

Implementation of KIP-1076 to allow for adding client application metrics to the KIP-714 framework

Integration and unit tests forthcoming

Committer Checklist (excluded from commit message)

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

apoorvmittal10

Thanks for the PR @bbejeck. I have taken a pass and some comments.

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java

clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java

.../java/org/apache/kafka/streams/internals/metrics/StreamsThreadDelegatingMetricsReporter.java

streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java

...src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryIntegrationTest.java

bbejeck · 2024-09-05T21:36:40Z

Thanks for the comments @apoorvmittal10 - I've addressed your comments.

mjsax

Not sure if I understand the end-to-end wiring already, so need to make another pass.

Btw: should we also verify state store metrics (or are they automatically verified as part of task metrics)?

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java

.../java/org/apache/kafka/streams/internals/metrics/StreamsThreadDelegatingMetricsReporter.java

...src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryIntegrationTest.java

bbejeck · 2024-09-09T20:52:25Z

should we also verify state store metrics (or are they automatically verified as part of task metrics)?

They are covered as part of task metrics, but I expanded the test to explicitly verify state store metrics are correct as well

mjsax

Thanks for the PR update. Made another pass.

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java

.../java/org/apache/kafka/streams/internals/metrics/StreamsClientMetricsDelegatingReporter.java

.../java/org/apache/kafka/streams/internals/metrics/StreamsThreadMetricsDelegatingReporter.java

...src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryIntegrationTest.java

bbejeck · 2024-09-13T17:21:38Z

@mjsax thanks for the second review, I've addressed your comments

.../java/org/apache/kafka/streams/internals/metrics/StreamsClientMetricsDelegatingReporter.java

bbejeck · 2024-09-30T17:39:58Z

@mjsax, @apoorvmittal10, @AndrewJSchofield comments addressed

...src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryIntegrationTest.java

AndrewJSchofield

Just one remaining comment.

streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java

bbejeck · 2024-10-01T17:46:41Z

@AndrewJSchofield, thanks for the review; I've addressed your comment.

AndrewJSchofield

lgtm

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java

...src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryIntegrationTest.java

mjsax · 2024-10-02T23:31:58Z

streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java

@@ -1812,7 +1812,6 @@ public Map<String, Object> getAdminConfigs(final String clientId) {

        // add client id with stream client id prefix
        props.put(CommonClientConfigs.CLIENT_ID_CONFIG, clientId);
-


blank line should be back in now

Still see it :)

…t; update tests

…, fix test

bbejeck · 2024-10-28T21:25:25Z

...src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryIntegrationTest.java

+                    .filter(metricName -> metricName.tags().containsKey("thread-id")).map(mn -> {
+                        final String name = mn.name().replace('-', '.');
+                        final String group = mn.group().replace("-metrics", "").replace('-', '.');
+                        return "org.apache.kafka." + group + "." + name;


Need to format the Kafka Streams metrics names into the KIP-714 naming format convention.

all clients in the ClientsInstanceIds in consumerInstanceIds map. This PR makes sure only the main consumer client instance id is selected.

mjsax · 2024-11-02T00:44:29Z

@bbejeck Can you rebase this PR to resolve merge conflicts?

bbejeck marked this pull request as draft August 27, 2024 22:26

bbejeck added streams kip Requires or implements a KIP KIP-1076 Metrics for client applications KIP-714 extension labels Aug 27, 2024

bbejeck force-pushed the kip_1076_implementation branch from 4e8ffa7 to d390f0a Compare August 28, 2024 22:11

bbejeck changed the title ~~[WIP NO MERGE] KAFKA-17248 - KIP 1076 implementation~~ KAFKA-17248 - KIP 1076 implementation Sep 2, 2024

bbejeck force-pushed the kip_1076_implementation branch from 61270fb to b566d34 Compare September 3, 2024 20:27

bbejeck marked this pull request as ready for review September 3, 2024 20:33

bbejeck force-pushed the kip_1076_implementation branch from d6f9919 to b96b967 Compare September 4, 2024 16:28

apoorvmittal10 self-requested a review September 4, 2024 20:23

apoorvmittal10 suggested changes Sep 5, 2024

View reviewed changes

mjsax reviewed Sep 8, 2024

View reviewed changes

mjsax reviewed Sep 12, 2024

View reviewed changes

bbejeck force-pushed the kip_1076_implementation branch from 020d7c4 to 965fd23 Compare September 13, 2024 17:00

apoorvmittal10 suggested changes Sep 25, 2024

View reviewed changes

.../java/org/apache/kafka/streams/internals/metrics/StreamsClientMetricsDelegatingReporter.java Outdated Show resolved Hide resolved

.../java/org/apache/kafka/streams/internals/metrics/StreamsClientMetricsDelegatingReporter.java Outdated Show resolved Hide resolved

bbejeck force-pushed the kip_1076_implementation branch from 965fd23 to 8d64d7d Compare September 25, 2024 19:38

github-actions bot added producer consumer clients labels Sep 25, 2024

bbejeck force-pushed the kip_1076_implementation branch from b3d420d to 235ccb6 Compare September 28, 2024 15:36

github-actions bot added the core Kafka Broker label Sep 28, 2024

bbejeck commented Sep 30, 2024

View reviewed changes

...src/test/java/org/apache/kafka/streams/integration/KafkaStreamsTelemetryIntegrationTest.java Show resolved Hide resolved

AndrewJSchofield suggested changes Oct 1, 2024

View reviewed changes

streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java Outdated Show resolved Hide resolved

AndrewJSchofield approved these changes Oct 2, 2024

View reviewed changes

mjsax reviewed Oct 2, 2024

View reviewed changes

bbejeck and others added 21 commits October 24, 2024 16:23

Add the telemetry reporter to the reporters list after creation

f3a944b

Fix merge conflic from Fix broken tests in KafkaAdminClientTest

90f117e

Minor cleanup

c164fb3

Fix merge conflict from Disable metrics push in AdminClient by defaul…

1f295eb

…t; update tests

Fix merge conflict from fix tests, start of end-to-end metrics test

013baf4

Call initiate close on ClientTelemetryReporter during admin close

b88930c

Updates per comments

dff3114

Fix missed items during rebase

3c9b0b9

Fix constructor that got mangled during rebase

578b48a

Fixes for resolve conflict merge mistakes during rebase

6bb3809

Address review comments

00e3f73

Checkstyle fixes

3183ebf

Address Comments

4db8b73

Completed integration test

69e014e

Get initial end-to-end integration test running

8f26d73

Respond to comments

d306faf

More descriptive name

ee5f45b

Add open telemetry protobuf as test dependency, update import control…

af727b8

…, fix test

Address comments, fix a potential bug and side cleanup

504f00c

Store clientInstanceId before returning

3a4eef2

Address review comments

cd8e201

bbejeck force-pushed the kip_1076_implementation branch from d3e36be to cd8e201 Compare October 24, 2024 20:28

Update end-to-end test

a893bca

bbejeck commented Oct 28, 2024

View reviewed changes

Update end-to-end test to validate metrics for INFO, DEBUG and TRACE

078b095

bbejeck mentioned this pull request Oct 29, 2024

KAFKA-17248 - KIP 1076 Add process-id to get carried to telemetry results #17630

Open

3 tasks

bbejeck added 3 commits October 30, 2024 15:57

commit f30f2d0 puts

0c46a53

all clients in the ClientsInstanceIds in consumerInstanceIds map. This PR makes sure only the main consumer client instance id is selected.

Standardize on lower-case if names ever change

9222b2c

Standardize on lower-case if names ever change

623cb7b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-17248 - KIP 1076 implementation #17021

KAFKA-17248 - KIP 1076 implementation #17021

bbejeck commented Aug 27, 2024 •

edited

Loading

apoorvmittal10 left a comment •

edited

Loading

bbejeck commented Sep 5, 2024

mjsax left a comment

bbejeck commented Sep 9, 2024

mjsax left a comment

bbejeck commented Sep 13, 2024

bbejeck commented Sep 30, 2024 •

edited

Loading

AndrewJSchofield left a comment

bbejeck commented Oct 1, 2024

AndrewJSchofield left a comment

mjsax Oct 2, 2024

bbejeck Oct 17, 2024

mjsax Oct 22, 2024

bbejeck Oct 28, 2024

mjsax commented Nov 2, 2024

		@@ -1812,7 +1812,6 @@ public Map<String, Object> getAdminConfigs(final String clientId) {

		// add client id with stream client id prefix
		props.put(CommonClientConfigs.CLIENT_ID_CONFIG, clientId);

KAFKA-17248 - KIP 1076 implementation #17021

Are you sure you want to change the base?

KAFKA-17248 - KIP 1076 implementation #17021

Conversation

bbejeck commented Aug 27, 2024 • edited Loading

Committer Checklist (excluded from commit message)

apoorvmittal10 left a comment • edited Loading

Choose a reason for hiding this comment

bbejeck commented Sep 5, 2024

mjsax left a comment

Choose a reason for hiding this comment

bbejeck commented Sep 9, 2024

mjsax left a comment

Choose a reason for hiding this comment

bbejeck commented Sep 13, 2024

bbejeck commented Sep 30, 2024 • edited Loading

AndrewJSchofield left a comment

Choose a reason for hiding this comment

bbejeck commented Oct 1, 2024

AndrewJSchofield left a comment

Choose a reason for hiding this comment

mjsax Oct 2, 2024

Choose a reason for hiding this comment

bbejeck Oct 17, 2024

Choose a reason for hiding this comment

mjsax Oct 22, 2024

Choose a reason for hiding this comment

bbejeck Oct 28, 2024

Choose a reason for hiding this comment

mjsax commented Nov 2, 2024

bbejeck commented Aug 27, 2024 •

edited

Loading

apoorvmittal10 left a comment •

edited

Loading

bbejeck commented Sep 30, 2024 •

edited

Loading