You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2021/06/10 17:25:48 UTC
[GitHub] [ratis] hanishakoneru opened a new pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
hanishakoneru opened a new pull request #481:
URL: https://github.com/apache/ratis/pull/481
## What changes were proposed in this pull request?
MetricsRegistriesImpl throws a warning when creating RatisMetricsRegistry without registering any reporters beforehand.
```
@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.");
StackTraceElement[] stackTrace = Thread.currentThread().getStackTrace();
for(StackTraceElement element : stackTrace) {
LOG.warn(" " + element.getClassName() + " - " + element.getMethodName() + " - " +
element.getLineNumber());
}
}
RatisMetricRegistry registry = factory.create(info);
reporterRegistrations.forEach(reg -> reg.accept(registry));
return registry;
});
}
```
This leads to a lot of noise in the logs as every ratis client logs this message.
```
21/06/02 12:37:35 INFO metrics.RatisMetrics: Creating Metrics Registry : ratis.client_message_metrics.client-CBAF50A69A13->24ecdbe1-e8c3-4211-94fc-31429681f184
21/06/02 12:37:35 WARN impl.MetricRegistriesImpl: First MetricRegistry has been created without registering reporters. You may need to call MetricRegistries.global().addReporterRegistration(...) before.
21/06/02 12:37:40 INFO metrics.RatisMetrics: Creating Metrics Registry : ratis.client_message_metrics.client-1A65382B9313->082aff6d-b8b6-435b-993e-1b3b37b9770b
21/06/02 12:37:40 WARN impl.MetricRegistriesImpl: First MetricRegistry has been created without registering reporters. You may need to call MetricRegistries.global().addReporterRegistration(...) before.
21/06/02 12:37:44 INFO metrics.RatisMetrics: Creating Metrics Registry : ratis.client_message_metrics.client-530530878B2F->06c72e97-a5ea-4474-b73c-5642d30ee3cf
21/06/02 12:37:44 WARN impl.MetricRegistriesImpl: First MetricRegistry has been created without registering reporters. You may need to call MetricRegistries.global().addReporterRegistration(...) before.
21/06/02 12:37:52 INFO metrics.RatisMetrics: Creating Metrics Registry : ratis.client_message_metrics.client-B39FB0CD2151->24ecdbe1-e8c3-4211-94fc-31429681f184
21/06/02 12:37:52 WARN impl.MetricRegistriesImpl: First MetricRegistry has been created without registering reporters. You may need to call MetricRegistries.global().addReporterRegistration(...) before.
21/06/02 12:37:57 INFO metrics.RatisMetrics: Creating Metrics Registry : ratis.client_message_metrics.client-12092E53EF9C->082aff6d-b8b6-435b-993e-1b3b37b9770b
21/06/02 12:37:57 WARN impl.MetricRegistriesImpl: First MetricRegistry has been created without registering reporters. You may need to call MetricRegistries.global().addReporterRegistration(...) before.
21/06/02 12:38:00 INFO metrics.RatisMetrics: Creating Metrics Registry : ratis.client_message_metrics.client-89C870BA12C6->d5196d6f-753d-4113-87ef-2bfbba66c2af
21/06/02 12:38:00 WARN impl.MetricRegistriesImpl: First MetricRegistry has been created without registering reporters. You may need to call MetricRegistries.global().addReporterRegistration(...) before.
21/06/02 12:38:03 INFO metrics.RatisMetrics: Creating Metrics Registry : ratis.client_message_metrics.client-A9412DBA8690->06c72e97-a5ea-4474-b73c-5642d30ee3cf
21/06/02 12:38:03 WARN impl.MetricRegistriesImpl: First MetricRegistry has been created without registering reporters. You may need to call MetricRegistries.global().addReporterRegistration(...) before.
21/06/02 12:38:25 INFO metrics.RatisMetrics: Creating Metrics Registry : ratis.client_message_metrics.client-49743027A7AD->d5196d6f-753d-4113-87ef-2bfbba66c2af
21/06/02 12:38:25 WARN impl.MetricRegistriesImpl: First MetricRegistry has been created without registering reporters. You may need to call MetricRegistries.global().addReporterRegistration(...) before.
```
We can remove the warning message or at the least change it to debug level.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1381
## How was this patch tested?
Tested on an Ozone deployment.
--
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
[GitHub] [ratis] hanishakoneru commented on a change in pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by 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
[GitHub] [ratis] hanishakoneru commented on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-860896794
> I am fine with removing the warning, but can you please give more information how can the problem be reproduced? As far as I remember I also saw it earlier, but today I just tested with ozone sh and ozonen freon and couldn't reproduce.
>
> Do we have any racecondition somewhere between adding reporters + registering the first metric? Or is the metric registration missing from somewhere?
I was able to see this just be running a {{ozone sh key put}} command. I think we log this warning once for each BlockOutputStream. When creating a large key (multiple blocks), this log message is seen multiple times.
--
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
[GitHub] [ratis] hanishakoneru edited a comment on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
hanishakoneru edited a comment on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-860896794
> I am fine with removing the warning, but can you please give more information how can the problem be reproduced? As far as I remember I also saw it earlier, but today I just tested with ozone sh and ozonen freon and couldn't reproduce.
I was able to see this just be running a {{ozone sh key put}} command. I think we log this warning once for each BlockOutputStream. When creating a large key (multiple blocks), this log message is seen multiple times.
> Do we have any racecondition somewhere between adding reporters + registering the first metric? Or is the metric registration missing from somewhere?
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.
--
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
[GitHub] [ratis] elek edited a comment on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [ratis] lokeshj1703 commented on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-860380597
@hanishakoneru TestNettyDataStreamStarTopologyWithGrpcCluster is related to Ratis streaming.
--
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
[GitHub] [ratis] elek commented on a change in pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #481:
URL: https://github.com/apache/ratis/pull/481#discussion_r650698669
##########
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:
I understand the motivation but for the sake of the simplicity I would vote to always use LOG.debug level if `LOG.isDebugEnabled` (and drop the specific parameter). I think the information here is not so important to have an explicit flag for it. If somebody were interested, debug log level always can be adjusted.
(But I am fine with either approach)
##########
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) {
+ LOG.debug("Creating Metrics Registry : {}", info.getName());
Review comment:
We have INFO log in the same file:
```
LOG.info("Unregistering Metrics Registry : {}", info.getName());
```
We can also modify the debug level here...
##########
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:
bq. 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.
I am fine with removing the warning, but can you please give more information how can the problem be reproduced? As far as I remember I also saw it earlier, but today I just tested with `ozone sh` and `ozonen freon` and couldn't reproduce.
Do we have any racecondition somewhere between adding reporters + registering the first metric? Or is the metric registration missing from somewhere?
--
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
[GitHub] [ratis] adoroszlai commented on a change in pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #481:
URL: https://github.com/apache/ratis/pull/481#discussion_r649927041
##########
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:
Why not `LOG.isDebugEnabled()`?
##########
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:
Doesn't the warning point to a real problem? Are client message metrics being reported?
--
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
[GitHub] [ratis] hanishakoneru commented on pull request #481: RATIS-1381. Add default Reporter if there are no reporters when creating Metrics Registry
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-865364347
Thanks @elek for the review and suggestions. I will merge this PR shortly.
--
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
[GitHub] [ratis] hanishakoneru commented on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-861706161
> TestNettyDataStreamStarTopologyWithGrpcCluster is related to Ratis streaming.
@lokeshj1703, sorry but does this mean it is related or unstable?
--
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
[GitHub] [ratis] hanishakoneru merged pull request #481: RATIS-1381. Add default Reporter if there are no reporters when creating Metrics Registry
Posted by GitBox <gi...@apache.org>.
hanishakoneru merged pull request #481:
URL: https://github.com/apache/ratis/pull/481
--
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
[GitHub] [ratis] hanishakoneru commented on a change in pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #481:
URL: https://github.com/apache/ratis/pull/481#discussion_r651168304
##########
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) {
+ LOG.debug("Creating Metrics Registry : {}", info.getName());
Review comment:
Done.
--
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
[GitHub] [ratis] hanishakoneru merged pull request #481: RATIS-1381. Add default Reporter if there are no reporters when creating Metrics Registry
Posted by GitBox <gi...@apache.org>.
hanishakoneru merged pull request #481:
URL: https://github.com/apache/ratis/pull/481
--
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
[GitHub] [ratis] elek edited a comment on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
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:
```java
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.
```java
@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:
users@infra.apache.org
[GitHub] [ratis] hanishakoneru commented on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-859258160
CI run is failing at unit test `TestNettyDataStreamStarTopologyWithGrpcCluster`. It passes locally for me. Is this test known to fail intermittently?
cc. @bshashikant, @elek, @lokeshj1703
--
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
[GitHub] [ratis] elek commented on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by 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
[GitHub] [ratis] adoroszlai commented on a change in pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #481:
URL: https://github.com/apache/ratis/pull/481#discussion_r650723055
##########
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:
I don't think it can be fixed on the server side. Reporters are registered with `MetricRegistries.global()`, which is a singleton limited to the Java process (i.e. here the client).
--
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
[GitHub] [ratis] hanishakoneru commented on a change in pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #481:
URL: https://github.com/apache/ratis/pull/481#discussion_r651167962
##########
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:
bq. I think the information here is not so important to have an explicit flag for it. If somebody were interested, debug log level always can be adjusted.
Agreed. I will remove the extra flag and keep the LOG at debug level.
--
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
[GitHub] [ratis] hanishakoneru commented on pull request #481: RATIS-1381. Add default Reporter if there are no reporters when creating Metrics Registry
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-865364347
Thanks @elek for the review and suggestions. I will merge this PR shortly.
--
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
[GitHub] [ratis] lokeshj1703 commented on pull request #481: RATIS-1381. Remove MetricsRegistries warning for creating without adding reporters
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #481:
URL: https://github.com/apache/ratis/pull/481#issuecomment-862103431
@hanishakoneru It should not be related. I am not sure about the stability part.
--
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