dlmarion commented on code in PR #5021:
URL: https://github.com/apache/accumulo/pull/5021#discussion_r1820622084
##########
server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsInfoImpl.java:
##########
@@ -251,23 +206,42 @@ public DistributionStatisticConfig configure(Meter.Id id,
String userRegistryFactories =
context.getConfiguration().get(Property.GENERAL_MICROMETER_FACTORY);
+ List<MeterRegistry> registries = new ArrayList<>();
+
for (String factoryName : getTrimmedStrings(userRegistryFactories)) {
try {
MeterRegistry registry = getRegistryFromFactory(factoryName,
context);
registry.config().commonTags(commonTags.values());
registry.config().meterFilter(replicationFilter);
- addRegistry(registry);
+ registries.add(registry);
} catch (ReflectiveOperationException ex) {
LOG.warn("Could not load registry {}", factoryName, ex);
}
}
- pendingRegistries.forEach(registry -> composite.add(registry));
+ if (registries.size() == 1) {
+ meterRegistry = registries.get(0);
+ } else {
+ var composite = new CompositeMeterRegistry();
+ composite.config().commonTags(commonTags.values());
+ registries.forEach(composite::add);
+ meterRegistry = composite;
Review Comment:
I noticed something interesting while working on #4993, but I have not dug
into it further.
[Metrics.globalRegistry](https://github.com/micrometer-metrics/micrometer/blob/main/micrometer-core/src/main/java/io/micrometer/core/instrument/Metrics.java#L36)
itself is a CompositeMeterRegistry. Our code ends up creating the following
structure:
```
-- CompositeMeterRegistry (Metrics.globalRegistry)
-- CompositeMeterRegistry (the one we create above)
-- LoggingMeterRegistry
-- Other user supplied MeterRegistry's
```
I don't think we need to create the second level CompositeMeterRegistry, I
think we can just call `MeterRegistry.add` for each user supplied
MeterRegistry. It's also the case that Meters are getting bound to registries
at different places in the hierarchy. For example, #5006 shows that ThreadPool
Meters are being bound to the top level global registry.
Finally, I think that the Property description changes that I made in #4993
are incorrect, but agagin, I have not tested this yet. I think that the current
code disables the reporting of the Metrics, but not the collection of the
Metrics in the Accumulo code.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
