[GitHub] [ratis] hanishakoneru commented on a change in pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters

[GitBox <gi...@apache.org>]

hanishakoneru commented on a change in pull request #481:
URL: https://github.com/apache/ratis/pull/481#discussion_r650066174



##########
File path: ratis-metrics/src/main/java/org/apache/ratis/metrics/impl/MetricRegistriesImpl.java
##########
@@ -61,11 +61,6 @@ public MetricRegistriesImpl(MetricRegistryFactory factory) {
   @Override
   public RatisMetricRegistry create(MetricRegistryInfo info) {
     return registries.put(info, () -> {
-      if (reporterRegistrations.isEmpty()) {
-        LOG.warn(
-            "First MetricRegistry has been created without registering reporters. You may need to call" +
-                " MetricRegistries.global().addReporterRegistration(...) before.");
-      }

Review comment:
       So without reported being added, I believe there would not be any metrics to report. But then, it should be fixed from the calling function. Currently, if we run a load on the system, we keep getting these warning messages from ratis clients. It does not serve the purpose to spam the client logs with this warning message, as it needs to fixed on server side.

##########
File path: ratis-metrics/src/main/java/org/apache/ratis/metrics/RatisMetrics.java
##########
@@ -31,9 +31,16 @@
   protected RatisMetricRegistry registry;
 
   protected static RatisMetricRegistry create(MetricRegistryInfo info) {
+    return create(info, true);
+  }
+
+  protected static RatisMetricRegistry create(MetricRegistryInfo info,
+      boolean shouldDebugLog) {
     Optional<RatisMetricRegistry> metricRegistry = MetricRegistries.global().get(info);
     return metricRegistry.orElseGet(() -> {
-      LOG.info("Creating Metrics Registry : {}", info.getName());
+      if (shouldDebugLog) {

Review comment:
       This mertics is used by different components. Currently we were seeing it create a lot of noise in client logs. Using this `shouldDebugLog` helps with not printing the message for client logs (even in Debug mode) but printing for other components such as LeaderElectionMetrics. 
   
   For the other components, we can keep it Info or Debug level. I am ok either way.

##########
File path: ratis-metrics/src/main/java/org/apache/ratis/metrics/impl/MetricRegistriesImpl.java
##########
@@ -61,11 +61,6 @@ public MetricRegistriesImpl(MetricRegistryFactory factory) {
   @Override
   public RatisMetricRegistry create(MetricRegistryInfo info) {
     return registries.put(info, () -> {
-      if (reporterRegistrations.isEmpty()) {
-        LOG.warn(
-            "First MetricRegistry has been created without registering reporters. You may need to call" +
-                " MetricRegistries.global().addReporterRegistration(...) before.");
-      }

Review comment:
       So without reporters being added, I believe there would not be any metrics to report. But then, it should be fixed from the calling function. Currently, if we run a load on the system, we keep getting these warning messages from ratis clients. It does not serve the purpose to spam the client logs with this warning message, as it needs to fixed on server side.




-- 
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.

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