KAFKA-9960: implement KIP-606 to add metadata context to MetricsReporter by xiaodongdu · Pull Request #8691 · apache/kafka

xiaodongdu · 2020-05-18T23:42:39Z

Implement KIP-606, add metadata context to MetricsReporter:

Added a new api to MetricsReporter to allow client to expose additional metadata fields to reporter plugin. Added an interface MetricsContext to encapsulate metadata.
Deprecated JmexReporter(String prefix) constructor. The prefix will be passed to the reporter via MetricsContext.
Replaced existing usage of JmxReporter with the default ImxReporter and pass JMX prefix to MetricsContext using _namespace as key.
From Kafka broker, populate MetricsContext with: kafka.cluster.id and kafka.nroker.id
From Connect, populate MetricsContext with: connect.kafka.cluster.id, connect.group.id

Committer Checklist (excluded from commit message)

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

xiaodongdu · 2020-05-19T00:01:52Z

@xvrl @rhauch @ijuma Could you review this PR for KIP-606. Thanks,

add changes to connect as part of KIP 606 implementation

rhauch

Thanks, @xiaodongdu. I'm only part way through the Connect portion, but I already have some comments/suggestions.

I'll continue to review over the next day or two.

rhauch · 2020-05-19T19:32:11Z

ok to test

rnpridgeon · 2020-05-19T20:49:00Z

Since MetricsReporter instances are the ultimate consumers of the MetadataContext instance I would like to propose moving construction MetricsContext there. This would allow each reporter to manage its own set of labels increasing encapsulation and extensibility.

…et cluster id

… kafka-9960-kip-606

rnpridgeon · 2020-05-21T16:16:23Z

