feat: Introduce metric validation for otter tests #19260

timo0 · 2025-05-18T13:25:10Z

This change introduces:

NetworkMetrics a singleton that collects all NodeMetric instances
NodeMetric lightweight in-memory Metrics implementation that instruments all added Metric with TestMetricInvocationHandler to record the history of the metrics
assertThat validations for metric validation

Currently only assertThat().neverExceeds() is implemented to show how the system works. More to follow.

Fix #18638 This change introduces: * `NetworkMetrics` a singleton that collects all `NodeMetric` instances * `NodeMetric` lightweight in-memory `Metrics` implementation that instruments all added `Metric` with `TestMetricInvocationHandler` to record the history of the metrics * `assertThat` validations for metric validation Currently only `assertThat().neverExceeds()` is implemented to show how the system works. More to follow. Signed-off-by: Timo Brandstätter <[email protected]>

codecov · 2025-05-18T13:34:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

@@            Coverage Diff            @@
##               main   #19260   +/-   ##
=========================================
  Coverage     69.35%   69.35%           
  Complexity    23268    23268           
=========================================
  Files          2645     2645           
  Lines        100710   100710           
  Branches      10409    10409           
=========================================
  Hits          69846    69846           
  Misses        26902    26902           
  Partials       3962     3962

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codacy-production · 2025-05-18T13:34:23Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.00% (target: -1.00%)	✅ ∅

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`a24192e`)	100660	73812	73.33%
Head commit (`78198fc`)	100660 (+0)	73812 (+0)	73.33% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#19260)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

netopyr · 2025-05-19T10:06:51Z

...sts/src/testFixtures/java/org/hiero/otter/fixtures/assertions/MultipleNodeMetricsAssert.java

+     * @param threshold the threshold for the metric
+     * @return the {@link MultipleNodeMetricsResults} instance
+     */
+    public MultipleNodeMetricsAssert neverExceeds(final double threshold) {


I am unsure if we should introduce new names or stick with typical AssertJ names (e.g., isLessThan()). I see the advantages of both.

netopyr · 2025-05-19T10:10:36Z

...tests/src/testFixtures/java/org/hiero/otter/fixtures/assertions/SingleNodeMetricsAssert.java

+                    .map(SingleNodeMetricsAssert::toDouble)
+                    .filter(v -> v > threshold)
+                    .toList();
+            failWithMessage(


wrongValues can potentially be very long. I wonder if there is a better way to report this. Maybe the maximum would be sufficient?

We should also report the timestamp when the limit was exceeded.

netopyr · 2025-05-19T10:12:54Z

...sensus-otter-tests/src/testFixtures/java/org/hiero/otter/fixtures/metric/NetworkMetrics.java

+ */
+public final class NetworkMetrics {
+
+    private static final NetworkMetrics INSTANCE = new NetworkMetrics();


Can we avoid introducing a Singleton?

Yes removed

netopyr · 2025-05-19T10:16:02Z

...consensus-otter-tests/src/testFixtures/java/org/hiero/otter/fixtures/metric/NodeMetrics.java

+        if (started) {
+            return;
+        }
+        started = true;


This is not thread-safe. Not sure if it is intended.

netopyr · 2025-05-19T10:19:28Z

...consensus-otter-tests/src/testFixtures/java/org/hiero/otter/fixtures/metric/NodeMetrics.java

+        final Thread updaterThread = new Thread(
+                () -> {
+                    while (started) {
+                        final Instant start = Instant.now();


We should not use Instant.now() but use the FakeTime instead.

On second thought. If this implementation is only used in Turtle tests, we should make NodeMetrics a TimeTickReceiver and call the updaters every time a tick is received. What do you think?

netopyr · 2025-05-19T10:21:23Z

...consensus-otter-tests/src/testFixtures/java/org/hiero/otter/fixtures/metric/NodeMetrics.java

+ * integration tests. It records the full history of every metric value so that assertions can be
+ * executed after a test run.
+ */
+public class NodeMetrics implements Metrics {


I think this implementation will only be used in tests running on Turtle. I suggest moving this package under the turtle package.

netopyr

I am afraid I have to refresh my memory about the Metrics implementation before I can finish the review.

# Conflicts: # platform-sdk/consensus-otter-tests/src/test/java/org/hiero/otter/test/HappyPathTest.java

Signed-off-by: Timo Brandstätter <[email protected]>

lpetrovic05 · 2025-05-19T13:40:46Z

platform-sdk/consensus-otter-tests/src/test/java/org/hiero/otter/test/HappyPathTest.java

        assertThat(logResults).noMessageWithLevelHigherThan(Level.INFO);

        assertThat(network.getStatusProgression())
                .hasSteps(target(ACTIVE).requiringInterim(REPLAYING_EVENTS, OBSERVING, CHECKING));
+
+        assertThat(network.getMetricsResultsFor("platform", "bytes_per_trans").ignoring(firstNode))
+                .neverExceeds(444);


can we make 444 a constant?

the strings should also be constants I think

I feel like we should introduce a class like MetricId that has category and name. and use that for creating metrics as well as when we are asserting them

timo0 added this to the v0.63 milestone May 18, 2025

timo0 self-assigned this May 18, 2025

timo0 requested review from a team as code owners May 18, 2025 13:25

timo0 requested a review from netopyr May 18, 2025 14:39

netopyr reviewed May 19, 2025

View reviewed changes

timo0 added 2 commits May 19, 2025 14:06

Merge branch 'main' into 18638-add-metrics-validation

7374dcb

# Conflicts: # platform-sdk/consensus-otter-tests/src/test/java/org/hiero/otter/test/HappyPathTest.java

Remove NetworkMetrics, moved NodeMetrics to turtle package

c110515

Signed-off-by: Timo Brandstätter <[email protected]>

lpetrovic05 reviewed May 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce metric validation for otter tests #19260

feat: Introduce metric validation for otter tests #19260

timo0 commented May 18, 2025

codecov bot commented May 18, 2025 •

edited

Loading

codacy-production bot commented May 18, 2025

netopyr May 19, 2025

netopyr May 19, 2025

netopyr May 19, 2025

timo0 May 19, 2025

netopyr May 19, 2025

netopyr May 19, 2025

netopyr May 19, 2025

netopyr May 19, 2025

netopyr left a comment

lpetrovic05 May 19, 2025

lpetrovic05 May 19, 2025

lpetrovic05 May 19, 2025

feat: Introduce metric validation for otter tests #19260

Are you sure you want to change the base?

feat: Introduce metric validation for otter tests #19260

Conversation

timo0 commented May 18, 2025

codecov bot commented May 18, 2025 • edited Loading

Codecov Report

codacy-production bot commented May 18, 2025

Coverage summary from Codacy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netopyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 18, 2025 •

edited

Loading