[1617] Add ConstnativeHistogram #1654

shivanthzen · 2024-10-16T12:18:54Z

Fixes: #1617
Add exported ConstNativeHistogram - this can be used to build native histograms if one already has the relevant data without calling the observe function. A viable use case is while converting exponential histograms to nativehistograms.

shivanthzen · 2024-10-16T12:47:35Z

@ArthurSens Maybe you can take a look at this PR.

ArthurSens

Thanks for working on this! I've added a few comments but I'm not a native histogram expert, it would be nice if @beorn7 or @krajorama could also take a look here.

question: Have you tried using your commit on OTel's side to make sure you can translate their format into ours?

prometheus/histogram.go

prometheus/histogram_test.go

prometheus/histogram.go

beorn7 · 2024-10-17T16:09:31Z

Will have a look ASAP (which won't be before next week, sorry).

krajorama

Hi, I understand the use case of wanting to expose OTEL exponential histograms from the Prometheus exporter in the collector. How would you use it actually? Constantly register and unregister the metric to update the exposed values? Or just use it's Write() method to get to the data?

In any case, the naming is going to be weird as you're not actually using it as a const :)

If I understand correctly, currently it will only work for counter histograms, not gauge histograms due to limitations in client_golang.

I'm a little afraid of abuse of this interface, but don't see true stoppers for now, will consult @beorn7 .

prometheus/histogram.go

krajorama · 2024-10-21T07:28:34Z

prometheus/histogram.go

+	zeroBucket      uint64
+}
+
+func NewConstNativeHistogram(desc *Desc, count uint64, sum float64, postiveBuckets, negativeBuckets map[int]int64, zeroBucket uint64,


Needs doc string to explain the arguments and the fact that this is exposed as a counter type histogram, treated as a counter on scrape.

In fact, I do actually think that exposing a gauge histogram will be one of the common use cases for using this function. So let's maybe add a bool argument here so that the user can switch between counter and gauge histogram.

Style nit: Prometheus code usually does a line break after the opening parenthesis in case of multiline argument lists.

Tried this. Wouldn't this blow up the argument list? Is it better to create a seperate a constNaticveGuageHistogram ?

I'm still unclear about including a gauge histogram . As per my understanding , we will need to add postiveBuckets, negativeBuckets of type map[int]float64 as an argument (or use a generic parameter). Adding a boolean parameter will not suffice.

It sounds like you are thinking about a float histogram. That's something we might want to add later (ideally together with the option of creating classic float histograms). However, the use case for float histograms in instrumentation is very rare.

I am in fact talking about a gauge histogram. It merely requires to set a different MetricType in the MetricFamily. However, I've just realized that we currently have no way of communicating the difference of counter histogram (i.e. the "normal" histogram) and gauge histogram during metrics collection. For that, we needed Desc to have a metric type field (which was the plan for quite some time).

So I'm pretty sure we cannot solve this problem in this PR, because it needed too much re-architecting.

But if we assume that the difference between counter histogram and gauge histogram will be in the Desc, we actually don't need a bool parameter here, but just have to later allow the option of creating "typed" Descs.

In conclusion, let's not do gauge histograms right now.

That's true. I was indeed confused about a float histogram and gauge histogram.

beorn7

Thanks @shivanthzen for tackling this, and thanks for the review so far @krajorama and @ArthurSens . If I haven't commented on a previous review comment, I agree with it. :)

beorn7 · 2024-10-22T17:20:00Z

prometheus/histogram.go

+	zeroBucket      uint64
+}
+
+func NewConstNativeHistogram(desc *Desc, count uint64, sum float64, postiveBuckets, negativeBuckets map[int]int64, zeroBucket uint64,


In fact, I do actually think that exposing a gauge histogram will be one of the common use cases for using this function. So let's maybe add a bool argument here so that the user can switch between counter and gauge histogram.

beorn7 · 2024-10-22T17:21:26Z

prometheus/histogram.go

+	zeroBucket      uint64
+}
+
+func NewConstNativeHistogram(desc *Desc, count uint64, sum float64, postiveBuckets, negativeBuckets map[int]int64, zeroBucket uint64,


Style nit: Prometheus code usually does a line break after the opening parenthesis in case of multiline argument lists.

prometheus/histogram_test.go

prometheus/histogram.go

shivanthzen · 2024-10-24T19:09:05Z

Thanks for the comments. I'll resolve them this weekend

ArthurSens

I had just a quick look today :)

prometheus/histogram.go

ArthurSens · 2024-10-28T22:13:36Z

prometheus/histogram.go

+	if err := validateCount(count, negativeBuckets, positiveBuckets, zeroBucket); err != nil {
+		return nil, err
+	}


