[GitHub] [geode] kohlmu-pivotal commented on issue #3153: GEODE-6295: Add Micrometer-based metrics collector

["kohlmu-pivotal (GitHub)" <gi...@apache.org>]

So.. in a nutshell, what @galen-pivotal and I spoke about:

- The notion of a `MetricsCollector` as it stands and is currently used, makes no sense. @upthewaterspout has already pointed out that it abstracts the usage of  the micrometer.io `CompositeMeterRegistry` and at the same time it does not. To me, the simplest approach is to pass in the `MeterRegistry` into the `InternalDistributedSystem` from "outside" (maybe as @upthewaterspout has indicated the `CacheFactory`.
- IMO, some thought has to be put into the current management layer, to enable the creation, amendment of a constructed `CompositeMeterRegistry`. If there would be a `MetricsConfigurationService`, it could potentially hold onto all configurations for downstream registries. With the correct amount of design/abstraction and implementation, this `*ConfigurationService` could "discovered" using the extensions framework. With a concrete `MetricsConfigurationService` all configurations can be distributed/replicated to other servers with the cluster. (also include persistence)
- The adding and removing of `MeterRegistry` from the `CompositeMeterRegistry` is a great feature! e.g Adding or removing a `JMXRegistry` at runtime is nice. BUT in lieu of a properly defined lifecycle and management service, I would not make that the only reason to have a `MetricController` interface.

[ Full content available at: https://github.com/apache/geode/pull/3153 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org