SendSumOfHistograms should support native histograms

[yeya24]

Is your feature request related to a problem? Please describe.
SendSumOfHistograms is a helper function Cortex uses to aggregate histograms from metrics registries. For example, we have certain metrics emitted from Thanos library and we aggregate them in Cortex.

cortex/pkg/storegateway/bucket_store_metrics.go

Line 339 in 7ec57ac

data.SendSumOfHistograms(out, m.indexHeaderLoadDuration, "thanos_bucket_store_indexheader_load_duration_seconds")

With the introduction of some metrics being native histograms in Thanos, SendSumOfHistograms is not working properly as the current implementation only support legacy histograms.

See

cortex/pkg/util/metrics_helper.go

Line 474 in 7ec57ac

histoBuckets := histo.GetBucket()

It ignores native histogram buckets now

	histoBuckets := histo.GetBucket()
	if len(histoBuckets) > 0 && d.buckets == nil {
		d.buckets = map[float64]uint64{}
	}

Describe the solution you'd like
Implement the sum aggregation with native histograms also

Additional context
It is unclear how would we merge legacy histograms with native histograms. Might not be a concern as we can assume in the single process it would be consistent to be either legacy or native histogram not both the same time.

[cswpy]

I'd love to take this issue but not sure whether I could take on this as a new contributor. If possible, I can spend some time familarizing myself with Cortex architecture and take a stab at this. Any tips would be appreciated :)

[yeya24]

Hi @cswpy, this specific PR doesn't need knowledge of Cortex architecture. It is more of aggregating histogram data types and handle native histograms. Some relevant knowledge are the usage of https://github.com/prometheus/client_golang and the implementation of native histogram type

[cswpy]

Thanks for the tips! I spent some time reading the Native Histogram Specs. From what I understood, since the native histograms have adaptive bucket boundaries, determined by schema, we need to calculate the upper boundaries and corresponding cumulative count, and add them to Cortex's *HistogramData (essentially convert the native histogram into conventional representation and merge into Cortex's data model). Feel free to correct me if I am wrong here.

[yeya24]

No, I think it is the opposite. HistogramData today has sample count, sum and histogram buckets and we aggregate classic histograms this way and at the end we expose HistogramData to the same format of classic histogram.

Native histogram uses a different format and our goal here is to aggregate native histograms and expose native histograms during scraping. We should extend HistogramData struct with native histogram related fields and at the end we aggregate using the new fields and expose native histogram.