OTLP: add support for histograms #133902
OTLP: add support for histograms #133902
Conversation
elasticsearchmachine
added
Team:StorageEngine
v9.2.0
external-contributor
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
...lugin/otel-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPoint.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jonas Kunz <[email protected]>
...lugin/otel-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPoint.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jonas Kunz <[email protected]>
.../src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPointGroupingContext.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @ParametersFactory(argumentFormatting = "%1$s") | ||
| public static List<Object[]> testCases() { |
There was a problem hiding this comment.
Can we add randomized testing too?
There was a problem hiding this comment.
Not sure how this would work. If we create the histogram randomly, how do we determine the expected values/counts?
I guess we could do some sort of randomized smoke testing to check that converting randomly created histograms does not throw exceptions, without validating the counts/values. Is that what you had in mind?
There was a problem hiding this comment.
You can generate a random set of values, then build an ExpHisto or bucketed histo out of them and a TDigest as a control. Converting should get you close to the control TDigest, with some error margin?
There was a problem hiding this comment.
I assume we'd need to compare the error in calculating percentile values. Comparing the resulting buckets directly doesn't seem feasible as there are no guarantee that an exponential histogram and a TDigest histogram would create an equal number of buckets for a given set of raw values.
This seems to test the algorithm for creating the histograms and calculating the percentiles more so than the actual conversion. Also not sure if there's a good way to estimate a sensible error bound. The error will depend a lot on the parameters of the histogram algorithm, such as the max number of buckets. Both algorithms have different parameters that affect the precision in different ways.
It goes into the direction of comparing the inherent accuracy of TDigest and exponential histograms which seems outside of the scope of converting an externally recorded exponential histogram into the most appropriate TDigest-based representation. We know that there will be a precision loss and we're working on having native support for exponential histograms.
There was a problem hiding this comment.
Are you suggesting that the error from the double conversion is unbound? That would be really bad..
Otherwise, let's make sure there's coverage with random datasets for the logic that performs the conversion. Percentile results should be close, with reasonable error margins.
There was a problem hiding this comment.
I'm not saying it's unbound, it's just difficult to set a bound on it.
Ideally, the error shouldn't exceed the individual error from exponential histograms plus TDigest. I made some empirical testing using different distributions and had to multiply the combined error by 2 to be on the safe side.
Does that level of testing seem good for now?
| this.valid = valid; | ||
| } | ||
|
|
||
| public void testHistograms() throws Exception { |
There was a problem hiding this comment.
Same can we do randomized testing too?
There was a problem hiding this comment.
This is more difficult compared to the testing for exponential histograms I did. I don't think there's a reasonable error bound that we can calculate or estimate for explicit boundary histograms as the bucket boundaries are user-configurable.
I'd say we skip this for now.
| } | ||
| double exponentialHistogramMaxError = QuantileAccuracyTests.getMaximumRelativeError(samples, numBuckets); | ||
| double combinedRelativeError = rawTDigestMaxError + exponentialHistogramMaxError; | ||
| assertThat(convertedTDigestMaxError, lessThanOrEqualTo(combinedRelativeError * 2)); |
There was a problem hiding this comment.
Let's add a comment about the 2 factor here.
I assume you ran it 1000 times to make sure this won't be noisy..
There was a problem hiding this comment.
I've added a comment.
I assume you ran it 1000 times to make sure this won't be noisy..
Yes I have. I have now also executed 10,000 runs and with the lowest number of buckets and samples. There were some failures, so I've bumped the min number of samples we're testing with.
| } | ||
|
|
||
| private static TDigest convertToTDigest(ExponentialHistogramDataPoint otlpHistogram) { | ||
| TDigest result = TDigest.createAvlTreeDigest(arrays, 100); |
There was a problem hiding this comment.
Nit: I'd use createHybridDigest that's the default.
There was a problem hiding this comment.
I've tested it but this makes the error worse so that the assertion frequently fails.
4 participants
Adds support for histograms and exponential histograms.
Part of #133057.