[GitHub] [sling-org-apache-sling-event] sagarmiglani commented on a diff in pull request #31: Fix infinite recursion issue: SLING-11918

["sagarmiglani (via GitHub)" <gi...@apache.org>]

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-unsubscribe@sling.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org