Implement java exponential histograms (#28903) #28995
Conversation
Merge master to forked repo. 10/13/2023-1
JayajP
force-pushed
the
exponentialhistograms
branch
from
448b72b to
9664a02
Compare
There was a problem hiding this comment.
is there a semantic meaning to -3?, same with the 3 on line 305.
can this be a constant declaration (private static final) with the semantic meaning in the name?
There was a problem hiding this comment.
same as comment above, maybe rename and declare as private static final int MAX_INPUT_NUM_BUCKETS = 32 inside ExponentialBuckets
There was a problem hiding this comment.
<a href="https://opentelemetry.io/docs/specs/otel/metrics/data-model/#exponentialhistogram">OpenTelemetry's notion of ExponentialHistogram</a>
| // The following tests cover exponential buckets. | ||
| @Test | ||
| public void testPositiveScaleBucket() { |
There was a problem hiding this comment.
maybe rename the tests that test exponential buckets to
testExponentialBuckets_{TEST_CASE}
for example testExponentialBuckets_positiveScaleBuckets()
| HistogramData data = HistogramData.exponential(0, 20); | ||
| assertThat(data.getBucketType().getBucketSize(0), equalTo(2.0)); | ||
| // 10th bucket contains [2^10, 2^11). | ||
| assertThat(data.getBucketType().getBucketSize(10), equalTo(1024.0)); | ||
|
|
||
| data = HistogramData.exponential(1, 20); | ||
| assertThat(data.getBucketType().getBucketSize(0), equalTo(Math.sqrt(2))); | ||
| // 10th bucket contains [2^5, 2^5.5). | ||
| assertThat(data.getBucketType().getBucketSize(10), closeTo(13.2, .1)); | ||
|
|
||
| data = HistogramData.exponential(-1, 20); | ||
| assertThat(data.getBucketType().getBucketSize(0), equalTo(4.0)); | ||
| // 10th bucket contains [2^20, 2^22). | ||
| assertThat(data.getBucketType().getBucketSize(10), equalTo(3145728.0)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testNumBuckets() { | ||
| // Validate that numBuckets clipping WAI. | ||
| HistogramData data = HistogramData.exponential(0, 200); | ||
| assertThat(data.getBucketType().getNumBuckets(), equalTo(32)); | ||
|
|
||
| data = HistogramData.exponential(3, 500); | ||
| assertThat(data.getBucketType().getNumBuckets(), equalTo(32 * 8)); | ||
|
|
||
| data = HistogramData.exponential(-3, 500); | ||
| assertThat(data.getBucketType().getNumBuckets(), equalTo(4)); | ||
| } |
There was a problem hiding this comment.
since HistogramData.exponential(...) creates a new instance of HistogramData, how about just declaring a new variable for every instance for clarity?
It is also ok to have even smaller tests with narrower assertions if each instance is testing a different part of the behavior of the method (in this casegetNumBuckets) go/tott/648
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Run Java PreCommit |
|
Different test failures on https://ci-beam.apache.org/job/beam_PreCommit_Java_Phrase/6297/ and https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/28670/ not likely relevant, merging for now |
* Implement java exponential histograms (apache#28903) * Address comments * Address comments
3 participants
Implement exponential histogram metric in Java.
Currently this only supports a scaling factor of 2^(2^k) for an integer k between [-3, 3].
e.g. Histograms with a growth factor of 2^(1/8), 2^(1/4), 2^(1/2), 2, 2^2.
See
HistogramData::exponentialfor further details.For the initial use-case we will use histograms with a growth factor of sqrt(2) to record BigQuery write latencies.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.