Data classes for exponential histogram prototype (#3550) #3637

jamesmoessis · 2021-09-20T03:52:06Z

This addresses the first task from Exponential Histogram Prototype #3550.
Adds exponential histogram to sdk.metrics.data.
No tests becase there aren't any existing tests for classes in this package
This PR should allow work on an aggregator for the exponential histogram to start.

New to OTel contribution so any constructive feedback is welcomed.

linux-foundation-easycla · 2021-09-20T03:52:09Z

The committers are authorized under a signed CLA.

✅ James Moessis (230f116)

...trics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleExponentialHistogramBuckets.java

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

...ics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleExponentialHistogramPointData.java

jsuereth · 2021-09-22T19:21:48Z

One question I have for @anuraaga and @jkwatson (and @jamesmoessis).

Given the cost of exponential histogram generation, would it make more sense for the "data" package to be an interface-only experience and we defer actual storage for an implementation? Specifically, the state of the art seems to be dynamic linked-list or bucket-expansion style classes. I think copying the data out of these representations into the data model may be expensive, looking to see if we can throw ourselves a bone by sticking to interfaces specifically.

jkwatson · 2021-09-22T19:28:13Z

One question I have for @anuraaga and @jkwatson (and @jamesmoessis).

Given the cost of exponential histogram generation, would it make more sense for the "data" package to be an interface-only experience and we defer actual storage for an implementation? Specifically, the state of the art seems to be dynamic linked-list or bucket-expansion style classes. I think copying the data out of these representations into the data model may be expensive, looking to see if we can throw ourselves a bone by sticking to interfaces specifically.

Seems totally reasonable to me.

jsuereth · 2021-09-22T19:32:24Z

Also @jamesmoessis Take a look at metrics-testing, where we have "assertions" that help make it easy to write unit tests. Not required in this PR, but definitely have come in handy.

codecov · 2021-09-23T01:40:28Z

Codecov Report

Merging #3637 (203d87a) into main (bad62ec) will increase coverage by 88.66%.
The diff coverage is 0.00%.

❗ Current head 203d87a differs from pull request most recent head 5908235. Consider uploading reports for the commit 5908235 to get more accurate results

@@             Coverage Diff             @@
##             main    #3637       +/-   ##
===========================================
+ Coverage        0   88.66%   +88.66%     
- Complexity      0     3698     +3698     
===========================================
  Files           0      446      +446     
  Lines           0    11641    +11641     
  Branches        0     1115     +1115     
===========================================
+ Hits            0    10322    +10322     
- Misses          0      937      +937     
- Partials        0      382      +382

Impacted Files	Coverage Δ
...dk/metrics/data/ExponentialHistogramPointData.java	`0.00% <0.00%> (ø)`
...io/opentelemetry/context/StrictContextStorage.java	`75.00% <0.00%> (ø)`
...y/sdk/logging/export/BatchLogProcessorBuilder.java	`66.66% <0.00%> (ø)`
...y/sdk/metrics/internal/state/MeterSharedState.java	`91.04% <0.00%> (ø)`
...porter/otlp/internal/metrics/SummaryMarshaler.java	`100.00% <0.00%> (ø)`
...telemetry/sdk/metrics/SdkMeterProviderBuilder.java	`100.00% <0.00%> (ø)`
...ntelemetry/sdk/metrics/data/DoubleSummaryData.java	`100.00% <0.00%> (ø)`
...telemetry/extension/noopapi/NoopOpenTelemetry.java	`75.00% <0.00%> (ø)`
.../opentelemetry/sdk/metrics/data/DoubleSumData.java	`100.00% <0.00%> (ø)`
...ntelemetry/sdk/testing/assertj/SpanDataAssert.java	`93.43% <0.00%> (ø)`
... and 436 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 bad62ec...5908235. Read the comment docs.

jamesmoessis · 2021-09-23T01:42:35Z

Given the cost of exponential histogram generation, would it make more sense for the "data" package to be an interface-only experience and we defer actual storage for an implementation?

I agree, and it's why I've backed off implementing any real logic in this PR. I could probably move it to a pure interface. Is something like this what you are imagining?

public interface ExponentialHistogramData extends Data<ExponentialHistogramPointData> {
  ...
}

---

public interface ExponentialHistogramPointData extends PointData {
  ...
}

Specifically, the state of the art seems to be dynamic linked-list or bucket-expansion style classes.

Which implementations are you referring to here? I would love to have a look and see the best ways of implementing.

jsuereth · 2021-09-23T12:44:12Z

@jamesmoessis

Is something like this what you are imagining?

Exactly. The requirements I imagine for usefulness are:

A set of interface/methods that make extracting OTLP simple
A way of constructing "dummy" data classes when testing export-only paths
A set of assertj classes to simplify asserting results of instruments (similar to this).

Not suggesting anything but (1) for this specific PR.

Which implementations are you referring to here? I would love to have a look and see the best ways of implementing.

HdrHistogram uses base10, but has similar behavior.
NrSketch New Relic's algorithm specfically against this proposal.
Prometheus SparseHistogram (go)

To a lesser extent, you can look at DDSketch , which may share similarities in storage, but not usage.

jsuereth · 2021-09-23T18:27:05Z

Also @jmacd added more links to prototypes, test code and codegen for constants here: open-telemetry/oteps#179

jamesmoessis · 2021-09-27T01:48:36Z

@jsuereth thanks for the explanations.

I've changed the abstract classes here to pure interfaces like you suggested. Essentially, a bunch of getters that map directly to the OTLP definitions.

I've left bucket mapping/indexing out because I believe these details are slightly more contentious and could be figured out in the implementations.

I think copying the data out of these representations into the data model may be expensive

Are you envisioning the implementations of these interfaces to be a mutable representation, to avoid copying?

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

jsuereth · 2021-09-27T12:36:22Z

Are you envisioning the implementations of these interfaces to be a mutable representation, to avoid copying?

I'd like to leave that possibility open to use, yeah. For initial version feel free to copy-data and just get it working (assuming @jkwatson @anuraaga are on board with that). Using interfaces, hopefully keeps your options open for how to optimise later.

jkwatson · 2021-09-27T16:12:43Z

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

+   *
+   * @return the bucket counts.
+   */
+  List<Long> getBucketCounts();


Do we think we need Long level numbers for the bucket counts? That's a lot of recordings for a single bucket.

In the NrSketch implementation there is a ~~backing array that uses variable-width counts~~ backing array that uses variable-width counts.

See also OTel-Go: open-telemetry/opentelemetry-go#2261

Looking at the details here, I think exposing List<Long> may be... overkill for Java. (@anuraaga will prove me wrong with some fun optimisation around List), but from every example @jmacd lists, I think we want to have the flexibilty to use a primitive array and keep the primitives. That implies exposing a long getBucketCountAt(int idx) vs. an actual list (which is what the Go SDK + NR impl are doing).

It would make sense to use something similar to the WindowedCounterArray backed by the MultiTypeCounterArray from the NrSketch that @jmacd mentioned.

I can replace List<Long> getBucketCounts() with long getBucketCountAt(int idx). Though, it does raise the question in my mind of how the entire collection of counts would be gathered easily. I can't see that in the Go SDK. Am I incorrectly assuming that it's a necessary method to have for aggregation/exporting?

Ok, I've changed this to long getBucketCountAt(int index) and updated relevant javadocs.

By the way, I expect marshaling to look like

int size = 0; for (int i = 0; i < getNumBuckets(); i++) { size += CodedOutputStream.computeUint32SizeNoTag(getBucketCountAt(i)); } writeTag(fieldNumber, size) for (int i = 0; i < getNumBuckets(); i++) { writeUint32NoTag(getBucketCountAt(i); }

By the way, just curious if any consideration was given to a sparse array given this comment in the proto

This field is expected to have many buckets, // especially zeros, so uint64 has been selected to ensure // varint encoding.

repeated fixed32 bucket_indexes; repeated fixed64 bucket_counts;

No need to write out zeros

@anuraaga @jsuereth @jkwatson I've made a few changes:

I've removed the List<Long> and replaced it with these three methods with the aim of being flexible for implementations.

long getBucketCountAt(int index)

int getNumBuckets()

int getStartIndex()

@anuraaga so your for loop would look like

for (int i = getStartIndex(); i < getNumBuckets(); i++) { size += CodedOutputStream.computeUint32SizeNoTag(getBucketCountAt(i)); }

EDIT: going to move back to List<Long> as per below conversation.

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

anuraaga · 2021-09-28T08:19:23Z

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

+   * @param index signed int corresponding to the relevant bucket.
+   * @return the number of measurements in the bucket.
+   */
+  long getBucketCountAt(int index);


Oh - I think we need the number of buckets (the upper limit of index) to be able to actually iterate this.

Probably needs to be the upper_limit_index - lower_limit_index since the index can be negative and doesn't always start from 0. It might be easier to implement or expose an Iterable, I would like to hear your thoughts on that.

As mentioned above, I've added getNumBuckets() and getStartIndex() to solve for this.

Thanks for iterating on this - I'm sorry for the back and forth, but this complexity of the indexes makes me prefer the iterable a lot then. Can we go back to List<Long>? Sorry about that.

We'll just cast to an internal hypothetical LongList that stores and allows iterating on long[] in the exporters so it's still efficient.

LongList is neat, that sounds good to me. I'll make the changes tomorrow.

jamesmoessis · 2021-09-29T23:58:51Z

@jkwatson @anuraaga @jsuereth I've pushed the change to move back to List<Long> getBucketCounts() as discussed. As mentioned, this can still be optimised in the implementation.

If there's no other points of discussion, I think this PR is in its final form.

anuraaga

Thanks @jamesmoessis - let me go ahead and merge this as if anything comes up we can easily follow up

Data classes for exponential histogram prototype (open-telemetry#3550)

230f116

jamesmoessis requested review from anuraaga, arminru, bogdandrutu, carlosalberto and jkwatson as code owners September 20, 2021 03:52

jamesmoessis requested a review from a user September 20, 2021 03:52

jamesmoessis requested review from Oberon00, pavolloffay, thisthat and tylerbenson as code owners September 20, 2021 03:52

jkwatson reviewed Sep 20, 2021

View reviewed changes

...trics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleExponentialHistogramBuckets.java Outdated Show resolved Hide resolved

jkwatson reviewed Sep 20, 2021

View reviewed changes

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleExponentialHistogramData.java Outdated Show resolved Hide resolved

jamesmoessis added 4 commits September 21, 2021 12:16

add javadoc to DoubleExponentialHistogramData.create()

0cf7ff4

appease linter

9105a63

more javadoc for exponential histogram

09ca9b5

more verbose param description of expo histogram scale

8960a6d

jkwatson reviewed Sep 22, 2021

View reviewed changes

...ics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleExponentialHistogramPointData.java Outdated Show resolved Hide resolved

add java doc to all public methods, remove unnecessary overrides

3b72b19

change expo histogram data to pure interface

b94a6e1

jsuereth approved these changes Sep 27, 2021

View reviewed changes

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/ExponentialHistogramPointData.java Outdated Show resolved Hide resolved

add default method for getBase()

e7ec6b8

jkwatson reviewed Sep 27, 2021

View reviewed changes

change from getBucketCounts() -> getBucketCountAt(idx)

203d87a

anuraaga approved these changes Sep 28, 2021

View reviewed changes

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/ExponentialHistogramPointData.java Outdated Show resolved Hide resolved

anuraaga reviewed Sep 28, 2021

View reviewed changes

jamesmoessis added 3 commits September 29, 2021 11:39

add getNumBuckets() and getStartIndex()

bc5910a

remove getBase() because it is not required

bb6a10d

go back to List<Long> for bucket counts

5908235

anuraaga approved these changes Sep 30, 2021

View reviewed changes

anuraaga merged commit 694ac3f into open-telemetry:main Sep 30, 2021

jamesmoessis mentioned this pull request Oct 11, 2021

Prototype for Exponential Histogram Aggregator #3724

Merged

5 tasks

This was referenced Dec 19, 2021

Temurin JDK #4011

Merged

use Eclipse Temurin JDK docker image #4012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data classes for exponential histogram prototype (#3550) #3637

Data classes for exponential histogram prototype (#3550) #3637

jamesmoessis commented Sep 20, 2021

linux-foundation-easycla bot commented Sep 20, 2021 •

edited

Loading

jsuereth commented Sep 22, 2021

jkwatson commented Sep 22, 2021

jsuereth commented Sep 22, 2021

codecov bot commented Sep 23, 2021 •

edited

Loading

jamesmoessis commented Sep 23, 2021

jsuereth commented Sep 23, 2021

jsuereth commented Sep 23, 2021

jamesmoessis commented Sep 27, 2021 •

edited

Loading

jsuereth commented Sep 27, 2021

jkwatson Sep 27, 2021

jmacd Sep 27, 2021 •

edited

Loading

jsuereth Sep 27, 2021

jamesmoessis Sep 28, 2021

jamesmoessis Sep 28, 2021

anuraaga Sep 28, 2021

anuraaga Sep 28, 2021

jamesmoessis Sep 29, 2021 •

edited

Loading

anuraaga Sep 28, 2021

jamesmoessis Sep 28, 2021 •

edited

Loading

jamesmoessis Sep 29, 2021

anuraaga Sep 29, 2021 •

edited

Loading

jamesmoessis Sep 29, 2021

jamesmoessis commented Sep 29, 2021

anuraaga left a comment

Data classes for exponential histogram prototype (#3550) #3637

Data classes for exponential histogram prototype (#3550) #3637

Conversation

jamesmoessis commented Sep 20, 2021

linux-foundation-easycla bot commented Sep 20, 2021 • edited Loading

jsuereth commented Sep 22, 2021

jkwatson commented Sep 22, 2021

jsuereth commented Sep 22, 2021

codecov bot commented Sep 23, 2021 • edited Loading

Codecov Report

jamesmoessis commented Sep 23, 2021

jsuereth commented Sep 23, 2021

jsuereth commented Sep 23, 2021

jamesmoessis commented Sep 27, 2021 • edited Loading

jsuereth commented Sep 27, 2021

Choose a reason for hiding this comment

jmacd Sep 27, 2021 • edited Loading

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 Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmoessis Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmoessis commented Sep 29, 2021

anuraaga left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Sep 20, 2021 •

edited

Loading

codecov bot commented Sep 23, 2021 •

edited

Loading

jamesmoessis commented Sep 27, 2021 •

edited

Loading

jmacd Sep 27, 2021 •

edited

Loading

jamesmoessis Sep 29, 2021 •

edited

Loading

jamesmoessis Sep 28, 2021 •

edited

Loading

anuraaga Sep 29, 2021 •

edited

Loading