nh support for metrics aggregation by Shvejan · Pull Request #7359 · cortexproject/cortex

Shvejan · 2026-03-18T08:56:44Z

What this PR does:

Add native histogram support to metrics aggregation helper, the change also supports dual-format histograms that expose both native and classic bucket formats.
few methods and structs like makeBucketsFromMap and nativeHistogramMetric are copied from Prometheus implementation to follow the same patterns.

Which issue(s) this PR fixes:
Fixes #6489

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Shvejan Mutheboyina <shvejan@amazon.com>

Signed-off-by: Shvejan <49836470+Shvejan@users.noreply.github.com>

Signed-off-by: Shvejan Mutheboyina <shvejan@amazon.com>

Signed-off-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com>

CHANGELOG.md

Co-authored-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com> Signed-off-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com>

Signed-off-by: Shvejan <49836470+Shvejan@users.noreply.github.com>

SungJin1212 · 2026-03-23T00:27:39Z

pkg/util/metrics_helper.go

+	if isNative(h1) || isNative(h2) {
+		// Use schema/threshold from whichever side has native data (they should match).
+		if !isNative(h1) {
+			schema := h2.GetSchema()
+			h1.Schema = &schema
+			zt := h2.GetZeroThreshold()
+			h1.ZeroThreshold = &zt
+		}


I wonder if h1 and h2 always have the same schema.

For this usecase for aggregating in SendSumOfHistograms, they should have the same schema as we aggregate the same histogram exposed by prometheus client go library.

yeya24

I'd like to also see an integration test that validates this code could by exposing both classic and NH metrics. Unit tests alone is not sufficient here

yeya24 · 2026-03-23T05:00:49Z

pkg/util/metrics_helper.go

+
+		// Sentinel span for native histograms with no observations.
+		// This is required to distinguish an empty native histogram from a classic histogram.
+		// This matches the prometheus behavior (histogram.go:1958)


Let's add the code link instead of just file and line.

yeya24 · 2026-03-23T05:03:09Z

pkg/util/metrics_helper.go

+	if isNative(h1) || isNative(h2) {
+		// Use schema/threshold from whichever side has native data (they should match).
+		if !isNative(h1) {
+			schema := h2.GetSchema()
+			h1.Schema = &schema
+			zt := h2.GetZeroThreshold()
+			h1.ZeroThreshold = &zt
+		}


For this usecase for aggregating in SendSumOfHistograms, they should have the same schema as we aggregate the same histogram exposed by prometheus client go library.

yeya24 · 2026-03-23T05:06:18Z

pkg/util/metrics_helper.go

+// spans+deltas encoding used by the native histogram proto representation.
+//
+// This implementation is the same as makeBucketsFromMap from prometheus
+// (histogram.go:2006) to include the gap-filling optimization


yeya24 · 2026-03-23T05:07:27Z

pkg/util/metrics_helper_test.go

+}
+
+// TestHistogramData_AddHistogram_Multiple tests merging multiple histograms
+func TestHistogramData_AddHistogram_Multiple(t *testing.T) {


Can we consolidate all TestHistogramData_AddHistogram tests? Seems they test similar things. Let's simplify the tests by consolidating them

yeya24 · 2026-03-23T05:08:33Z

pkg/util/metrics_helper_test.go

+}
+
+// TestMergeHistogram tests the mergeHistogram function
+func TestMergeHistogram(t *testing.T) {


We need more coverage for this test case like merging native and classic or merging only classic ones

added some unit tests to merge classic + native metrics

Signed-off-by: Shvejan Mutheboyina <shvejan@amazon.com>

Signed-off-by: Shvejan <49836470+Shvejan@users.noreply.github.com>

yeya24 · 2026-03-25T21:36:25Z

integration/scrape_native_histogram_metrics_test.go

+		// Testing Native histogram fields
+		require.Equal(t, int32(3), h.GetSchema())
+		require.NotNil(t, h.GetZeroThreshold())
+		require.Equal(t, uint64(0), h.GetZeroCount())


Can we validate the actual spans and buckets of native histogram? Validating these fields might not be useful

Signed-off-by: Shvejan Mutheboyina <shvejan@amazon.com>

yeya24

Thanks!

nh support for metrics aggregation

62d56be

Signed-off-by: Shvejan Mutheboyina <shvejan@amazon.com>

pull-request-size bot added the size/XL label Mar 18, 2026

dosubot bot added the type/feature label Mar 18, 2026

Shvejan and others added 2 commits March 18, 2026 01:57

Merge branch 'master' into expose-nh-metrics

cfc5cee

Signed-off-by: Shvejan <49836470+Shvejan@users.noreply.github.com>

small fix: make modernize

4740331

Signed-off-by: Shvejan Mutheboyina <shvejan@amazon.com>

Shvejan force-pushed the expose-nh-metrics branch from 3b84c49 to 4740331 Compare March 19, 2026 19:39

Merge branch 'master' into expose-nh-metrics

751d91e

Signed-off-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com>

friedrichg reviewed Mar 20, 2026

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

friedrichg and others added 2 commits March 20, 2026 13:39

Fix PR number in changelog

ba81824

Co-authored-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com> Signed-off-by: Friedrich Gonzalez <1517449+friedrichg@users.noreply.github.com>

Merge branch 'master' into expose-nh-metrics

67b5d04

Signed-off-by: Shvejan <49836470+Shvejan@users.noreply.github.com>

SungJin1212 reviewed Mar 23, 2026

View reviewed changes

yeya24 reviewed Mar 23, 2026

View reviewed changes

Shvejan and others added 2 commits March 25, 2026 19:33

fixup! nh support for metrics aggregation

568fc2d

Signed-off-by: Shvejan Mutheboyina <shvejan@amazon.com>

Merge branch 'master' into expose-nh-metrics

af7e145

Signed-off-by: Shvejan <49836470+Shvejan@users.noreply.github.com>

Shvejan force-pushed the expose-nh-metrics branch 2 times, most recently from a554ea2 to 9d15b5c Compare March 25, 2026 20:34

yeya24 reviewed Mar 25, 2026

View reviewed changes

fixing tests, lint

c62f5c6

Signed-off-by: Shvejan Mutheboyina <shvejan@amazon.com>

Shvejan force-pushed the expose-nh-metrics branch from 9d15b5c to c62f5c6 Compare March 25, 2026 22:29

yeya24 approved these changes Mar 25, 2026

View reviewed changes

yeya24 merged commit 8fbfe5f into cortexproject:master Mar 25, 2026
33 of 35 checks passed

Conversation

Shvejan commented Mar 18, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants