-
Notifications
You must be signed in to change notification settings - Fork 14.6k
KAFKA-15186 AppInfo metrics don't contain the client-id #20493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
@@ -67,8 +68,11 @@ public static synchronized void registerAppInfo(String prefix, String id, Metric | |||
} | |||
AppInfo mBean = new AppInfo(nowMs); | |||
server.registerMBean(mBean, name); | |||
|
|||
registerMetrics(metrics, mBean); // prefix will be added later by JmxReporter | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the indent.
registerMetrics(metrics, mBean); // prefix will be added later by JmxReporter | ||
|
||
registerMetrics(metrics, mBean, null); // prefix will be added later by JmxReporter | ||
if (!metrics.config().tags().containsKey("client-id")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please move this condition into registerMetrics
? for example:
public static synchronized void unregisterAppInfo(String prefix, String id, Metrics metrics) {
MBeanServer server = ManagementFactory.getPlatformMBeanServer();
try {
ObjectName name = new ObjectName(prefix + ":type=app-info,id=" + Sanitizer.jmxSanitize(id));
if (server.isRegistered(name))
server.unregisterMBean(name);
unregisterMetrics(metrics, id);
} catch (JMException e) {
log.warn("Error unregistering AppInfo mbean", e);
} finally {
log.info("App info {} for {} unregistered", prefix, id);
}
}
private static void registerMetrics(Metrics metrics, AppInfo appInfo, String clientId) {
if (metrics == null) return;
// add comments for this case
metrics.addMetric(metricName(metrics, "version", Map.of()), (Gauge<String>) (config, now) -> appInfo.getVersion());
metrics.addMetric(metricName(metrics, "commit-id", Map.of()), (Gauge<String>) (config, now) -> appInfo.getCommitId());
metrics.addMetric(metricName(metrics, "start-time-ms", Map.of()), (Gauge<Long>) (config, now) -> appInfo.getStartTimeMs());
// add comments for this case
if (!metrics.config().tags().containsKey("client-id")) {
metrics.addMetric(metricName(metrics, "version", Map.of("client-id", clientId)), (Gauge<String>) (config, now) -> appInfo.getVersion());
metrics.addMetric(metricName(metrics, "commit-id", Map.of("client-id", clientId)), (Gauge<String>) (config, now) -> appInfo.getCommitId());
metrics.addMetric(metricName(metrics, "start-time-ms", Map.of("client-id", clientId)), (Gauge<Long>) (config, now) -> appInfo.getStartTimeMs());
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!metrics.config().tags().containsKey("client-id"))
may need to include clientId != null
All Kafka component register AppInfo metrics to track the application
start time or commit-id etc. These metrics are useful for monitoring and
debugging. However, the AppInfo doesn't provide client-id, which is an
important information for custom metrics reporter.
The AppInfoParser class registers a JMX MBean with the provided
client-id, but when it adds metrics to the Metrics registry, the
client-id is not included. This KIP aims to add the client-id as a tag.