Skip to content

Commit

Permalink
[prometheusremotewrite] fix: counter name check (#2613)
Browse files Browse the repository at this point in the history
* [prometheusremotewrite] fix: counter name check

fixes  #2608

Ensure that the metric name is greater than the length of the
counter suffix ("_total") before checking if it contains the counter
suffix to prevent crashes for short (smaller or eq to the length of the
suffix) metric names.

The edge case where the metric name is "_total" is not handled (considered
not to have the suffix), but should not occur IRL.

Also fix the spelling of "delimiter".

Signed-off-by: naseemkullah <naseem@transit.app>

* use strings.HasSuffix()

Signed-off-by: naseemkullah <naseem@transit.app>

* add already suffixed counter test

Signed-off-by: naseemkullah <naseem@transit.app>
  • Loading branch information
Naseem authored Mar 8, 2021
1 parent 65c4c4a commit 35cf41e
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 19 deletions.
32 changes: 14 additions & 18 deletions exporter/prometheusremotewriteexporter/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,16 @@ import (
)

const (
nameStr = "__name__"
sumStr = "_sum"
countStr = "_count"
bucketStr = "_bucket"
leStr = "le"
quantileStr = "quantile"
pInfStr = "+Inf"
totalStr = "total"
delimeter = "_"
keyStr = "key"
nameStr = "__name__"
sumStr = "_sum"
countStr = "_count"
bucketStr = "_bucket"
leStr = "le"
quantileStr = "quantile"
pInfStr = "+Inf"
counterSuffix = "_total"
delimiter = "_"
keyStr = "key"
)

// ByLabelName enables the usage of sort.Sort() with a slice of labels
Expand Down Expand Up @@ -188,14 +188,11 @@ func getPromMetricName(metric *otlp.Metric, ns string) string {
b.WriteString(ns)

if b.Len() > 0 {
b.WriteString(delimeter)
b.WriteString(delimiter)
}
name := metric.GetName()
b.WriteString(name)

// do not add the total suffix if the metric name already ends in "total"
isCounter = isCounter && name[len(name)-len(totalStr):] != totalStr

// Including units makes two metrics with the same name and label set belong to two different TimeSeries if the
// units are different.
/*
Expand All @@ -205,9 +202,8 @@ func getPromMetricName(metric *otlp.Metric, ns string) string {
}
*/

if b.Len() > 0 && isCounter {
b.WriteString(delimeter)
b.WriteString(totalStr)
if b.Len() > 0 && isCounter && !strings.HasSuffix(name, counterSuffix) {
b.WriteString(counterSuffix)
}
return sanitize(b.String())
}
Expand Down Expand Up @@ -262,7 +258,7 @@ func sanitize(s string) string {
// See https://github.com/orijtech/prometheus-go-metrics-exporter/issues/4.
s = strings.Map(sanitizeRune, s)
if unicode.IsDigit(rune(s[0])) {
s = keyStr + delimeter + s
s = keyStr + delimiter + s
}
if s[0] == '_' {
s = keyStr + s
Expand Down
8 changes: 7 additions & 1 deletion exporter/prometheusremotewriteexporter/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,13 @@ func Test_getPromMetricName(t *testing.T) {
"total_suffix",
validMetrics1[validIntSum],
ns1,
"test_ns_" + validIntSum + delimeter + totalStr,
"test_ns_" + validIntSum + counterSuffix,
},
{
"already_has_total_suffix",
validMetrics1[suffixedCounter],
ns1,
"test_ns_" + suffixedCounter,
},
{
"dirty_string",
Expand Down
13 changes: 13 additions & 0 deletions exporter/prometheusremotewriteexporter/testutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ var (
validIntHistogram = "valid_IntHistogram"
validDoubleHistogram = "valid_DoubleHistogram"
validDoubleSummary = "valid_DoubleSummary"
suffixedCounter = "valid_IntSum_total"

validIntGaugeDirty = "*valid_IntGauge$"

Expand Down Expand Up @@ -136,6 +137,18 @@ var (
},
},
},
suffixedCounter: {
Name: suffixedCounter,
Data: &otlp.Metric_IntSum{
IntSum: &otlp.IntSum{
DataPoints: []*otlp.IntDataPoint{
getIntDataPoint(lbs1, intVal1, time1),
nil,
},
AggregationTemporality: otlp.AggregationTemporality_AGGREGATION_TEMPORALITY_CUMULATIVE,
},
},
},
validDoubleSum: {
Name: validDoubleSum,
Data: &otlp.Metric_DoubleSum{
Expand Down

0 comments on commit 35cf41e

Please sign in to comment.