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

[Prometheus] Update prometheus histogram #36537

Merged
merged 14 commits into from
Sep 22, 2023
Merged

[Prometheus] Update prometheus histogram #36537

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
cfce8fb
Update prometheus histogram.
constanca-m Sep 8, 2023
7625f1d
Merge branch 'main' into fix-prometheus-histogram
constanca-m Sep 8, 2023
ad87224
Update tests.
constanca-m Sep 8, 2023
58b6842
Fix error.
constanca-m Sep 8, 2023
4708d81
Revert change on expected files.
constanca-m Sep 8, 2023
dd3f323
Merge branch 'main' into fix-prometheus-histogram
constanca-m Sep 21, 2023
c27e8e6
Add missing comments.
constanca-m Sep 21, 2023
9e050a1
Add unit test.
constanca-m Sep 21, 2023
561dfd7
Add unit test.
constanca-m Sep 21, 2023
31ece69
Run mage check.
constanca-m Sep 21, 2023
3113763
Update changelog.
constanca-m Sep 22, 2023
c474d64
Add comment.
constanca-m Sep 22, 2023
ad81af3
Update CHANGELOG-developer.next.asciidoc
constanca-m Sep 22, 2023
a41d066
Merge branch 'main' into fix-prometheus-histogram
constanca-m Sep 22, 2023
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
45 changes: 30 additions & 15 deletions metricbeat/helper/prometheus/textparse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ func (m *Info) GetValue() int64 {
}
return 0
}

func (m *Info) HasValidValue() bool {
return m != nil && *m.Value == 1
}
Expand Down Expand Up @@ -395,6 +396,10 @@ func summaryMetricName(name string, s float64, qv string, lbls string, t *int64,
return name, metric
}

/*
histogramMetricName returns the metric name without the suffix and its histogram.
OpenMetric suffixes: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#suffixes.
ChrsMark marked this conversation as resolved.
Show resolved Hide resolved
*/
func histogramMetricName(name string, s float64, qv string, lbls string, t *int64, isGaugeHistogram bool, e *exemplar.Exemplar, histogramsByName map[string]map[string]*OpenMetric) (string, *OpenMetric) {
var histogram = &Histogram{}
var bucket = []*Bucket{}
Expand All @@ -404,21 +409,18 @@ func histogramMetricName(name string, s float64, qv string, lbls string, t *int6
case isCount(name):
u := uint64(s)
histogram.SampleCount = &u
name = name[:len(name)-6]
name = strings.TrimSuffix(name, suffixCount)
case isSum(name):
histogram.SampleSum = &s
name = name[:len(name)-4]
name = strings.TrimSuffix(name, suffixSum)
case isGaugeHistogram && isGCount(name):
u := uint64(s)
histogram.SampleCount = &u
name = name[:len(name)-7]
name = strings.TrimSuffix(name, suffixGCount)
case isGaugeHistogram && isGSum(name):
histogram.SampleSum = &s
name = name[:len(name)-5]
default:
if isBucket(name) {
name = name[:len(name)-7]
}
name = strings.TrimSuffix(name, suffixGSum)
case isBucket(name):
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any unit-tests that cover these changes? It's hard to understand if something might break here in any corner case or whatever?

If not would you mind adding some tests? Otherwise we can exclude these changes from the patch if not needed for the fix. Improving code should always go along with adding tests ensuring that functionality is not broken :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added one more test for this exact function. Do you think that is enough?

Copy link
Member

Choose a reason for hiding this comment

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

I think that for the new functionality the added test is fine but what about the existing functionality that is touched here? How we can be sure that the code which is changed (improved) is not broken?
I'm talking mostly about the changes like the one at https://github.com/elastic/beats/pull/36537/files#diff-d66f15900f5676672ddf353458a9373cf8b1ab8e237c473191e4a64ae93cbba2L407-R416.

So the point here is that if we want to improve the code we should ensure that we don't break it. So to my mind we should either:

  1. ensure that we have tests for these lines that still work
  2. if we don't have tests then add them along with the corresponding code changes.
  3. if we cannot add tests maybe it's safer to avoid any code changes that might be prone to errors

Could you verify the above cases please?

Copy link
Contributor Author

@constanca-m constanca-m Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We never had tests for those cases. Do you mean adding a test case for every type (histogram, histogram gauge etc)? I only changed it to strings.TrimSuffix because then we know what is happening instead of defining the lenght of the suffix and then subtracting like we were doing.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean adding a test case for every type (histogram, histogram gauge etc)?

Ideally yes since we touch these specific lines of code. Otherwise the improvement seems to be incomplete to me and potentially prone to errors :).
Maybe that's out of the scope of this PR but the generic idea is what I mention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. So should I try to add new tests for this PR or should we leave it to a different one?

Copy link
Member

Choose a reason for hiding this comment

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

That would be nice since we are on this. Otherwise I doubt it will happen at any time soon.
Another option would be to exclude the specific changes/improvements that are not related to this specific fix and add them later in a follow-up along with tests. That would unblock this one and ensures the quality as well. It's up to you to decide.

f, err := strconv.ParseFloat(qv, 64)
if err != nil {
f = math.MaxUint64
Expand All @@ -433,6 +435,9 @@ func histogramMetricName(name string, s float64, qv string, lbls string, t *int6
}
bkt.Exemplar = e
}
name = strings.TrimSuffix(name, suffixBucket)
default:
return "", nil
}