Do we need to validate Sum as well?

Not really. Sum could by anything. I mean, there could be consistency checks like the sum cannot be negative if there are only positive observations, or the sum has to be zero without any observations, and many more. But I don't know if that would be very helpful. We should only check for conditions that would make Prometheus reject the histogram upon ingestion.

ArthurSens · 2024-10-28T22:14:46Z

prometheus/histogram.go

+	return nil
+}
+
+func makeBucketsAny(buckets map[int]int64) ([]*dto.BucketSpan, []int64) {


hey, could you clarify what Any means here?

Probably "positive or negative".
I would just call it makeBuckets.

+1 for makeBuckets

Hmm, makebuckets alreadyexists in the namespace. I could do makeBucketfrommap

beorn7

Looks good in general.

Despite the frequent nit pickings about naming etc., I haven't yet checked the technical correctness very deeply yet (although it looks fine as far as I can see right now).

prometheus/histogram.go

beorn7 · 2024-10-29T15:19:42Z

prometheus/histogram.go

+	if err := validateCount(count, negativeBuckets, positiveBuckets, zeroBucket); err != nil {
+		return nil, err
+	}


Not really. Sum could by anything. I mean, there could be consistency checks like the sum cannot be negative if there are only positive observations, or the sum has to be zero without any observations, and many more. But I don't know if that would be very helpful. We should only check for conditions that would make Prometheus reject the histogram upon ingestion.

beorn7 · 2024-10-29T15:20:57Z

prometheus/histogram.go

+	return nil
+}
+
+func makeBucketsAny(buckets map[int]int64) ([]*dto.BucketSpan, []int64) {


Probably "positive or negative".
I would just call it makeBuckets.

prometheus/histogram.go

ArthurSens

LGTM, but let's wait for an approval from Beorn or Krajo as well

Thanks for the work!

ArthurSens · 2024-10-30T13:11:12Z

Oh, looks like some commits weren't signed and there's one test failing 🤔

The test seems to be flaky, it's also failing in other PRs

shivanthzen · 2024-11-01T13:42:04Z

I can sign the commits once the PR is approved, It seems signing will need a force push

beorn7 · 2024-11-02T12:12:40Z

I can sign the commits once the PR is approved, It seems signing will need a force push

Yes, it changes the commits and therefore needs a force-push. However, it is essential to sign before approval. That's kind of the point.

beorn7

Thanks for addressing the feedback.

I have no realized something about exemplars we had not discussed before.

I have also realized we cannot do gauge histograms right now. Needs more discussion and planning, which is out of scope of this PR.

prometheus/histogram.go

prometheus/histogram_test.go

shivanthzen · 2024-11-03T19:42:32Z

Force pushed with signed commits.

ArthurSens · 2024-11-04T21:00:31Z

@shivanthzen, looks like you accidentally added unrelated commits to your PR, could you keep only the ones that you've authored here?

shivanthzen · 2024-11-06T07:29:15Z

@ArthurSens Removed other commits from the branch

krajorama

couple of minor comments

prometheus/histogram.go

kakkoyun · 2024-11-11T14:32:40Z

@beorn7 could you have another look at it?

beorn7 · 2024-11-12T15:57:24Z

This is very close to the top of my review queue now. Will have a look soon, hopefully today.

beorn7

Just some minor things about doc comments left.

prometheus/histogram.go

beorn7 · 2024-11-13T10:34:04Z

Looks good to me, but there are lint warnings now. (They are coming from a recently added check, but they are still relevant.) Also, DCO signing is missing. @shivanthzen could you address the lint warnings and sign the commits? (Feel free to squash commits into fewer or just one before doing that.)

Signed-off-by: Shivanth <[email protected]> Apply suggestions from code review Co-authored-by: Arthur Silva Sens <[email protected]> Signed-off-by: Shivanth <[email protected]> Fix references and existing tests Signed-off-by: Shivanth <[email protected]> Review comments Signed-off-by: Shivanth <[email protected]> fmt Signed-off-by: Shivanth <[email protected]> validation Signed-off-by: Shivanth <[email protected]> Count validation Signed-off-by: Shivanth <[email protected]> Validation Signed-off-by: Shivanth <[email protected]> Review comments Signed-off-by: Shivanth <[email protected]> Space formatting Signed-off-by: Shivanth <[email protected]> Review comments Signed-off-by: Shivanth <[email protected]> Rename SyncMaptoMap -> SyncMapToMap Signed-off-by: Shivanth <[email protected]> Remove exemplars from parameters for constNativeHistogram function Signed-off-by: Shivanth <[email protected]> Update prometheus/histogram.go Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Shivanth MP <[email protected]> Update prometheus/histogram.go Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Shivanth MP <[email protected]> Update prometheus/histogram.go Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Shivanth MP <[email protected]> Update prometheus/histogram.go Co-authored-by: Björn Rabenstein <[email protected]> Signed-off-by: Shivanth MP <[email protected]> Review comments Signed-off-by: Shivanth <[email protected]> Lint fix Signed-off-by: Shivanth <[email protected]>

shivanthzen · 2024-11-13T16:51:21Z

@beorn7 Done

beorn7

Thank you very much. I leave it to the maintainers to do the final approval and merge.

ArthurSens

LGTM, thank you all for the work here :)

