sagarmiglani commented on code in PR #31:
URL:
https://github.com/apache/sling-org-apache-sling-event/pull/31#discussion_r1246307588
##########
src/main/java/org/apache/sling/event/impl/jobs/stats/GaugeSupport.java:
##########
@@ -150,8 +154,10 @@ private void registerWithSuffix(String suffix, int count,
Gauge<Long> value) {
metricRegistry.register(metricName, value);
Review Comment:
In my opinion, if we are aware that the metricRegistry throws an
`IllegalArgumentException (IAE)` when a name already exists, wouldn't it be
better if we use gaugeMetricNames to skip the already existing names and avoid
catching `IAE`?
Furthermore, instead of relying on a count, wouldn't it be better to utilize
a randomly generated key to minimize the probability of collisions?
While I have limited knowledge about this module, I believe, by limiting the
count, we are restricting the number of metrics with the same name which was
not the case before.
In my view, using a random key would have prevented the potential situation
of an infinite loop. WDYT?
--
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: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
