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

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

elek commented on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-862213865


   > When creating the RatisMetricRegistry, it is expecting that there should be some reporters registered already. If not, this warning message is thrown. In JVMMetrics, we add consoleReporter and JmxReporter before creating the MetricsRegistry. But in RatisMetrics, this is not done.
   
   Thanks to explain it. Now I can reproduce the problem with Ozone Freon.
   
   My problem is that the warning looks to emphases a real problem.
   
   If we add reporter after creating new `metricRegistry`, all the metrics will be silently ignored (which is dangerous):
   
   ```java
       final MetricRegistriesImpl metricRegistries = new MetricRegistriesImpl();
       final RatisMetricRegistry ratisMetricRegistry = metricRegistries.create(new MetricRegistryInfo("prefix", "name", "component", "desc"));
       ratisMetricRegistry.counter("test1").inc();
       ratisMetricRegistry.counter("test2").inc();
       metricRegistries.enableConsoleReporter(TimeDuration.ONE_SECOND);
       Thread.sleep(10000L);
   ```
   
   Here the warning is printed out, and we must have this warning as this is a real problem:
   
   ```
   11:35:22.916 [main] WARN org.apache.ratis.metrics.impl.MetricRegistriesImpl - New reporters are added after registries were created. Some metric will be missing from the reporter. Please add reporter before adding any new registry
   ```
   
   We can fix it in Ozone side by adding the JMX reporter in freon (looks to be missing) but it also means that all the user of OzoneClient should have this line to avoid the warning:
   
   ```
   MetricRegistries.global().enableJmxReporter();
   ```
   
   What do you think about this approach? -->
   
   Instead of printing out warning, we can register JMX reporter by default.
   
   ```
     @Override
     public RatisMetricRegistry create(MetricRegistryInfo info) {
       return registries.put(info, () -> {
         if (reporterRegistrations.isEmpty()) {
           enableJmxReporter();
         }
         RatisMetricRegistry registry = factory.create(info);
         reporterRegistrations.forEach(reg -> reg.accept(registry));
         return registry;
       });
     }
   ```
   
   
   But now we have another problem. It's possible that somebody adds a reporter AFTER the first metric registry is created. In this case, the first metric registry will be missing from the second reporter:
   
   ```java
     @Test
     public void lateInit() throws InterruptedException {
       final MetricRegistriesImpl metricRegistries = new MetricRegistriesImpl();
       //implicitly adds jmx reporter
       final RatisMetricRegistry ratisMetricRegistry = metricRegistries.create(new MetricRegistryInfo("prefix", "name", "component", "desc"));
       ratisMetricRegistry.counter("test1").inc();
       ratisMetricRegistry.counter("test2").inc();
      
      //test1 and test2 won't be added to this reporter
       metricRegistries.enableConsoleReporter(TimeDuration.ONE_SECOND);
       final RatisMetricRegistry ratisMetricRegistry2 = metricRegistries.create(new MetricRegistryInfo("prefix2", "name2", "component2", "desc"));
       ratisMetricRegistry2.counter("test3").inc();
       Thread.sleep(10000L);
     }
   ```
   
   In this code the console reporter will print out only test3 metric test1/test2 are slienlty ignored.
   
   But we can put back the warning to here:
   
   ```java
     @Override
     public void addReporterRegistration(Consumer<RatisMetricRegistry> reporterRegistration,
         Consumer<RatisMetricRegistry> stopReporter) {
       if (registries.size() > 0) {
         LOG.warn("New reporters are added after registries were created. Some metric will be missing from the reporter. "
             + "Please add reporter before adding any new registry");
       }
       this.reporterRegistrations.add(reporterRegistration);
       this.stopReporters.add(stopReporter);
     }
   ```
   
   With this approach we won't see the warning by default (as a default reporter will be provided) but in the rare case of an out-of-rder metric registration / reporter registartion a warning can be seen....


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