_, k := histogramsByName[name]
Expand Down Expand Up @@ -537,15 +542,19 @@ func ParseMetricFamilies(b []byte, contentType string, ts time.Time) ([]*MetricF
var lbls strings.Builder
lbls.Grow(len(mets))
var labelPairs = []*labels.Label{}
var qv string // value of le or quantile label
for _, l := range lset.Copy() {
if l.Name == labels.MetricName {
continue
}

if l.Name != model.QuantileLabel && l.Name != labels.BucketLabel { // quantile and le are special labels handled below

if l.Name != model.QuantileLabel && l.Name != labels.BucketLabel {
constanca-m marked this conversation as resolved.
Show resolved Hide resolved
lbls.WriteString(l.Name)
lbls.WriteString(l.Value)
} else if l.Name == model.QuantileLabel {
constanca-m marked this conversation as resolved.
Show resolved Hide resolved
qv = lset.Get(model.QuantileLabel)
} else {
qv = lset.Get(labels.BucketLabel)
}
n := l.Name
v := l.Value
Expand All @@ -568,8 +577,8 @@ func ParseMetricFamilies(b []byte, contentType string, ts time.Time) ([]*MetricF
var counter = &Counter{Value: &v}
mn := lset.Get(labels.MetricName)
metric = &OpenMetric{Name: &mn, Counter: counter, Label: labelPairs}
if isTotal(metricName) && contentType == OpenMetricsType { // Remove suffix _total, get lookup metricname
ChrsMark marked this conversation as resolved.
Show resolved Hide resolved
lookupMetricName = metricName[:len(metricName)-6]
if isTotal(metricName) && contentType == OpenMetricsType {
lookupMetricName = strings.TrimSuffix(metricName, suffixTotal)
} else {
lookupMetricName = metricName
}
Expand All @@ -583,7 +592,7 @@ func ParseMetricFamilies(b []byte, contentType string, ts time.Time) ([]*MetricF
metric = &OpenMetric{Name: &metricName, Info: info, Label: labelPairs}
lookupMetricName = metricName
case textparse.MetricTypeSummary:
lookupMetricName, metric = summaryMetricName(metricName, v, lset.Get(model.QuantileLabel), lbls.String(), &t, summariesByName)
lookupMetricName, metric = summaryMetricName(metricName, v, qv, lbls.String(), &t, summariesByName)
metric.Label = labelPairs
if !isSum(metricName) {
continue
Expand All @@ -593,7 +602,10 @@ func ParseMetricFamilies(b []byte, contentType string, ts time.Time) ([]*MetricF
if hasExemplar := parser.Exemplar(&e); hasExemplar {
exm = &e
}
lookupMetricName, metric = histogramMetricName(metricName, v, lset.Get(labels.BucketLabel), lbls.String(), &t, false, exm, histogramsByName)
lookupMetricName, metric = histogramMetricName(metricName, v, qv, lbls.String(), &t, false, exm, histogramsByName)
if metric == nil { // metric name does not have a suffix supported for the type histogram
constanca-m marked this conversation as resolved.
Show resolved Hide resolved
continue
}
metric.Label = labelPairs
if !isSum(metricName) {
continue
Expand All @@ -603,7 +615,10 @@ func ParseMetricFamilies(b []byte, contentType string, ts time.Time) ([]*MetricF
if hasExemplar := parser.Exemplar(&e); hasExemplar {
exm = &e
}
lookupMetricName, metric = histogramMetricName(metricName, v, lset.Get(labels.BucketLabel), lbls.String(), &t, true, exm, histogramsByName)
lookupMetricName, metric = histogramMetricName(metricName, v, qv, lbls.String(), &t, true, exm, histogramsByName)
if metric == nil { // metric name does not have a suffix supported for the type gauge histogram
continue
}
metric.Label = labelPairs
metric.Histogram.IsGaugeHistogram = true
if !isGSum(metricName) {
Expand Down
Loading