-
Notifications
You must be signed in to change notification settings - Fork 14.7k
KAFKA-19684: Move Gauge#value to MetricValueProvider #20543
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
For binary compatibility issues, the following tests were performed:
![]()
Compile using the code in the current trunk branch and version 1.0.1. Then run using the code that includes the changes in this PR. |
Hello, @ijuma @rajinisivaram @chia7712 , when have time, please review this PR , thanks ! |
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.
@majialoong thanks for this patch!
return ((Gauge<?>) metricValueProvider).value(config, now); | ||
else | ||
throw new IllegalStateException("Not a valid metric: " + this.metricValueProvider.getClass()); | ||
return metricValueProvider.value(config, now); |
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.
Can we update the docs so they match these changes?
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.
Thanks for the reminder, I will update this doc.
T value(MetricConfig config, long now); | ||
|
||
} | ||
public interface Gauge<T> extends MetricValueProvider<T> { } |
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.
The Gauge
type seems redundant at this stage. Would it be worth filing a KIP to deprecate it?
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.
Keeping the Gauge type can improves code readability. Readers immediately understand that the monitoring metric represents an instantaneous value, and it can also be equated with the concept of Gauge in mainstream monitoring systems. Directly using the MetricValueProvider may be a bit too abstract.
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.
Another follow-up (KIP) could be removing addMetric(MetricName metricName, Measurable measurable)
, which would let us drop the explicit type from the lambda
before
metrics.addMetric(metricName(metrics, "version", Map.of()), (Gauge<String>) (config, now) -> appInfo.getVersion());
after
metrics.addMetric(metricName(metrics, "version", Map.of()), (config, now) -> appInfo.getVersion());
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.
This is a good suggestion, I've created a JIRA to track this issue:https://issues.apache.org/jira/browse/KAFKA-19729
Thanks for the PR. Regarding compatibility, you'd want to test both binary and source compatibility. |
@ijuma Thank you for your comments. In the above conversation, I listed two methods for testing binary compatibility. If there are any scenarios I've overlooked, please comment and add them. I'll test and verify them promptly. I will also conduct some further testing and verification and add to this in subsequent conversations. |
KIP-188 introduced MetricValueProvider, adding Measurable and Gauge as its sub interfaces. However, this left a legacy issue: move the value method from Gauge to the super interface, MetricValueProvider.
This PR moves the value method from Gauge to MetricValueProvider and provides a default implementation in Measurable. This unifies the methods used by Gauge and Measurable to obtain monitoring values.