-
Notifications
You must be signed in to change notification settings - Fork 224
Add AggregatedMetrics to support multiple Metrics implementations #2917
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
Add AggregatedMetrics to support multiple Metrics implementations #2917
Conversation
f36afa7
to
2528836
Compare
2528836
to
e80267d
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.
Pull Request Overview
This PR introduces an AggregatedMetrics
class that implements the composite pattern to aggregate multiple Metrics
implementations, allowing simultaneous collection of metrics data by different monitoring systems.
Key changes:
- New
AggregatedMetrics
class that delegates calls to multiple metrics instances - Special handling for
timeControllerExecution()
which only delegates to the first metrics instance - Comprehensive test suite with 100% coverage of the new functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
AggregatedMetrics.java | Core implementation of the composite metrics aggregator with delegation logic |
AggregatedMetricsTest.java | Complete test suite covering constructor validation, method delegation, and special cases |
pom.xml | Added mockito-core test dependency for unit testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...pport/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/AggregatedMetrics.java
Outdated
Show resolved
Hide resolved
...t/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AggregatedMetricsTest.java
Outdated
Show resolved
Hide resolved
The new class should probably go into the same package as the |
* All method calls are delegated to each metrics instance in the list in the order they were | ||
* provided to the constructor. | ||
* | ||
* <p><strong>Important:</strong> The {@link #timeControllerExecution(ControllerExecution)} method |
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.
Good point. I forgot about that when I suggested this design! 😓
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.
LGTM
Hi @Donnerbart loogks, great, would be better if you could target |
e80267d
to
255ef8a
Compare
425634c
to
3e80997
Compare
…tions Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
3e80997
to
ac8e46e
Compare
Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
|
Partially fixes #2884
Alternative to #2912
See #2912 (comment)