KAFKA-6069: Properly tag KafkaStreams metrics with the client id. #4081
Conversation
|
lgtm |
|
Thanks for the patch @twbecker. It would be nice to have a test for this, but there doesn't seem to be a reasonable approach ATM. LGTM. |
|
This is a great catch, thanks @twbecker ! Maybe you can take a look at |
|
@guozhangwang Gave it a shot. |
| static final Map<MetricName, KafkaMetric> METRICS = new HashMap<>(); | ||
|
|
||
| @Override | ||
| public void configure(Map<String, ?> configs) { } |
There was a problem hiding this comment.
nit: make parameter final. Same for other test methods.
|
Thanks, @twbecker left some comments. |
| public void configure(final Map<String, ?> configs) { } | ||
|
|
||
| @Override | ||
| public void init(List<KafkaMetric> metrics) { |
There was a problem hiding this comment.
nit: final for parameter here
|
|
||
| @Override | ||
| public void init(List<KafkaMetric> metrics) { | ||
| for (KafkaMetric metric : metrics) { |
There was a problem hiding this comment.
It never occurs to me to sprinkle these finals everywhere ;) If this is the standard, might I humbly suggest that Checkstyle be configured to enforce it? Would probably save both reviewers and contributors some time.
| StreamsKafkaClient.create(new StreamsConfig(config)); | ||
| assertFalse(TestMetricsReporter.METRICS.isEmpty()); | ||
| for (KafkaMetric kafkaMetric : TestMetricsReporter.METRICS.values()) { | ||
| assertEquals("some_client_id", kafkaMetric.metricName().tags().get("client-id")); |
There was a problem hiding this comment.
Since just one value to check do we need the for loop?
There was a problem hiding this comment.
There are ~30 metrics registered at this point. I'm just asserting that they are all tagged with the proper client id, rather than how many there are.
|
@twbecker thanks for following up. Left some minor comments, otherwise LGTM. |
|
Addressed comments and rebased, since there's been some churn. |
|
ping @guozhangwang @dguy for final review and merging. |
Author: Tommy Becker <tobecker@tivo.com> Reviewers: Bill Bejeck <bill@confluent.io>, Damian Guy <damian.guy@gmail.com> Closes #4081 from twbecker/KAFKA-6069 (cherry picked from commit 249e398) Signed-off-by: Damian Guy <damian.guy@gmail.com>
|
merged to trunk and 1.0 |
|
Thanks @dguy |
Author: Tommy Becker <tobecker@tivo.com> Reviewers: Bill Bejeck <bill@confluent.io>, Damian Guy <damian.guy@gmail.com> Closes apache#4081 from twbecker/KAFKA-6069
No description provided.