+     */
+    public KafkaMetricsContext(String namespace, Map<String, ?> metadata) {
+        this.metadata.put(MetricsContext.NAMESPACE, namespace);
+        metadata.forEach((key, value) -> this.metadata.put(key, value.toString()));


Use putIfAbsent to avoid silently overwriting over labels set upstream.

I think we it's ok for the component that owns the reporter to take precedence over the labels passed from upstream. We did not specify the behavior in the KIP, so implementations should use namespacing of labels to avoid this. If in practice we find this behavior is less desirable, we can file a follow-on KIP, since the interface is till evolving.

Mostly I'm concerned about the case where some composite may share similar labels to the underlying client it manages. If we allow the downstream client to overwrite such a label we will lose a portion of the upstream components context.

the client currently only injects the labels passed in via client properties, so that wouldn't happen

rnpridgeon · 2020-05-22T12:27:59Z

 public final class ConnectUtils {
    private static final Logger log = LoggerFactory.getLogger(ConnectUtils.class);
+    public static final String CONNECT_KAFKA_CLUSTER_ID = "connect.kafka.cluster.id";
+    public static final String CONNECT_GROUP_ID = "connect.group.id";


Maybe it would be helpful to add CONNECT_VERSION as well.

Version is added in ce-kafka.

rhauch · 2020-05-26T23:46:38Z

+     * @param metricsContext the metric context
+     */
+    @InterfaceStability.Evolving
+    default void contextChange(MetricsContext metricsContext) {


It would also be good to identify when this is called relative to other methods. For example, it is always called before init(...) is called. But can it be called again, or is that the only time this method is called?

rhauch · 2020-05-26T23:48:28Z

ok to test

rhauch · 2020-05-27T14:24:11Z

@xiaodongdu please address the conflicts.

rhauch

It'd be nice to get clarification on the call pattern for MetricsReporter.contextChange(..) method to potentially update the JavaDoc. Otherwise, this looks good for the general code and Connect-related code.

rhauch · 2020-05-27T17:56:01Z

ok to test

rhauch

I've reviewed the common code, Connect, and MirrorMaker2. Those parts LGTM, pending a green build.

rhauch · 2020-05-27T18:40:11Z

ok to test

rhauch · 2020-05-27T18:40:33Z

retest this please

rhauch · 2020-05-27T19:11:02Z

retest this please

kkonstantine · 2020-05-27T19:15:59Z

ok to test

kkonstantine · 2020-05-27T19:16:25Z

retest this please

rhauch · 2020-05-27T19:19:12Z

ok to test

rhauch · 2020-05-27T19:19:26Z

retest this please

rhauch · 2020-05-27T19:40:50Z

retest this please

rhauch · 2020-05-27T19:41:03Z

retest this please

rhauch · 2020-05-27T20:18:58Z

ok to test

rhauch · 2020-05-27T20:47:07Z

retest this please

rhauch · 2020-05-27T23:57:21Z

One seemingly unrelated failure on JDK11 with a known flaky test:

kafka.api.PlaintextProducerSendTest.testNonBlockingProducer
    java.util.concurrent.TimeoutException: Timeout after waiting for 10000 ms.

mumrah

LGTM

xiaodongdu added 14 commits May 12, 2020 18:25

KAFKA-9960: implement KIP-606 to add metadata context to MetricsReporter

5232433

add changes to connect as part of KIP 606 implementation

82c0a0d

fix string constant

5d388cf

Changes to address comments

92d6389

Fix a issue when setting group id

e893850

use group.id from original config when pass connect.group.id to client

2b36bc6

remove redundant call, add more unit test

24bc78e

More changes to address code review comments

65195e1

More changes to address code review comments

9511de8

More changes to address code review coments

4964aef

More changes to address code review coments

8fd4ef5

add an integration test, strip off metrics.context from metrics context

69e4b7c

More changes to address PR review

0e1a5b6

Remove support of reading metric context properties for broker

386a5da

rhauch added connect core Kafka Broker streams labels May 19, 2020

rnpridgeon mentioned this pull request May 19, 2020

KIP-606 MetricsContext confluentinc/rest-utils#180

Merged

KAFKA-9960: implement KIP-606 to add metadata context to MetricsReporter

6005c9a

add changes to connect as part of KIP 606 implementation

xiaodongdu force-pushed the kafka-9960-kip-606 branch from 386a5da to 6005c9a Compare May 19, 2020 16:37

rhauch reviewed May 19, 2020

View reviewed changes

xiaodongdu added 2 commits May 20, 2020 08:51

Remove cluster id from a few constructors and use utility method to g…

d78a1e0

…et cluster id

Merge branch 'kafka-9960-kip-606' of github.com:xiaodongdu/kafka into…

2484d71

… kafka-9960-kip-606

xiaodongdu requested a review from rhauch May 20, 2020 18:01

remove clusterId from WorkGroupMember constructor

85f7421

rnpridgeon reviewed May 21, 2020

View reviewed changes

rnpridgeon reviewed May 22, 2020

View reviewed changes

rhauch reviewed May 26, 2020

View reviewed changes

Update javadoc and add annotation

071872f

xiaodongdu requested a review from rhauch May 27, 2020 00:43

rhauch reviewed May 27, 2020

View reviewed changes

Update javadoc

7bcd057

xiaodongdu requested a review from rhauch May 27, 2020 16:12

Merge remote-tracking branch 'origin/trunk' into kafka-9960-kip-606

319f5c5

rhauch approved these changes May 27, 2020

View reviewed changes

rhauch closed this May 27, 2020

rhauch reopened this May 27, 2020

andrewchoi5 approved these changes May 27, 2020

View reviewed changes

mumrah approved these changes May 28, 2020

View reviewed changes

rhauch merged commit 9c833f6 into apache:trunk May 28, 2020

rhauch mentioned this pull request Jun 5, 2020

KAFKA-10110: Corrected potential NPE when null label value added to KafkaMetricsContext #8811

Merged

3 tasks

mjsax added the kip Requires or implements a KIP label Jun 12, 2020

Uh oh!

Conversation

xiaodongdu commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

xiaodongdu commented May 19, 2020

Uh oh!

rhauch 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!

Uh oh!

Uh oh!

rhauch commented May 19, 2020

Uh oh!

rnpridgeon commented May 19, 2020

Uh oh!

rnpridgeon May 21, 2020

Choose a reason for hiding this comment

Uh oh!

xvrl May 22, 2020

Choose a reason for hiding this comment

Uh oh!

rnpridgeon May 22, 2020

Choose a reason for hiding this comment

Uh oh!

xvrl May 22, 2020

Choose a reason for hiding this comment

Uh oh!

rnpridgeon May 22, 2020 • 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!

xiaodongdu May 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rhauch May 26, 2020

Choose a reason for hiding this comment

Uh oh!

rhauch commented May 26, 2020

Uh oh!

rhauch commented May 27, 2020

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

rhauch commented May 27, 2020

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

rhauch commented May 27, 2020

Uh oh!

rhauch commented May 27, 2020

Uh oh!

rhauch commented May 27, 2020

Uh oh!

kkonstantine commented May 27, 2020

Uh oh!

kkonstantine commented May 27, 2020

Uh oh!

rhauch commented May 27, 2020

Uh oh!

rhauch commented May 27, 2020

Uh oh!

rhauch commented May 27, 2020

Uh oh!

rhauch commented May 27, 2020

Uh oh!

rhauch commented May 27, 2020

xiaodongdu commented May 18, 2020 •

edited

Loading

rnpridgeon May 22, 2020 •

edited

Loading