You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/01/08 00:56:01 UTC
[GitHub] [kafka] kirktrue commented on a change in pull request #9584: [KAFKA-10708]: Add "group-id" Tag to Kafka Consumer Metrics
kirktrue commented on a change in pull request #9584:
URL: https://github.com/apache/kafka/pull/9584#discussion_r780603531
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
##########
@@ -858,14 +862,23 @@ public KafkaConsumer(Map<String, Object> configs,
this.kafkaConsumerMetrics = new KafkaConsumerMetrics(metrics, "consumer");
}
- private static Metrics buildMetrics(ConsumerConfig config, Time time, String clientId) {
- Map<String, String> metricsTags = Collections.singletonMap(CLIENT_ID_METRIC_TAG, clientId);
+ private static Metrics buildMetrics(ConsumerConfig config, Time time, String clientId, String groupId) {
+ Map<String, String> metricsTags = new HashMap<>();
+ metricsTags.put(CLIENT_ID_METRIC_TAG, clientId);
+ if (groupId != null && !groupId.trim().isEmpty()) {
Review comment:
The secondary `!groupId.trim().isEmpty()` clause of the `if` statement shouldn't ever catch given that that case is checked for in the constructor before calling `buildMetrics`, right?
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
##########
@@ -858,14 +862,23 @@ public KafkaConsumer(Map<String, Object> configs,
this.kafkaConsumerMetrics = new KafkaConsumerMetrics(metrics, "consumer");
}
- private static Metrics buildMetrics(ConsumerConfig config, Time time, String clientId) {
- Map<String, String> metricsTags = Collections.singletonMap(CLIENT_ID_METRIC_TAG, clientId);
+ private static Metrics buildMetrics(ConsumerConfig config, Time time, String clientId, String groupId) {
+ Map<String, String> metricsTags = new HashMap<>();
+ metricsTags.put(CLIENT_ID_METRIC_TAG, clientId);
+ if (groupId != null && !groupId.trim().isEmpty()) {
+ metricsTags.put(GROUP_ID_METRIC_TAG, groupId);
+ }
MetricConfig metricConfig = new MetricConfig().samples(config.getInt(ConsumerConfig.METRICS_NUM_SAMPLES_CONFIG))
.timeWindow(config.getLong(ConsumerConfig.METRICS_SAMPLE_WINDOW_MS_CONFIG), TimeUnit.MILLISECONDS)
.recordLevel(Sensor.RecordingLevel.forName(config.getString(ConsumerConfig.METRICS_RECORDING_LEVEL_CONFIG)))
.tags(metricsTags);
+ final Map<String, Object> reporterConfigOverrides = new HashMap<>();
+ reporterConfigOverrides.put(ConsumerConfig.CLIENT_ID_CONFIG, clientId);
+ if (groupId != null && !groupId.trim().isEmpty()) {
Review comment:
Same minor nitpick about whether or not we need to check for an empty group ID.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org