Implement java exponential histograms (#28903) #28995

JayajP · 2023-10-13T21:13:09Z

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::exponential for 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:

Mention the appropriate issue in your description (for example: 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, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

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.

Merge master to forked repo. 10/13/2023-1

m-trieu · 2023-10-13T21:38:34Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java

+    public abstract double getRangeTo();
+
+    public static ExponentialBuckets of(int scale, int numBuckets) {
+      if (scale < -3) {


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?

m-trieu · 2023-10-13T21:40:10Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java

+    private static int computeNumberOfBuckets(int scale, int inputNumBuckets) {
+      if (scale == 0) {
+        // When base=2 then the bucket at index 31 contains [2^31, 2^32).
+        return Math.min(32, inputNumBuckets);


same as comment above, maybe rename and declare as private static final int MAX_INPUT_NUM_BUCKETS = 32 inside ExponentialBuckets

m-trieu · 2023-10-13T21:43:18Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java

+   * </pre>
+   *
+   * <pre>
+   * Example sacle/boundaries:


m-trieu · 2023-10-13T21:45:00Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java

+   * When scale=-1, buckets 0,1,2...i have lowerbounds 0, 4, 4^2, ... 4^(i).
+   * </pre>
+   *
+   * Scale parameter is similar to OpenTelemetry's notion of ExponentialHistogram.


<a href="https://opentelemetry.io/docs/specs/otel/metrics/data-model/#exponentialhistogram">OpenTelemetry's notion of ExponentialHistogram</a>

m-trieu · 2023-10-13T21:53:11Z

sdks/java/core/src/test/java/org/apache/beam/sdk/util/HistogramDataTest.java

+  // The following tests cover exponential buckets.
+  @Test
+  public void testPositiveScaleBucket() {


maybe rename the tests that test exponential buckets to

testExponentialBuckets_{TEST_CASE}
for example testExponentialBuckets_positiveScaleBuckets()

m-trieu · 2023-10-13T21:58:18Z

sdks/java/core/src/test/java/org/apache/beam/sdk/util/HistogramDataTest.java

+    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));
+  }


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

github-actions · 2023-10-14T00:18:09Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

m-trieu

LGTM

Abacn · 2023-10-18T19:26:00Z

Run Java PreCommit

Abacn · 2023-10-18T23:45:04Z

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

github-actions bot added the java label Oct 13, 2023

JayajP added 2 commits October 13, 2023 14:40

Merge pull request #1 from apache/master

52c7335

Merge master to forked repo. 10/13/2023-1

Implement java exponential histograms (apache#28903)

9664a02

JayajP force-pushed the exponentialhistograms branch from 448b72b to 9664a02 Compare October 13, 2023 21:45

m-trieu suggested changes Oct 13, 2023

View reviewed changes

JayajP added 3 commits October 16, 2023 15:18

Address comments

bf328cb

Address comments

3e254cd

address comments

7394c77

m-trieu approved these changes Oct 16, 2023

View reviewed changes

Abacn merged commit 9aaf9c2 into apache:master Oct 18, 2023
33 of 37 checks passed

kkdoon pushed a commit to twitter-forks/beam that referenced this pull request Oct 21, 2023

Implement java exponential histograms (apache#28903) (apache#28995)

10b8d10

* Implement java exponential histograms (apache#28903) * Address comments * Address comments

JayajP deleted the exponentialhistograms branch November 15, 2023 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement java exponential histograms (#28903) #28995

Implement java exponential histograms (#28903) #28995

JayajP commented Oct 13, 2023

m-trieu Oct 13, 2023

JayajP Oct 16, 2023

m-trieu Oct 13, 2023

JayajP Oct 16, 2023

m-trieu Oct 13, 2023

JayajP Oct 16, 2023

m-trieu Oct 13, 2023

JayajP Oct 16, 2023

m-trieu Oct 13, 2023

JayajP Oct 16, 2023

m-trieu Oct 13, 2023

JayajP Oct 16, 2023

github-actions bot commented Oct 14, 2023

m-trieu left a comment

Abacn commented Oct 18, 2023

Abacn commented Oct 18, 2023

Implement java exponential histograms (#28903) #28995

Implement java exponential histograms (#28903) #28995

Conversation

JayajP commented Oct 13, 2023

GitHub Actions Tests Status (on master branch)

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

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

github-actions bot commented Oct 14, 2023

m-trieu left a comment

Choose a reason for hiding this comment

Abacn commented Oct 18, 2023

Abacn commented Oct 18, 2023