Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exemplars for native histograms #1686

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add exemplars for native histograms #1686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a6a456
Add exemplars for native histograms
shivanthzen Nov 17, 2024
537b51f
Merge branch 'main' into nativehistogram_exemplars
shivanthzen Jan 7, 2025
67dbd86
Add doc string for NewMetricWithExemplars
shivanthzen Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions prometheus/metric.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,12 @@ func (m *withExemplarsMetric) Write(pb *dto.Metric) error {
pb.Counter.Exemplar = m.exemplars[len(m.exemplars)-1]
case pb.Histogram != nil:
for _, e := range m.exemplars {
if pb.Histogram.Schema != nil {
if *pb.Histogram.Schema > math.MinInt32 && e.GetTimestamp() != nil {
pb.Histogram.Exemplars = append(pb.Histogram.Exemplars, e)
}
continue
Copy link
Member

Choose a reason for hiding this comment

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

If a classic and a native histogram is exposed at the same time (for the scraper to pick), we should have the exemplars on both. Maybe the following works for that:

Suggested change
continue
if len(pb.Histogram.Bucket) == 0 {
// Don't proceed to classic buckets if there are none.
continue
}

We do have to check if there are already buckets defined, because otherwise there will be an Inf bucket created just for the histograms.

}
// pb.Histogram.Bucket are sorted by UpperBound.
i := sort.Search(len(pb.Histogram.Bucket), func(i int) bool {
return pb.Histogram.Bucket[i].GetUpperBound() >= e.GetValue()
Expand Down Expand Up @@ -227,6 +233,7 @@ type Exemplar struct {
// Only last applicable exemplar is injected from the list.
// For example for Counter it means last exemplar is injected.
// For Histogram, it means last applicable exemplar for each bucket is injected.
// Note that for a nativeHistogram, all valid exemplars are injected.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Note that for a nativeHistogram, all valid exemplars are injected.
// For a Native Histogram, all valid exemplars are injected.

//
// NewMetricWithExemplars works best with MustNewConstMetric and
// MustNewConstHistogram, see example.
Expand Down
28 changes: 28 additions & 0 deletions prometheus/metric_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ package prometheus
import (
"math"
"testing"
"time"

dto "github.com/prometheus/client_model/go"

"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/timestamppb"
)

func TestBuildFQName(t *testing.T) {
Expand Down Expand Up @@ -90,3 +92,29 @@ func TestWithExemplarsMetric(t *testing.T) {
}
})
}

func TestWithExemplarsNativeHistogramMetric(t *testing.T) {
t.Run("histogram", func(t *testing.T) {
// Create a constant histogram from values we got from a 3rd party telemetry system.
h := MustNewConstNativeHistogram(
NewDesc("http_request_duration_seconds", "A histogram of the HTTP request durations.", nil, nil),
10, 12.1, map[int]int64{1: 7, 2: 1, 3: 2}, map[int]int64{}, 0, 2, 0.2, time.Date(
2009, 11, 17, 20, 34, 58, 651387237, time.UTC))
m := &withExemplarsMetric{Metric: h, exemplars: []*dto.Exemplar{
{Value: proto.Float64(2000.0), Timestamp: timestamppb.New(time.Date(2009, 11, 17, 20, 34, 58, 3243244, time.UTC))},
Copy link
Member

Choose a reason for hiding this comment

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

We should also test the following cases:

  • More than one exemplar.
  • An exemplar without timestamp (to verify that a timestamp is added automatically then, because exemplars without timestamps are not allowed for native histograms).
  • A histogram with both classic and native buckets. (I believe for that you might have to create an ad-hoc implementation of Metric that spits out a custom-made dto.Metric.)

}}
metric := dto.Metric{}
if err := m.Write(&metric); err != nil {
t.Fatal(err)
}
if want, got := 1, len(metric.GetHistogram().Exemplars); want != got {
t.Errorf("want %v, got %v", want, got)
}

for _, b := range metric.GetHistogram().Bucket {
if b.Exemplar != nil {
t.Error("Not expecting exemplar for bucket")
}
}
})
}
Loading