Skip to content

Commit

Permalink
[chore] prometheusremotewrite.FromMetrics: Fix createLabels closures …
Browse files Browse the repository at this point in the history 
…wrt. extra labels (open-telemetry#31635)

**Description:** <Describe what has changed.>
The functions for adding histogram and sum data points have a logic
error in their `createLabels` closures. The two `createLabels` closures
each take an optional "extras" slice, of label pairs. However, the loop
logic doesn't advance to the next pair, just the first label value in
fact, so if there were more than one extra pair (there isn't currently),
there would be a bug.

I'm refactoring the two `createLabels` closures into a free function by
the same name, that loops correctly over `extras` arguments and handles
if they are of uneven length (ignoring the unpaired `extra`). Also
including a comprehensive test suite for `createLabels`.

Marking the PR as a chore, since as described above, the logic error
should currently not cause a bug.

**Link to tracking Issue:** <Issue number if applicable>

**Testing:** <Describe what testing was performed and which tests were
added.>
I've tested locally, there shouldn't be any practical changes since the
`createLabels` closures are only called with one extra label pair at
most.

**Documentation:** <Describe the documentation added.>

---------

Signed-off-by: Arve Knudsen <[email protected]>
  • Loading branch information
@aknuds1
aknuds1 authored Mar 28, 2024
1 parent 0a12ede commit b3ffb62
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 38 deletions.
65 changes: 27 additions & 38 deletions pkg/translator/prometheusremotewrite/helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,21 +253,6 @@ func addSingleHistogramDataPoint(pt pmetric.HistogramDataPoint, resource pcommon
timestamp := convertTimeStamp(pt.Timestamp())
baseLabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels)

createLabels := func(nameSuffix string, extras ...string) []prompb.Label {
extraLabelCount := len(extras) / 2
labels := make([]prompb.Label, len(baseLabels), len(baseLabels)+extraLabelCount+1) // +1 for name
copy(labels, baseLabels)

for extrasIdx := 0; extrasIdx < extraLabelCount; extrasIdx++ {
labels = append(labels, prompb.Label{Name: extras[extrasIdx], Value: extras[extrasIdx+1]})
}

// sum, count, and buckets of the histogram should append suffix to baseName
labels = append(labels, prompb.Label{Name: model.MetricNameLabel, Value: baseName + nameSuffix})

return labels
}

// If the sum is unset, it indicates the _sum metric point should be
// omitted
if pt.HasSum() {
Expand All @@ -280,7 +265,7 @@ func addSingleHistogramDataPoint(pt pmetric.HistogramDataPoint, resource pcommon
sum.Value = math.Float64frombits(value.StaleNaN)
}

sumlabels := createLabels(sumStr)
sumlabels := createLabels(baseName+sumStr, baseLabels)
addSample(tsMap, sum, sumlabels, metric.Type().String())

}
Expand All @@ -294,7 +279,7 @@ func addSingleHistogramDataPoint(pt pmetric.HistogramDataPoint, resource pcommon
count.Value = math.Float64frombits(value.StaleNaN)
}

countlabels := createLabels(countStr)
countlabels := createLabels(baseName+countStr, baseLabels)
addSample(tsMap, count, countlabels, metric.Type().String())

// cumulative count for conversion to cumulative histogram
Expand All @@ -316,7 +301,7 @@ func addSingleHistogramDataPoint(pt pmetric.HistogramDataPoint, resource pcommon
bucket.Value = math.Float64frombits(value.StaleNaN)
}
boundStr := strconv.FormatFloat(bound, 'f', -1, 64)
labels := createLabels(bucketStr, leStr, boundStr)
labels := createLabels(baseName+bucketStr, baseLabels, leStr, boundStr)
sig := addSample(tsMap, bucket, labels, metric.Type().String())

bucketBounds = append(bucketBounds, bucketBoundsData{sig: sig, bound: bound})
Expand All @@ -330,7 +315,7 @@ func addSingleHistogramDataPoint(pt pmetric.HistogramDataPoint, resource pcommon
} else {
infBucket.Value = float64(pt.Count())
}
infLabels := createLabels(bucketStr, leStr, pInfStr)
infLabels := createLabels(baseName+bucketStr, baseLabels, leStr, pInfStr)
sig := addSample(tsMap, infBucket, infLabels, metric.Type().String())

bucketBounds = append(bucketBounds, bucketBoundsData{sig: sig, bound: math.Inf(1)})
Expand All @@ -339,7 +324,7 @@ func addSingleHistogramDataPoint(pt pmetric.HistogramDataPoint, resource pcommon
// add _created time series if needed
startTimestamp := pt.StartTimestamp()
if settings.ExportCreatedMetric && startTimestamp != 0 {
labels := createLabels(createdSuffix)
labels := createLabels(baseName+createdSuffix, baseLabels)
addCreatedTimeSeriesIfNeeded(tsMap, labels, startTimestamp, pt.Timestamp(), metric.Type().String())
}
}
Expand Down Expand Up @@ -452,20 +437,6 @@ func addSingleSummaryDataPoint(pt pmetric.SummaryDataPoint, resource pcommon.Res
timestamp := convertTimeStamp(pt.Timestamp())
baseLabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels)

createLabels := func(name string, extras ...string) []prompb.Label {
extraLabelCount := len(extras) / 2
labels := make([]prompb.Label, len(baseLabels), len(baseLabels)+extraLabelCount+1) // +1 for name
copy(labels, baseLabels)

for extrasIdx := 0; extrasIdx < extraLabelCount; extrasIdx++ {
labels = append(labels, prompb.Label{Name: extras[extrasIdx], Value: extras[extrasIdx+1]})
}

labels = append(labels, prompb.Label{Name: model.MetricNameLabel, Value: name})

return labels
}

// treat sum as a sample in an individual TimeSeries
sum := &prompb.Sample{
Value: pt.Sum(),
Expand All @@ -475,7 +446,7 @@ func addSingleSummaryDataPoint(pt pmetric.SummaryDataPoint, resource pcommon.Res
sum.Value = math.Float64frombits(value.StaleNaN)
}
// sum and count of the summary should append suffix to baseName
sumlabels := createLabels(baseName + sumStr)
sumlabels := createLabels(baseName+sumStr, baseLabels)
addSample(tsMap, sum, sumlabels, metric.Type().String())

// treat count as a sample in an individual TimeSeries
Expand All @@ -486,7 +457,7 @@ func addSingleSummaryDataPoint(pt pmetric.SummaryDataPoint, resource pcommon.Res
if pt.Flags().NoRecordedValue() {
count.Value = math.Float64frombits(value.StaleNaN)
}
countlabels := createLabels(baseName + countStr)
countlabels := createLabels(baseName+countStr, baseLabels)
addSample(tsMap, count, countlabels, metric.Type().String())

// process each percentile/quantile
Expand All @@ -500,18 +471,36 @@ func addSingleSummaryDataPoint(pt pmetric.SummaryDataPoint, resource pcommon.Res
quantile.Value = math.Float64frombits(value.StaleNaN)
}
percentileStr := strconv.FormatFloat(qt.Quantile(), 'f', -1, 64)
qtlabels := createLabels(baseName, quantileStr, percentileStr)
qtlabels := createLabels(baseName, baseLabels, quantileStr, percentileStr)
addSample(tsMap, quantile, qtlabels, metric.Type().String())
}

// add _created time series if needed
startTimestamp := pt.StartTimestamp()
if settings.ExportCreatedMetric && startTimestamp != 0 {
createdLabels := createLabels(baseName + createdSuffix)
createdLabels := createLabels(baseName+createdSuffix, baseLabels)
addCreatedTimeSeriesIfNeeded(tsMap, createdLabels, startTimestamp, pt.Timestamp(), metric.Type().String())
}
}

// createLabels returns a copy of baseLabels, adding to it the pair model.MetricNameLabel=name.
// If extras are provided, corresponding label pairs are also added to the returned slice.
// If extras is uneven length, the last (unpaired) extra will be ignored.
func createLabels(name string, baseLabels []prompb.Label, extras ...string) []prompb.Label {
extraLabelCount := len(extras) / 2
labels := make([]prompb.Label, len(baseLabels), len(baseLabels)+extraLabelCount+1) // +1 for name
copy(labels, baseLabels)

n := len(extras)
n -= n % 2
for extrasIdx := 0; extrasIdx < n; extrasIdx += 2 {
labels = append(labels, prompb.Label{Name: extras[extrasIdx], Value: extras[extrasIdx+1]})
}

labels = append(labels, prompb.Label{Name: model.MetricNameLabel, Value: name})
return labels
}

// addCreatedTimeSeriesIfNeeded adds {name}_created time series with a single
// sample. If the series exists, then new samples won't be added.
func addCreatedTimeSeriesIfNeeded(
Expand Down
94 changes: 94 additions & 0 deletions pkg/translator/prometheusremotewrite/helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -880,3 +880,97 @@ func TestAddSingleHistogramDataPoint(t *testing.T) {
})
}
}

