[GitHub] [geode] demery-pivotal opened pull request #3153: GEODE-6295: Add Micrometer-based metrics collector

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

This PR adds a Micrometer-based metrics system to Geode.

On startup, Geode configures a "primary" meter registry. Developers use
the primary registry to create meters (gauges, counters, timers, and
others) to instrument Geode and client code.

Internally, the meter registry is a "composite" registry, which allows
attaching any number of "downstream" registries. Each downstream
registry will typically publish the metrics to an external monitoring
system, or provide an endpoint where an external monitoring system can
"scrape" the metrics.

In Geode, when a meter is added or removed from the primary registry,
the primary registry adds or removes a corresponding meter in each
downstream registry.  When a meter in the primary registry is updated
(e.g. incremented), the primary registry forwards each operation to the
corresponding meter in each downstream registry.

A MetricsCollector provides access to the primary registry, and includes
methods for adding and removing downstream registries.

The MetricsCollector itself is currently available via a method on
InternalDistributedSystem.  Eventually we plan to  make the
MetricsCollector available through DistributedSystem, or some other
non-internal class or interface.

The key components of this change are:
- MetricsCollector (interface) allows access to the primary registry,
  and allows adding and removing downstream registries.
- CompositeMetricsCollector is the concrete implementation of
  MetricsCollector, and creates the internal composite registry.
- InternalDistributedSystem creates the MetricsCollector and makes it
  available via a new getMetricsCollector() method.
- The Micrometer system for instrumenting and maintaining metrics (added
  to Geode as a dependency). See http://micrometer.io/ for details.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] Have you written or updated unit tests to verify your changes?

- [X] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


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

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

["upthewaterspout (GitHub)" <gi...@apache.org>]

One other concern with this API - users can only add MeterRegistries after a distributed system is already created. But we will likely end up creating an updating a number of meters when the distributed system is created. If a user can't add MeterRegistries until after the distributed system creation, they will miss events.

Maybe the user's downstream MeterRegistries should be passed in to the CacheFactory?

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

[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

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

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

@galen-pivotal Yeah, those two kinds of uses (instrumenting and publishing) are distinct, so the methods for them don't belong in a single interface.

So we may as well expose `getMeterRegistry()` directly on whatever class ends up owning the registry.

Cache may indeed be the right place for the registry.

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

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

["upthewaterspout (GitHub)" <gi...@apache.org>]

Hi Dale,

That helps. I missed the part where you explicitly said you were going to make this external at a later point in time. So the intent is to keep this feature hidden on develop until it is more fully baked - that makes sense.

Regarding MetricsCollector vs. MeterRegistry - I see. So you are not trying to hide micrometer from the user, but trying to hide CompositeMeterRegistry.



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

[GitHub] [geode] galen-pivotal commented on pull request #3153: GEODE-6295: Add Micrometer-based metrics collector

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

It looks like this non-static is in the middle of the "static methods" section in this class. Maybe it should be moved lower?

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

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

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

Hi Dan,

This PR does not add meters. We have identified a small number of meters add to geode-core very soon. The goal of this PR is to get the system in place to do that.

The API is temporarily internal in case we want to tweak it before moving it to a non-internal place. Specifically, we intend to make the MeterCollector interface and the method to obtain one externally available. We considered putting getMetricsCollector() in DistributedSystem and marking it experimental, but one of the questions yet to be decided is whether DistributedSystem is the right place to expose the metrics system.

We intend to expose the CompositeMeterRegistry only as a MeterRegistry, to allow us to substitute other implementations later if we choose. The MeterRegistry (abstract class) is all that’s needed to create meters.

The CompositeMetricsCollector allows us to move the configuration of the CompositeMeterRegistry out of InternalDistributedSystem, which doesn’t need yet more responsibilities. (Though, uh, in the code I submitted, the configuration is done in InternalDistributedSystem. That should be moved into CompositeMetricsCollector.)

MetricsCollector  (the primary hub for accessing the metrics system) is an interface in order to allow us to substitute other implementations later, and make it easier to test things that don’t need the concrete implementation.

Thank you for your review. Have I left you more puzzled? Less puzzled? Differently puzzled?

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

[GitHub] [geode] demery-pivotal commented on pull request #3153: GEODE-6295: Add Micrometer-based metrics collector

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

I'm thinking of moving the tag-adding stuff into the `CompositeMetricsCollector`. If we do that, what's left in `configureMeterRegistry()` might be small enough to inline. Otherwise, I'll move it out of the static methods section. And rename it `configureMetricsCollector()`.

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

[GitHub] [geode] demery-pivotal closed pull request #3153: GEODE-6295: Add Micrometer-based metrics collector

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

[ pull request closed by demery-pivotal ]

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

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

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

> * 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`.

By "the" do you mean that the user would pass in the _one-and-only_ registry for the system to use? Or a downstream registry to be added to the internally constructed/configured composite? Or something else?

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

Yes, the life cycle and configuration are the big puzzles that make us want to keep this internal, but to get it into develop so we can get some experience.

Do you have any scenarios in mind that would help flesh out what the life cycle might look like?

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

`MetricsController` is doomed.

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

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

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

Also, he made the point that the best place for users to get a hold of the meter registry would probably be the Cache, since that's our best holds-the-entire-world object right now.

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

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

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

Yes. The intention is for the `MetricsCollector` interface to expose _everything_ needed by two kinds of users. Developers wanting to instrument code will use it to get Geode's meter registry, so they can add meters. Developers wanting to publish metrics will use it to add/remove downstream registries.

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

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

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

Superseded by https://github.com/apache/geode/pull/3180.

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

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

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

Superseded by https://github.com/apache/geode/pull/3180.

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

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

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

@upthewaterspout I'll take a look at injecting the downstream registry (and maybe multiple registries) via the cache factory.

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