-
Notifications
You must be signed in to change notification settings - Fork 682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GEODE-7171: Encapsulate metrics session #4203
GEODE-7171: Encapsulate metrics session #4203
Conversation
7c58ca9
to
d22bd66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is a significant improvement to our metrics session code. I added some comments inline, but also wanted to share my opinion about error handling here because it appears in many places in the code.
I am leaning towards logging and then re-throwing exceptions instead of just logging them (in most places). Here's my thinking:
- This will help users/developers find problems quickly with their registries/binders/publishing services because the re-thrown exceptions will cause the system to "fail hard" instead of printing something in a log file that may go unnoticed.
- I can't think of any specific exception that we expect to occur where it would be a major problem if we re-throw the exception.
- The only caveat is that we may want to catch/ignore exceptions from
AutoCloseable.close()
or wrap them in unchecked exceptions so that we don't have to propagatethrows Exception
to every method signature in the hierarchy.
I'm interested to hear @kirklund's thoughts on the error handling.
geode-core/src/main/java/org/apache/geode/metrics/MetricsPublishingService.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/metrics/internal/CompoundMeterBinder.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/metrics/internal/StandardMeterBinder.java
Show resolved
Hide resolved
...src/main/java/org/apache/geode/metrics/internal/InternalDistributedSystemMetricsService.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/DistributedSystem.java
Outdated
Show resolved
Hide resolved
...ationTest/java/org/apache/geode/distributed/internal/InternalDistributedSystemJUnitTest.java
Outdated
Show resolved
Hide resolved
geode-core/src/test/java/org/apache/geode/internal/cache/InternalCacheBuilderTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Providing my feedback as comments. Feel free to use or ignore any of these comments!
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/metrics/MetricsPublishingService.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/geode/metrics/internal/InternalDistributedSystemMetricsService.java
Show resolved
Hide resolved
...va/org/apache/geode/metrics/internal/InternalDistributedSystemMetricsServiceBuilderTest.java
Show resolved
Hide resolved
geode-junit/src/main/java/org/apache/geode/test/micrometer/AbstractMeterAssert.java
Outdated
Show resolved
Hide resolved
7fc560e
to
4dfed8a
Compare
Metrics session responsibilities are starting to appear in too many parts of Geode. This change encapsulates those responsibilities into a single class. Changes to core Geode classes: - GemFireCacheImpl no longer holds a meter registry or the set of "user" registries added by the cache builder. - InternalCacheBuilder no longer constructs objects on behalf of the metrics session. Instead, it gathers meter registries and other details into a MetricsService.Builder, which it passes to the InternalDistributedSystem.Builder. - InternalDistributedSystem no longer knows about client meter registries. Instead, it holds a MetricsService, which remembers its builder. During reconnect, the InternalDistributedSystem retrieves the builder from its metrics service, and uses the builder to build a similar metrics service in the reconnected system. New classes and interfaces (org.apache.geode.metrics.internal): - StandardMeterBinder - Binds standard meter binders to the meter registry. - close() closes the closeable binders, freeing their resources. - MetricsService interface - Extends MetricsSession. - Adds methods to start and stop the service, and to access the service's meter registry. - Adds a method that returns the builder that built the metrics service, so that reconnect can use the same builder to build the metrics service for the reconnected system. - InternalDistributedSystemMetricsService - Implements MetricsService. - Creates and retains a composite meter registry to manage meters. - Configures the composite composite registry: - Adds common tags that describe the system - Adds client meter registries to the composite. - Applies a StandardMeterBinder to add standard meters to the registry. - Discovers, starts, and stops metrics publishing services. - Adds and removes sub-registries as instructed by publishing services. - When stopped, stops each publishing service, closes all closeable meter binders, removes all sub-registries, and closes its composite meter registry. - MetricsService.Builder interface - InternalDistriburtedSystemMetricsService.Builder - Implements MetricsService.Builder - Accumulates and retains constructor parameters for the eventual InternalDistributedSystemMetricsService. - In particular, maintains a collection of meter registries added by the cache builder. It calls these 'persistent' registries, because they will persist across reconnects. Other changes: - Added MetricsSession parameter to MetricsPublishingService.stop(). Authored-by: Dale Emery <demery@pivotal.io>
4dfed8a
to
65d3417
Compare
This PR gathers metrics session responsibilities, which have become scattered across too many core Geode classes, and encapsulates them into a small set of closely related classes.
Changes to core Geode classes:
GemFireCacheImpl no longer holds a meter registry or the set of "user"
registries added by the cache builder.
InternalCacheBuilder no longer constructs objects on behalf of the
metrics session. Instead, it gathers meter registries and other
details into a MetricsService.Builder, which it passes to the
InternalDistributedSystem.Builder.
InternalDistributedSystem no longer knows about client meter
registries. Instead, it holds a MetricsService, which remembers its
builder. During reconnect, the InternalDistributedSystem retrieves
the builder from its metrics service, and uses the builder to build
a similar metrics service in the reconnected system.
New classes and interfaces (org.apache.geode.metrics.internal):
StandardMeterBinder
MetricsService interface
service's meter registry.
service, so that reconnect can use the same builder to build the
metrics service for the reconnected system.
InternalDistributedSystemMetricsService
registry.
services.
meter binders, removes all sub-registries, and closes its composite
meter registry.
MetricsService.Builder interface
InternalDistriburtedSystemMetricsService.Builder
InternalDistributedSystemMetricsService.
the cache builder. It calls these 'persistent' registries, because
they will persist across reconnects.
Authored-by: Dale Emery demery@pivotal.io
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:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop
)?Is your initial contribution a single, squashed commit?
Does
gradlew build
run cleanly?Have you written or updated unit tests to verify your changes?
[N/A] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?