Skip to content

Conversation

felixbarny
Copy link
Member

Adds support for histograms and exponential histograms.

Part of #133057.

@felixbarny felixbarny self-assigned this Sep 1, 2025
@felixbarny felixbarny added >non-issue :StorageEngine/TSDB You know, for Metrics labels Sep 1, 2025
@elasticsearchmachine elasticsearchmachine added Team:StorageEngine v9.2.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Sep 1, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@felixbarny felixbarny requested a review from a team as a code owner September 2, 2025 10:09
}

@ParametersFactory(argumentFormatting = "%1$s")
public static List<Object[]> testCases() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add randomized testing too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same can we do randomized testing too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd use createHybridDigest that's the default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it but this makes the error worse so that the assertion frequently fails.

Copy link
Contributor

@kkrik-es kkrik-es left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

@felixbarny felixbarny enabled auto-merge (squash) September 9, 2025 12:09
@felixbarny felixbarny merged commit 63840f1 into elastic:main Sep 9, 2025
34 checks passed
@felixbarny felixbarny deleted the otlp-histograms branch September 9, 2025 12:24
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Sep 9, 2025
Kubik42 pushed a commit to Kubik42/elasticsearch that referenced this pull request Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue :StorageEngine/TSDB You know, for Metrics Team:StorageEngine v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants