Prometheus compatible histograms

[clementnuss]

as a follow-up/alternative to #45 and to solve #28, I'm starting this PR which adds support for OTLP histograms (the common le - static bucket type of histograms, not the complex exponential histograms/native histograms).

I know that VM focus is on log based buckets and really appreciate those, but having the opportunity to use "normal" histograms with this library would be greatly appreciated, as documented in #28

before working too much on this, I'd like to know whether support for normal / OTLP style buckets could be considered or not. I don't want to spend too much time on this PR if at the end @valyala @hagen1778 you decide against such a feature.

I'd also like to start a discussion on the name. I picked OTLPHistogram in reference to the histograms described in the OpenTelemetry Protocol, but perhaps the naming is bad.
[EDIT]: I think that CompatibleHistogram would make more sense than OTLPHistogram, had not thought about it until after I started implementing the functions

currently the basic functionality is implemented, and a testcase covers a simple usage. if there is a decision in favor of this PR, I will add the missing testcases and complete the implementation (add a function for custom buckets/upper bounds for example)

[clementnuss]

after a bit of reflexion, I believe it could also be possible to add a HistogramType field in here:

metrics/histogram.go

Line 48 in 704aa40

type Histogram struct {

that way we could have several implementations and depending on the histogram type use e.g. compatible/le-style or vmrange histograms. this would spare the choice of a new name for the histogram.

type HistogramType int

const (
	VictoriaMetrics = iota // or OTLP, Static, ...
        Compatible // or vmrange, log-based, VM, ...
)

[hagen1778]

Hello @clementnuss! Thanks for the initiative!

I think the rationale you provided, and the linked issues, all make sense. It would be beneficial for many users and for this package to support the traditional histograms with static list of buckets.
The only thing that concerns me is a picked name :) Reading the title about OTLP histogram type made me thinking me in another way. To me, the traditional histogram implementation is more like Prometheus type. Which, I believe, would be clearer to the users. Can we switch to this name if you agree?

after a bit of reflexion, I believe it could also be possible to add a HistogramType field in here:

To keep things simple, I'd suggest to have it as completely separate structure. It should be simpler from API and implementation perspective.

[clementnuss]

My pleasure 🙃 I will change the name and complete the implementation.

which one works best?

• PrometheusHistogram 🚀
• CompatibleHistogram 👍🏼

[clementnuss]

the PR is ready, let me know what needs to be fixed. I wasn't sure whether I should implement the set.Get... methods as they are less performant than .New... but I did implement those anyway, as they were implemented already for the other metric type.

[codecov]

Codecov Report

Attention: Patch coverage is 93.87755% with 9 lines in your changes missing coverage. Please review.

Project coverage is 87.53%. Comparing base (704aa40) to head (5bfbe76).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
prometheus_histogram.go	95.65%	5 Missing ⚠️
set.go	87.50%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   86.89%   87.53%   +0.64%     
==========================================
  Files          11       12       +1     
  Lines        1450     1597     +147     
==========================================
+ Hits         1260     1398     +138     
- Misses        142      149       +7     
- Partials       48       50       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[f41gh7]

Overall looks good to me, please see few minor comments

[clementnuss]

@f41gh7 reviewing the coverage I noticed I had forgotten to delete the implementation of .Merge, I had only removed the tests. this is now fixed and I force-pushed.

[hagen1778]

Looks good! See some comments

[clementnuss]

did implement most of the suggestions. added a few questions as a result!

[clementnuss]

and I already switched kubenurse to VictoriaMetrics/metrics go library with the prometheus histogram. seems that it works perfectly!

[hagen1778]

Thanks for your contribution @clementnuss!
I hope to review&merge it this Friday.

[hagen1778]

@clementnuss please see review updates in 95d307d

[hagen1778]

Thanks for the contribution @clementnuss !