func TestCreateLabels(t *testing.T) {
testCases := []struct {
name string
metricName string
baseLabels []prompb.Label
extras []string
expected []prompb.Label
}{
{
name: "no base labels, no extras",
metricName: "test",
baseLabels: nil,
expected: []prompb.Label{
{Name: model.MetricNameLabel, Value: "test"},
},
},
{
name: "base labels, no extras",
metricName: "test",
baseLabels: []prompb.Label{
{Name: "base1", Value: "value1"},
{Name: "base2", Value: "value2"},
},
expected: []prompb.Label{
{Name: "base1", Value: "value1"},
{Name: "base2", Value: "value2"},
{Name: model.MetricNameLabel, Value: "test"},
},
},
{
name: "base labels, 1 extra",
metricName: "test",
baseLabels: []prompb.Label{
{Name: "base1", Value: "value1"},
{Name: "base2", Value: "value2"},
},
extras: []string{"extra1", "extraValue1"},
expected: []prompb.Label{
{Name: "base1", Value: "value1"},
{Name: "base2", Value: "value2"},
{Name: "extra1", Value: "extraValue1"},
{Name: model.MetricNameLabel, Value: "test"},
},
},
{
name: "base labels, 2 extras",
metricName: "test",
baseLabels: []prompb.Label{
{Name: "base1", Value: "value1"},
{Name: "base2", Value: "value2"},
},
extras: []string{"extra1", "extraValue1", "extra2", "extraValue2"},
expected: []prompb.Label{
{Name: "base1", Value: "value1"},
{Name: "base2", Value: "value2"},
{Name: "extra1", Value: "extraValue1"},
{Name: "extra2", Value: "extraValue2"},
{Name: model.MetricNameLabel, Value: "test"},
},
},
{
name: "base labels, unpaired extra",
metricName: "test",
baseLabels: []prompb.Label{
{Name: "base1", Value: "value1"},
{Name: "base2", Value: "value2"},
},
extras: []string{"extra1", "extraValue1", "extra2"},
expected: []prompb.Label{
{Name: "base1", Value: "value1"},
{Name: "base2", Value: "value2"},
{Name: "extra1", Value: "extraValue1"},
{Name: model.MetricNameLabel, Value: "test"},
},
},
{
name: "no base labels, 1 extra",
metricName: "test",
baseLabels: nil,
extras: []string{"extra1", "extraValue1"},
expected: []prompb.Label{
{Name: "extra1", Value: "extraValue1"},
{Name: model.MetricNameLabel, Value: "test"},
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
lbls := createLabels(tc.metricName, tc.baseLabels, tc.extras...)
assert.Equal(t, lbls, tc.expected)
})
}
}

0 comments on commit b3ffb62

Please sign in to comment.