elek edited a comment 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();
```
I have the following alternative suggestion (please let me know what do you
think).
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 silently ignored.
But we can put back warning to this new place:
```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 registration 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:
[email protected]
