Prototype for Exponential Histogram Aggregator #3724

jamesmoessis · 2021-10-11T04:21:48Z

Thought I'd raise a draft PR since this piece of work is becoming quite large, and it would probably be better to have some iterative review on it. I apologise for the size, but it was necessary to get any sort of functionality for testing.

This histogram autoscales according to the recordings it receives. Each histogram starts at scale 20 and downscales if a recording does not fit (if calculated index can't be represented by int, or value would cause there to be more buckets than allowed). The max number of buckets is 320. I have taken these values from the NrSketch and the Go implementation done by @jmacd. The unit tests demonstrate this working.

The Indexer used is a simple Logarithm mapper as seen in DoubleExponentialHistogramBuckets.LogarithmMapper. I haven't done any fancy indexing techniques.

The recordings are stored via the DoubleExponentialHistogramBuckets which uses NrSketch's circular backing array, the WindowedCounterArray, which MultiTypeCounterArray for variable bit-length. I've taken them directly from NrSketch which is Apache-2.0 so I've retained their copyright notice next to the Opentelemetry one. However the spotlessJava doesn't like this currently so the build fails. These classes need some additional cleanup too. EDIT: changed this to a reference implementation MapCounter as discussed.

For this to come out of draft, there are some todos which I have commented throughout the code:

Thread safety
Write and test merge() which actually aggregates the accumulations together.
Write more tests to push the boundaries of the histogram. - EDIT: will do more of this in a later PR once we have settled on multiple indexing strategies. See discussion below
Create assert types in metrics-testing and use them for the tests
LongList optimisation in getBucketCounts() mentioned previously Data classes for exponential histogram prototype (#3550) #3637

...a/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java

jsuereth

Overall, looking really good!

...a/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java

jsuereth · 2021-10-11T12:22:33Z

...java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java

+
+class DoubleExponentialHistogramBuckets implements ExponentialHistogramBuckets {
+
+  public static final int MAX_BUCKETS = 320;


IIUC - This is the maximum of JUST positive buckets and we have another 320 for negative?

I think this works out well, because our primary use case (initially) is latency, where we won't have negative buckets.

Yes, the positive and negative buckets are separate, both instances of this class. So overall the max buckets for a histogram would be 640 plus the zero count.

That reminds me that I should probably lazily-instantiate the positive and negative buckets according to the data. No need to use memory on the negative buckets if there aren't any negative recordings.

...java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java

...in/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramData.java

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricDataType.java

jsuereth · 2021-10-11T12:42:48Z

...a/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java

+import java.util.function.Supplier;
+
+final class DoubleExponentialHistogramAggregator
+    extends AbstractAggregator<ExponentialHistogramAccumulation> {


As an FYI - plan to refactor aggregator to have less... history in it and streamline which methods you actually need/use.

A few comments/thoughts:

Your merge method will likely need to figure out how to re-scale histograms between recordings. Specifically for stateful / cumulative aggregation, you're likely to have a lower scale in the cumulative than the most recent recordings. If this turns out to be a major performance issue, there are some alternatives we can talk over.

For delta expert, the merge method is unused, which I assume is how this code is working right now?

I mention this below, but Handle needs to be threadsafe, while all these other method are assumed to only use input values to produce their outputs. For a method like "accumulateDouble" on the aggregator this is only used by async instruments, so I wouldn't worry about optimising it too much. (Creating a temporary handle to accumulate is reasonable).

...java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java

.../java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramAccumulation.java

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/WindowedCounterArray.java

anuraaga · 2021-10-12T02:53:41Z

...java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java

+  @Nonnull
+  @Override
+  public List<Long> getBucketCounts() {
+    // todo LongList optimisation


Can get much of the way there by creating an array here and returning Arrays.asList, LongList will have a similar API

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/WindowedCounterArray.java

jamesmoessis · 2021-10-15T02:59:28Z

Question for @jsuereth - do you think an accumulation should always remain immutable? I ask because it would likely be more efficient to merge accumulation directly into previousAccumulation rather than creating an entirely new accumulation. This seems to be what other implementations are doing.

If it should be immutable that's also fine, was just wondering if I should create a new accumulation or merge into an existing one.

edit: On second thoughts, I think it's fine because when converted to metric data everything is copied. I was just wary because the accumulations are autovalue which are usually immutable classes.

jsuereth · 2021-10-18T18:22:59Z

Question for @jsuereth - do you think an accumulation should always remain immutable? I ask because it would likely be more efficient to merge accumulation directly into previousAccumulation rather than creating an entirely new accumulation. This seems to be what other implementations are doing.

@jamesmoessis I think mutating accumulations as a performance optimisation is ok, but we should be strict about when/how and lifecycle/ownership. Specifcally if you look at DeltaMetricStorage, it relies on immutable accumulations right now. We could adapt this to do a clone + mutate approach when merging deltas. Can you open a ticket on that? I can take a crack, or you can feel free to as well.

Also, FYI - this PR will break significantly here: #3762

jamesmoessis · 2021-10-18T23:04:07Z

@jsuereth Cool, I'll make them entirely immutable for now and we can look at those optimisations later. I have raised the ticket: #3766.

Also, FYI - this PR will break significantly here: #3762

Thanks for the heads up. Will have to do some refactoring when that merges.

jamesmoessis · 2021-10-20T05:38:13Z

@yzhuge Are these algorithms supposed to work with recording the full double range in a single histogram? I am getting some inaccuracies while testing with Double.MAX_VALUE. The bucket's lower bound calculated at that index is slightly larger than the recorded value of Double.MAX_VALUE. It's off by one bucket.

// scale = -3. 
long i = valueToIndex(Double.MAX_VALUE); //  i = 128
BigDecimal lowerBound = BigDecimal.valueOf(256).pow( (int) i); // 256^128
assertThat(BigDecimal.valueOf(Double.MAX_VALUE)).isGreaterThanOrEqualTo(lowerBound); // fails. lower bound > max double
// but passes if I set i = 129

The indexing strategy is as shown in valueToIndex is in the PR, the simple scalb method.

This seems inconsistent to me, would you have any idea what this is due to?

yzhuge · 2021-10-20T17:17:31Z

@jamesmoessis "off by one bucket" is expected on log() based methods. When a value is near a boundary, the method may return either buckets on the two sides of the boundary. Double.MAX is close to power of 2, therefore close to a boundary. Thus the off by one error. This is normal on floating point calculation.
I have tests on scale and limit on various methods starting from https://github.com/newrelic-experimental/newrelic-sketch-java/blob/main/src/test/java/com/newrelic/nrsketch/indexer/BucketIndexerTest.java#L294

For zero and negative scales, the ExponentIndexer is completely accurate because it uses only integer operations.

ScaledExpIndexer.getMaxIndex(), getMinIndexNormal(), getMinIndex() gives completely accurate theoretical value for min and max indexes at a given scale. For scale -3, Max index is 127. So 128 is OK.

codecov · 2021-10-21T00:37:20Z

Codecov Report

Merging #3724 (16564b0) into main (7b86d53) will increase coverage by 0.32%.
The diff coverage is 94.27%.

@@             Coverage Diff              @@
##               main    #3724      +/-   ##
============================================
+ Coverage     89.32%   89.65%   +0.32%     
- Complexity     4085     4229     +144     
============================================
  Files           488      505      +17     
  Lines         12602    13054     +452     
  Branches       1226     1274      +48     
============================================
+ Hits          11257    11703     +446     
  Misses          925      925              
- Partials        420      426       +6

Impacted Files	Coverage Δ
...xporter/otlp/internal/metrics/MetricMarshaler.java	`91.30% <0.00%> (-1.88%)`	⬇️
...entelemetry/exporter/prometheus/MetricAdapter.java	`90.64% <0.00%> (-1.27%)`	⬇️
.../io/opentelemetry/sdk/metrics/data/MetricData.java	`86.20% <50.00%> (-5.80%)`	⬇️
.../aggregator/DoubleExponentialHistogramBuckets.java	`90.09% <90.09%> (ø)`
...lemetry/sdk/metrics/internal/state/MapCounter.java	`95.83% <95.83%> (ø)`
...ng/assertj/metrics/ExponentialHistogramAssert.java	`100.00% <100.00%> (ø)`
...rtj/metrics/ExponentialHistogramBucketsAssert.java	`100.00% <100.00%> (ø)`
...j/metrics/ExponentialHistogramPointDataAssert.java	`100.00% <100.00%> (ø)`
.../sdk/testing/assertj/metrics/MetricAssertions.java	`57.14% <100.00%> (+7.14%)`	⬆️
.../sdk/testing/assertj/metrics/MetricDataAssert.java	`100.00% <100.00%> (ø)`
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b86d53...16564b0. Read the comment docs.

jamesmoessis · 2021-10-21T00:48:43Z

@yzhuge thankyou for the explanation, that makes sense.

For the purposes of keeping this PR simple, I want just the one indexing strategy for now. I will make a note that other indexing strategies are optimal at different scales, and the code here is set up easily so they can be switched out for each other when scale changes. I can address that in a separate PR, along with more rigorous testing.

…some comments to javadoc

… scale reduction bug

...g/src/main/java/io/opentelemetry/sdk/testing/assertj/metrics/ExponentialHistogramAssert.java

jsuereth

Minor comment, but this is looking great for an initial implementation/hook into the SDK! Great work.

jkwatson · 2021-11-04T20:10:02Z

@jsuereth is this something we want in the SDK for the next release, or are we waiting for spec stability around it?

jsuereth · 2021-11-08T22:21:29Z

@jkwatson Regarding stability, only the MetricData interface piece matters (which is effectively based on our stable protocol), The rest is hidden in internal and cannot be exposed in the current public API. From that standpoint I think this is ok.

@jkwatson / @anuraaga If you have suggestions on how to "hide" or "denote experimental" anything in MetricData, let me know. I'd prefer (if possible) to just put a disclaimer on the class itself (and the enum value). While ExponentialHistogram cannot change in breaking ways in our protocol, we may want room to encode it in a different way. For now it's a pure interface, so I think we may have enough flexibility here, but PTAL at that area.

anuraaga · 2021-11-09T05:25:01Z

@jkwatson @jsuereth This is an alpha module, I think it's always ok to merge if the current state looks fine.

@jamesmoessis Can you merge main since it's been a little while? I tried but don't seem to have permission to push to this PR

jamesmoessis · 2021-11-09T05:36:02Z

@anuraaga I've merged main

anuraaga

Thanks @jamesmoessis - I understand there will be some more test coverage in the future but tried to identify stuff that might not be "pushing the boundaries of the histogram" :) But let me know if you'd rather work later, keeping an eye on https://app.codecov.io/gh/open-telemetry/opentelemetry-java

anuraaga · 2021-11-09T06:54:52Z

...g/src/main/java/io/opentelemetry/sdk/testing/assertj/metrics/ExponentialHistogramAssert.java

+    isNotNull();
+    if (actual.getAggregationTemporality() != AggregationTemporality.CUMULATIVE) {
+      failWithActualExpectedAndMessage(
+          actual,


We'll need to add test cases for this class - should we file an issue for it or is it simple enough now? Or can we delete the class for now if it's not useful?

I've gone ahead and added tests for it. Codecov seems to be green now.

anuraaga · 2021-11-09T06:55:57Z

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricData.java

+      return (ExponentialHistogramData) getData();
+    }
+    return DoubleExponentialHistogramData.EMPTY;
+  }


Probably easier to add a test case than a TODO

anuraaga · 2021-11-09T06:56:44Z

...java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java

+      return;
+    } else if (by < 0) {
+      logger.warning(
+          "downScale() expects non-negative integer but was given"


Is this a programming bug in our codebase? Then we can throw IllegalStateException instead. Otherwise probably can add a test case

I believe this should be IllegalStateException.

jsuereth · 2021-11-09T12:59:58Z

@jkwatson @jsuereth This is an alpha module, I think it's always ok to merge if the current state looks fine.

We're trying to get Metrics graduated and out of alpha quickly. I think we should start treating new-feature code review different than bug-fix / friction-fix / tuning code reviews (e.g. Jack's cardinality limits) differently going forward for metrics to make the job of releasing easier.

I.e. rather than viewing Metrics as alpha (where we can continue to dig holes we can't get out of), view it as "Beta" or "approaching stable".

That said my comment around this PR stands. I think it's fine to include, the publicly exposed parts are based on stable protocol and flexible enough we can tweak the implementation going forward.

jmacd · 2021-11-09T16:01:15Z

I will review this work in depth some time today, if that will help. I have a branch with an OTel-Go implementation of this and I'd like to compare and make notes.

jmacd · 2021-11-09T23:13:09Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/MetricAdapter.java

@@ -92,6 +92,8 @@ private static String cleanMetricName(String descriptorMetricName) {
        return Collector.Type.SUMMARY;
      case HISTOGRAM:
        return Collector.Type.HISTOGRAM;
+      case EXPONENTIAL_HISTOGRAM:
+        return Collector.Type.UNKNOWN; // todo exporter for exponential histogram


There's an implied question here -- what should an exporter or processor do when it sees this data and only knows about the old style of histogram. The same question comes up (and is even more pressing) for the OTel collector. I prototyped a converted to produce explicit-boundary histograms here: open-telemetry/opentelemetry-collector#3841

For Prometheus, there will be special considerations -- an auto-scaling aggregator is going to create problems.

Interesting point. Converting to explicit boundary histogram could work. I also wonder how various backends would handle that. I certainly don't have the answers for that.

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/BucketMapper.java

jmacd · 2021-11-09T23:25:39Z

...java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java

+      return;
+    } else if (by < 0) {
+      logger.warning(
+          "downScale() expects non-negative integer but was given"


I believe this should be IllegalStateException.

jkwatson · 2021-11-10T02:35:24Z

...java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java

@@ -48,6 +45,9 @@
  }

  public boolean record(double value) {
+    if (value == 0.0) {
+      throw new IllegalStateException("Illegal attempted recording of zero at bucket level.");


this will potentially crash the user's app, correct? Are we really ok with that, rather than ignoring the recording, and using the ThrottlingLogger to make sure we don't spam the logs too hard?

It's indicative of a bug if this happens. The Handle ensures that all zero values go towards a separate counter zeroCount. This avoids Math.log(0).

Up to you if we log or throw an exception, I was just aligning with what @anuraaga said here on a similar issue: #3724 (comment)

I'm not too sure, but one difference between the two is that one is a private method but this one is public.

Perhaps my comment wasn't clear enough, by programming bug I meant a programming bug of the OTel SDK, not user app. If this is a public API that can be called by a user, and it's their problematic code that causes 0.0 to be passed, then it's a case we wouldn't (can't based on our error handling policy) throw an exception.

This brings another issue to mind, this method should be package private, so I've got ahead and made it so. This is not a user facing method. In fact the class itself is package private. If 0 is passed to this, it would mean there's a bug in the SDK.

Given this info, should it be a log or an exception?

exception is fine. I see this case is guarded by the caller. Can. you add a comment to that effect right before the line where you throw here? Thanks!

Sounds good, I've added a comment 👍

jamesmoessis · 2021-11-10T03:19:06Z

Thanks @jmacd feel free to message me on Slack if you ever want to chat about exponential histogram stuff.

anuraaga requested review from jsuereth and anuraaga October 11, 2021 04:42

jsuereth reviewed Oct 11, 2021

View reviewed changes

...a/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java Show resolved Hide resolved

jsuereth reviewed Oct 11, 2021

View reviewed changes

jamesmoessis mentioned this pull request Oct 11, 2021

Exponential Histogram Prototype #3550

Closed

3 tasks

anuraaga reviewed Oct 12, 2021

View reviewed changes

jamesmoessis force-pushed the exp-hist-aggregator branch from 3c2040b to c2edebc Compare October 18, 2021 03:19

jamesmoessis mentioned this pull request Oct 18, 2021

Allow mutable accumulations #3766

Closed

jamesmoessis added 15 commits October 22, 2021 12:48

draft for exponential histogram aggregator

0846871

autoscaling for exponential histogram

981a98d

more tests for exp histogram

c1a5fa0

re-add nr copyright notice after spotlessapply deleted them; convert …

52f5a9f

…some comments to javadoc

change to long index for bucketmapper

8c64d1d

add scale to hashCode for buckets

c3c95fa

implement jsuereth initial review for exphist

b90f218

move data and pointdata for exphist into data package

327d388

replace windowedCounterArray with minimal map implementation

4fcd0d0

use Arrays.asList() for exposing bucket counts

dab7a84

remove WindowedCounterArray and MultiTypeCounterArray

35dc1b4

javadoc for ExponentialCounter

2473205

implement and test merge() for exponential histogram aggregation; fix…

92a28f9

… scale reduction bug

appease spotless check

8889b02

optimize inital hashmap size for MapCounter

2132acb

jamesmoessis added 2 commits November 1, 2021 14:33

add another test for diff exphist

9438c5b

spotless

2283cc1

breedx-splk mentioned this pull request Nov 2, 2021

Update Metrics to latest specification. #3518

Closed

32 tasks

jsuereth reviewed Nov 4, 2021

View reviewed changes

...g/src/main/java/io/opentelemetry/sdk/testing/assertj/metrics/ExponentialHistogramAssert.java Outdated Show resolved Hide resolved

jsuereth approved these changes Nov 4, 2021

View reviewed changes

address comment regarding failure message

b10c9ea

jamesmoessis mentioned this pull request Nov 9, 2021

Exponential Histogram indexing improvements #3842

Closed

Merge branch 'main' into exp-hist-aggregator

326be07

anuraaga approved these changes Nov 9, 2021

View reviewed changes

jamesmoessis mentioned this pull request Nov 9, 2021

Optimisation and benchmarking of backing data structure for exponential histogram #3848

Closed

jmacd approved these changes Nov 9, 2021

View reviewed changes

jamesmoessis added 3 commits November 10, 2021 11:21

metric assertions test for exponential histogram assertions

e992fc8

replace warning log with IllegalStateException

9beea22

improve test coverage

44504c5

jkwatson reviewed Nov 10, 2021

View reviewed changes

make record package private

fa4f368

anuraaga approved these changes Nov 10, 2021

View reviewed changes

jamesmoessis added 2 commits November 10, 2021 14:57

remove unnecessary double check to improve code cov

75e71ea

add comments

16564b0

jkwatson merged commit 82e2bc2 into open-telemetry:main Nov 12, 2021

jamesmoessis deleted the exp-hist-aggregator branch November 15, 2021 02:34

This was referenced Dec 19, 2021

Temurin JDK #4011

Merged

use Eclipse Temurin JDK docker image #4012

Merged


		class DoubleExponentialHistogramBuckets implements ExponentialHistogramBuckets {

		public static final int MAX_BUCKETS = 320;

Prototype for Exponential Histogram Aggregator #3724

Prototype for Exponential Histogram Aggregator #3724

Conversation

jamesmoessis commented Oct 11, 2021 • edited Loading

jsuereth 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmoessis commented Oct 15, 2021 • edited Loading

jsuereth commented Oct 18, 2021

jamesmoessis commented Oct 18, 2021

jamesmoessis commented Oct 20, 2021

yzhuge commented Oct 20, 2021

codecov bot commented Oct 21, 2021 • edited Loading

Codecov Report

jamesmoessis commented Oct 21, 2021 • edited Loading

jsuereth left a comment

Choose a reason for hiding this comment

jkwatson commented Nov 4, 2021

jsuereth commented Nov 8, 2021

anuraaga commented Nov 9, 2021

jamesmoessis commented Nov 9, 2021

anuraaga 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuereth commented Nov 9, 2021

jmacd commented Nov 9, 2021

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

anuraaga Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

jamesmoessis Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmoessis commented Nov 10, 2021

jamesmoessis commented Oct 11, 2021 •

edited

Loading

jamesmoessis commented Oct 15, 2021 •

edited

Loading

codecov bot commented Oct 21, 2021 •

edited

Loading

jamesmoessis commented Oct 21, 2021 •

edited

Loading

anuraaga Nov 10, 2021 •

edited

Loading

jamesmoessis Nov 10, 2021 •

edited

Loading