Skip to content

[receiver/prometheusreceiver] Convert NHCB to explicit histogram metric#43095

Merged
songy23 merged 9 commits into
open-telemetry:mainfrom
jelly-afk:convert-nhcb-explicit-histogram-metric
Oct 14, 2025
Merged

[receiver/prometheusreceiver] Convert NHCB to explicit histogram metric#43095
songy23 merged 9 commits into
open-telemetry:mainfrom
jelly-afk:convert-nhcb-explicit-histogram-metric

Conversation

@jelly-afk

@jelly-afk jelly-afk commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

Description

added prometheus NHCB (native histogram with custom buckets) to otel explicit histogram conversion.

Link to tracking issue

Fixes #41131

Testing

Added TestMetricGroupData_toNHCBDistributionUnitTest test function to test the conversion.

@jelly-afk jelly-afk requested review from a team, ArthurSens and dashpole as code owners October 1, 2025 07:34
@github-actions github-actions Bot added the receiver/prometheus Prometheus receiver label Oct 1, 2025
@jelly-afk jelly-afk changed the title [receiver/prometheusreceiver] Convert nhcb explicit histogram metric [receiver/prometheusreceiver] Convert NHCB to explicit histogram metric Oct 1, 2025
@jelly-afk jelly-afk force-pushed the convert-nhcb-explicit-histogram-metric branch from c61c27b to 957df66 Compare October 1, 2025 07:37
@jelly-afk jelly-afk marked this pull request as draft October 2, 2025 14:11
@jelly-afk

Copy link
Copy Markdown
Contributor Author

I would like do some changes here, will open when done.

@jelly-afk

Copy link
Copy Markdown
Contributor Author

In remotewrite receiver if spans are empty, bucket counts are not filled

bucketCounts := make([]uint64, len(histogram.CustomValues)+1)
// NHCB uses the positive bucket list and spans for all buckets
if len(histogram.PositiveSpans) == 0 {
return bucketCounts
}

we should follow it here too, right?

@dashpole

dashpole commented Oct 2, 2025

Copy link
Copy Markdown
Contributor

I believe it is filled with all zeroes by the make() line

@jelly-afk jelly-afk marked this pull request as ready for review October 3, 2025 12:56
@jelly-afk

Copy link
Copy Markdown
Contributor Author

ready for review

@ArthurSens ArthurSens left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for working on this one! It looks almost ready, to be honest. I think we can improve a little bit on the readability of the getOrCreatedMetricFamily method, adding -1 in almost all calls of that function is looking weird 🤔

Comment on lines +224 to +226
// getOrCreateMetricFamily returns the metric family for the given metric name and scope,
// and true if an existing family was found.
func (t *transaction) getOrCreateMetricFamily(key resourceKey, scope scopeID, mn string) *metricFamily {
func (t *transaction) getOrCreateMetricFamily(key resourceKey, scope scopeID, mn string, schema int32) *metricFamily {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something feels off with this strategy of adding a schema as an argument 🤔

What if we took a similar approach to the one we use when detecting a NativeHistogram? When we call AppendHistogram, we toggle a flag in the transaction object called addingNativeHistogram. Couldn't we add the flag there, too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes that sounds good too

Comment on lines +301 to 307
var schema int32
if h != nil {
schema = h.Schema
} else if fh != nil {
schema = fh.Schema
}
t.addingNativeHistogram = true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the place I had in mind in the comment above. Couldn't we add a second flag here, e.g.

	var schema int32
	if h != nil {
		schema = h.Schema
	} else if fh != nil {
		schema = fh.Schema
	}
	t.addingNativeHistogram = true
	if schema == -53 {
		t.addingNHCB = true
	}

If we use that flag when calling getOrCreateMetricFamily, then the schema argument isn't needed and we don't need to add -1 in weird places 😅

Comment on lines 247 to 252
if curMf.mtype == pmetric.MetricTypeHistogram && mfKey.isExponentialHistogram {
curMf.mtype = pmetric.MetricTypeExponentialHistogram
// Don't convert NHCB to ExponentialHistogram.
if schema != -53 {
curMf.mtype = pmetric.MetricTypeExponentialHistogram
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this could become a single if...?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, missed that

@jelly-afk

Copy link
Copy Markdown
Contributor Author

oh changelog got new validations, will fix

@jelly-afk

Copy link
Copy Markdown
Contributor Author

ah chlog files from prev release

@jelly-afk jelly-afk force-pushed the convert-nhcb-explicit-histogram-metric branch from cdd2b3c to 1780f80 Compare October 8, 2025 13:25

@ArthurSens ArthurSens left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but I'd love it if we could wait for @dashpole or @krajorama's review as well

Comment thread receiver/prometheusreceiver/internal/transaction.go Outdated
@dashpole

dashpole commented Oct 10, 2025

Copy link
Copy Markdown
Contributor

huh, something strange happened with my comments. Maybe I didn't actually leave them? Ignore them for now

Edit: Re-posted it below

Comment thread receiver/prometheusreceiver/internal/metricfamily.go
jelly-afk and others added 2 commits October 10, 2025 20:54
Co-authored-by: David Ashpole <dashpole@google.com>
@songy23 songy23 merged commit 2d31449 into open-telemetry:main Oct 14, 2025
197 of 199 checks passed
@github-actions github-actions Bot added this to the next release milestone Oct 14, 2025
@otelbot

otelbot Bot commented Oct 14, 2025

Copy link
Copy Markdown
Contributor

Thank you for your contribution @jelly-afk! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help.

ChrsMark pushed a commit to ChrsMark/opentelemetry-collector-contrib that referenced this pull request Oct 20, 2025
…ic (open-telemetry#43095)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
added prometheus NHCB (native histogram with custom buckets) to otel
explicit histogram conversion.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes open-telemetry#41131 

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Added `TestMetricGroupData_toNHCBDistributionUnitTest` test function to
test the conversion.

---------

Co-authored-by: David Ashpole <dashpole@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

receiver/prometheus Prometheus receiver

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[receiver/prometheusreceiver] does not handle native histograms with custom bucket correctly

5 participants