Follow-up here would be to tackle the addition of exemplars!

beorn7 · 2024-11-14T14:24:20Z

Follow-up here would be to tackle the addition of exemplars!

To clarify: This means making withExemplarsMetric.Write aware of native histogram exemplars. Then, we only need to update the doc comment of NewMetricWithExemplar and everything should work fine.

shivanthzen force-pushed the constNativeHistogram branch from 85c4d89 to 473720d Compare October 16, 2024 12:31

ArthurSens reviewed Oct 17, 2024

View reviewed changes

shivanthzen force-pushed the constNativeHistogram branch 2 times, most recently from 96eeddd to ad21924 Compare October 18, 2024 10:04

krajorama reviewed Oct 21, 2024

View reviewed changes

beorn7 requested changes Oct 22, 2024

View reviewed changes

ArthurSens reviewed Oct 28, 2024

View reviewed changes

beorn7 requested changes Oct 29, 2024

View reviewed changes

ArthurSens approved these changes Oct 30, 2024

View reviewed changes

beorn7 requested changes Nov 2, 2024

View reviewed changes

shivanthzen force-pushed the constNativeHistogram branch from afbdefd to f900abb Compare November 3, 2024 19:40

shivanthzen force-pushed the constNativeHistogram branch from 356875e to 698a313 Compare November 3, 2024 19:52

shivanthzen force-pushed the constNativeHistogram branch from 698a313 to d7d3022 Compare November 6, 2024 07:26

krajorama reviewed Nov 8, 2024

View reviewed changes

prometheus/histogram.go Outdated Show resolved Hide resolved

prometheus/histogram.go Outdated Show resolved Hide resolved

prometheus/histogram.go Show resolved Hide resolved

prometheus/histogram.go Outdated Show resolved Hide resolved

kakkoyun requested a review from beorn7 November 11, 2024 14:32

kakkoyun added this to the v1.21.0 milestone Nov 11, 2024

beorn7 requested changes Nov 12, 2024

View reviewed changes

prometheus/histogram.go Show resolved Hide resolved

prometheus/histogram.go Outdated Show resolved Hide resolved

prometheus/histogram.go Show resolved Hide resolved

shivanthzen force-pushed the constNativeHistogram branch from b6bed96 to ae84979 Compare November 13, 2024 13:19

beorn7 approved these changes Nov 13, 2024

View reviewed changes

ArthurSens approved these changes Nov 13, 2024

View reviewed changes

ArthurSens merged commit 291b0b0 into prometheus:main Nov 14, 2024
10 checks passed

beorn7 mentioned this pull request Nov 14, 2024

Consider supporting constant native histograms #1617

Open

This was referenced Nov 14, 2024

Support Prometheus native histograms open-telemetry/opentelemetry-go#5777

Open

[prometheusexporter] Expose native histograms open-telemetry/opentelemetry-collector-contrib#33703

Open

[1617] Add ConstnativeHistogram #1654

[1617] Add ConstnativeHistogram #1654

Conversation

shivanthzen commented Oct 16, 2024 • edited Loading

shivanthzen commented Oct 16, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

beorn7 commented Oct 17, 2024

krajorama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanthzen Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanthzen commented Oct 24, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens commented Oct 30, 2024

shivanthzen commented Nov 1, 2024

beorn7 commented Nov 2, 2024

beorn7 left a comment

Choose a reason for hiding this comment

shivanthzen commented Nov 3, 2024

ArthurSens commented Nov 4, 2024

shivanthzen commented Nov 6, 2024 • edited Loading

krajorama left a comment

Choose a reason for hiding this comment

kakkoyun commented Nov 11, 2024

beorn7 commented Nov 12, 2024

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 commented Nov 13, 2024

shivanthzen commented Nov 13, 2024

beorn7 left a comment

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

beorn7 commented Nov 14, 2024

shivanthzen commented Oct 16, 2024 •

edited

Loading

shivanthzen Nov 1, 2024 •

edited

Loading

shivanthzen commented Nov 6, 2024 •

edited

Loading