xiangfu0 commented on code in PR #19872:
URL: https://github.com/apache/pinot/pull/19872#discussion_r4289882078
##########
pinot-plugins/pinot-metrics/pinot-dropwizard/src/main/java/org/apache/pinot/plugin/metrics/dropwizard/DropwizardMetricsRegistry.java:
##########
@@ -69,8 +69,7 @@ public PinotCounter newCounter(PinotMetricName name) {
@Override
public PinotTimer newTimer(PinotMetricName name, TimeUnit durationUnit,
TimeUnit rateUnit) {
- Timer timer = _metricRegistry.timer(name.getMetricName().toString(),
- () -> new Timer(new SlidingTimeWindowArrayReservoir(15,
TimeUnit.MINUTES)));
+ Timer timer = _metricRegistry.timer(name.getMetricName().toString());
Review Comment:
Nit: the timer is now switched to the bounded default reservoir, but
`newHistogram` just below still uses `SlidingTimeWindowArrayReservoir(15,
TimeUnit.MINUTES)`, so it keeps the same unbounded-growth shape the timer fix
removed. In practice nothing in the main codebase calls `newHistogram` today,
so this is only a consistency/future-proofing note rather than a real memory
risk — worth aligning it with the timer while you're here, or leaving a comment
on why it's intentionally left as-is.
##########
docker/images/pinot/etc/jmx_prometheus_javaagent/configs-dropwizard/broker.yml:
##########
@@ -0,0 +1,45 @@
+rules:
+# Dropwizard exposes Number as an alias of Value for numeric gauges.
Review Comment:
This `name: ""` rule to drop the Dropwizard `Number` alias is a nice touch,
but the enabled tests only assert the `Value` series is present — they never
assert `Number` is absent, so the suppression itself isn't covered by CI. Could
you confirm from an actual scrape (under jmx_exporter 1.7.0) that no
`pinot_..._Number` series leaks through? If it ever stops suppressing, the
failure is a silent duplicate series rather than a test failure.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
