KAFKA-8696: clean up Sum/Count/Total metrics#7057
Conversation
|
I noticed this while looking at the comments on #6979 I remembered these metrics being super confusing in the past, and it didn't seem too hard to just clean it up. What do you think? -John |
There was a problem hiding this comment.
I'm not generally a fan of building tall class hierarchies, but in this case, it seemed like the cleanest way to delegate to Sum, which has the advantage of code de-duplication, but more importantly makes the code more self-documenting... I.e., this metric is exactly the same as Sum, except that it always increments by 1.
There was a problem hiding this comment.
This metric was redundant with Sum (their implementations are exactly the same). I decided in favor of Sum because it's a top-level class.
Since this class is public and not internal, though, I figure we'd better just mark it as deprecated for now, although it's unused in our code base as of this PR.
There was a problem hiding this comment.
This metric had been added as an internal class in Streams, since it was only needed by Streams, but this actually seems like a liability...
Putting it here reduces the chance that someone would accidentally use Count, not realizing it is sampled, whereas this metric's presence would force you to consider which of the two options you want.
Also, if someone did want a non-sampled count in the future, they might just re-implement it here, if they didn't happen to find the one in Streams, leading us back into the situation I'm trying to fix here, where we have redundant metrics.
These two points seem to outweigh the advantage of keeping a class defined in the smallest scope in which it's needed.
There was a problem hiding this comment.
Since we're relocating packages, it's a good opportunity to try an pick a more semantic and consistent name... Not sure if I succeeded.
|
None of those test timeouts seem related... Retest this, please. |
|
Retest this, please. |
|
Would this need a KIP? |
|
retest this please |
|
@mjsax , yeah, I guess it would, since we've moving a metric from |
|
Created KIP-488: https://cwiki.apache.org/confluence/x/kkAyBw |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
Just one question about naming compared with KIP itself, otherwise lgtm.
bbejeck
left a comment
bbejeck
left a comment
There was a problem hiding this comment.
Overall this LGTM, just waiting for updates from KIP changes
vvcephei
changed the title
* Clean up one redundant and one misplaced metric * Clarify the relationship among these metrics to avoid future confusion
vvcephei
left a comment
vvcephei
left a comment
There was a problem hiding this comment.
Hi @bbejeck @mjsax @guozhangwang ,
I believe this is ready for reviews, as the KIP has just been accepted.
Still waiting on the build, so the jury is out on style/compilation/tests.
Thanks,
-John
| public Meter(TimeUnit unit, SampledStat rateStat, MetricName rateMetricName, MetricName totalMetricName) { | ||
| if (!(rateStat instanceof SampledTotal) && !(rateStat instanceof Count)) { | ||
| throw new IllegalArgumentException("Meter is supported only for SampledTotal and Count"); | ||
| if (!(rateStat instanceof WindowedSum)) { |
There was a problem hiding this comment.
note that WindowedCount is a subclass of WindowedSum, so we just need one condition to check both (and also to permit any of the deprecated classes, as they all also subclass WindowedSum).
|
All test failures seem transient. retest this please |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
LGTM except a few minor things. Wait for one green build.
| } | ||
|
|
||
| /** | ||
| * @deprecated since 2.4 Use {@link SampledTotal} instead. |
There was a problem hiding this comment.
The javadoc seems incorrect.
There was a problem hiding this comment.
Oops! I wonder how that happened... Thanks for catching it.
| for (Sensor sensor : Arrays.asList(parent, child)) { | ||
| sensor.add(metrics.metricName(sensor.name() + ".avg", "grp1"), new Avg()); | ||
| sensor.add(metrics.metricName(sensor.name() + ".count", "grp1"), new Count()); | ||
| sensor.add(metrics.metricName(sensor.name() + ".count", "grp1"), new WindowedCount()); |
There was a problem hiding this comment.
nit: also change the metric name?
There was a problem hiding this comment.
I don't know... I'm reluctant to expand the scope of this change to change registered metric names. I know these are just in tests, but still, it seems not necessary.
Do you think changing these metric names increases clarity, or were you just asking if we should consider changing them?
There was a problem hiding this comment.
I do not feel strong about it, just asking whether it's worth doing. Seems it's not worth doing after all.
There was a problem hiding this comment.
Ok. Thanks for bringing it up, but let's just leave it alone... It will be an opportunity for someone to use git blame later and curse my name.
| break; | ||
| case TOTAL: | ||
| sensor.add(metrics.metricName("test.metric.total", "total", tags), new Total()); | ||
| sensor.add(metrics.metricName("test.metric.total", "total", tags), new CumulativeSum()); |
There was a problem hiding this comment.
nit: ditto here about metric name.
|
retest this please |
|
Test failures are transient, merging to trunk now. |
|
cc @cadonna : the other PR can be rebased. |
|
Yay! Thanks, @guozhangwang . |
4 participants
Committer Checklist (excluded from commit message)