Skip to content
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

646: Provided wrapper for histogram #649

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gkatzioura
Copy link

@gkatzioura gkatzioura commented Mar 22, 2025

Purpose

Linked issue: close #646

Brief change log

Increased coverage for FlinkCounter, FlinkGauge, FlinkHistogram, FlinkHistogramStatistics, FlinkMeter.

Tests

FlinkCounterTest, FlinkHistogramStatisticsTest, FlinkHistogramTest, FlinkMeterTest, FlinkMetricsITCase

API and Format

Documentation

I initially went for the direction of increasing the coverage through FlinkMetricsITCase,
Yet could not find ways to invoke the creation of Counter or the methods missing coverage from the other metric classes.

I went to implement tests per metric.

Since those classes are wrapper classes (FlinkCounter, FlinkGauge FlinkHistogram FlinkMeter) I proceeded on testing their proxy behaviour.
I created WrapperMetricsTestSuite which lists all the public methods expected to be proxied and invokes those methods.
Onwards it checks if the underlying wrapped metric instance has been invoked for the corresponding method.

Another option was to test each class and verify invocations for every public method, for example:

        Counter counter = mock(Counter.class);
        FlinkCounter flinkCounter = new FlinkCounter(counter);
        flinkCounter.inc();
        verify(counter, times(1)).inc();
        flinkCounter.inc(1);
        verify(counter, times(1)).inc(1);

Can switch to this direction if requested.

The FlinkHistogramStatistics class was not used.
I proceeded to create an instances of it during the initialization of FlinkHistogram, thus wrapping the original statistics instance.
Whenever FlinkHistogram.getStatistics is called it returns back the Wrapper instance of FlinkHistogramStatistics.

@wuchong
Copy link
Member

wuchong commented Mar 24, 2025

Thanks, @gkatzioura, for your contribution! The current test only verifies the success of the proxy invocation but does not validate the logic of the methods themselves. I believe a better approach would be to test each class and method individually like how Flink does:

https://github.com/apache/flink/tree/bfe359c3e52ded31fb0673b48baece7c53b5e65b/flink-metrics/flink-metrics-dropwizard/src/test/java/org/apache/flink/dropwizard/metrics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Umbrella] Subissue Improve Flink Metrics coverage for Unit and Integration Test
2 participants