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.

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

[prometheusremotewriteexporter] Allow to disable sanitize metric labels #8270

Merged
merged 20 commits into from
Apr 18, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- `googlecloudexporter`: [Alpha] Translate metrics directly from OTLP to gcm using the `exporter.googlecloud.OTLPDirect` feature-gate (#7177)
- `simpleprometheusreceiver`: Add support for static labels (#7908)
- `prometheusremotewriteexporter`: Allow to disable sanitize metric labels (#8270)

### 🛑 Breaking changes 🛑

Expand Down
4 changes: 3 additions & 1 deletion exporter/prometheusremotewriteexporter/exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const maxBatchByteSize = 3000000
// prwExporter converts OTLP metrics to Prometheus remote write TimeSeries and sends them to a remote endpoint.
type prwExporter struct {
namespace string
sanitizeLabel bool
externalLabels map[string]string
endpointURL *url.URL
client *http.Client
Expand Down Expand Up @@ -71,6 +72,7 @@ func newPRWExporter(cfg *Config, set component.ExporterCreateSettings) (*prwExpo

return &prwExporter{
namespace: cfg.Namespace,
sanitizeLabel: cfg.sanitizeLabel,
externalLabels: sanitizedLabels,
endpointURL: endpointURL,
wg: new(sync.WaitGroup),
Expand Down Expand Up @@ -107,7 +109,7 @@ func (prwe *prwExporter) PushMetrics(ctx context.Context, md pdata.Metrics) erro
case <-prwe.closeChan:
return errors.New("shutdown has been called")
default:
tsMap, err := prometheusremotewrite.FromMetrics(md, prometheusremotewrite.Settings{Namespace: prwe.namespace, ExternalLabels: prwe.externalLabels})
tsMap, err := prometheusremotewrite.FromMetrics(md, prometheusremotewrite.Settings{Namespace: prwe.namespace, ExternalLabels: prwe.externalLabels, SanitizeLabel: prwe.sanitizeLabel})
Copy link
Member

Choose a reason for hiding this comment

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

Can this be done without changing the exported interface of the PRW translator package?

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 don't see much options, one is to introduce a feature flag in the translator package, but I was thinking to reuse the same feature flag as basically it's about the same thing as #7112 , thus an additional fields in prometheusremotewrite.Settings will be introduced to follow previous behavior by default.

Copy link
Member

Choose a reason for hiding this comment

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

The thing is that feature gates are intended to be temporary and removed eventually. Changes to the public API of a package are not reversible (assuming a world where we're v1.x, which we need to start planning for) and thus should be avoided in conjunction with feature gates. It is hypothesized that the sanitization of labels starting with _ (but not __) is unnecessary, so perhaps we should focus on finding a way to gain confidence in that hypothesis and eliminating the need for this feature gate.

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 understand the concern of changing the public API in translator, but it's still the same behavior by default, with regards to gain confidence in the hypothesis, I am not sure how we can move forward on that. could you please elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @Aneurysm9 any thoughts on how we can move forward on this?

Copy link
Member

Choose a reason for hiding this comment

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

Then we should probably eliminate it altogether rather than further propagating it. Changing the public API of this module does not seem like the correct thing to do.

I'm not sure I'm comfortable making this decision by myself. @dashpole @bogdandrutu @jpkrohling @codeboten @gouthamve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that sounds like this will be a breaking change which is going to remove the key prefix.

If not to change the public API, would it work that introduce a new method similar to FromMetrics which does not sanitize label(no key prefix)?

Copy link
Member

Choose a reason for hiding this comment

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

Changing the public API of this module does not seem like the correct thing to do.

I share the same opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree we should not change the public API. We should either make the change outright, or use a feature-gate to slowly make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a feature gate for this.

if err != nil {
err = consumererror.NewPermanent(err)
}
Expand Down
59 changes: 44 additions & 15 deletions pkg/translator/prometheusremotewrite/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func timeSeriesSignature(metric pdata.Metric, labels *[]prompb.Label) string {
// createAttributes creates a slice of Cortex Label with OTLP attributes and pairs of string values.
// Unpaired string value is ignored. String pairs overwrites OTLP labels if collision happens, and the overwrite is
// logged. Resultant label names are sanitized.
func createAttributes(resource pdata.Resource, attributes pdata.AttributeMap, externalLabels map[string]string, extras ...string) []prompb.Label {
func createAttributes(resource pdata.Resource, attributes pdata.AttributeMap, externalLabels map[string]string, sanitizeLabel bool, extras ...string) []prompb.Label {
// map ensures no duplicate label name
l := map[string]prompb.Label{}

Expand All @@ -159,19 +159,33 @@ func createAttributes(resource pdata.Resource, attributes pdata.AttributeMap, ex

resource.Attributes().Range(func(key string, value pdata.AttributeValue) bool {
if isUsefulResourceAttribute(key) {
l[key] = prompb.Label{
Name: sanitize(key),
Value: value.AsString(),
if sanitizeLabel {
l[key] = prompb.Label{
Name: sanitize(key),
Value: value.AsString(),
}
} else {
l[key] = prompb.Label{
Name: sanitizeLabels(key),
Value: value.AsString(),
}
}
}

return true
})

attributes.Range(func(key string, value pdata.AttributeValue) bool {
l[key] = prompb.Label{
Name: sanitize(key),
Value: value.AsString(),
if sanitizeLabel {
l[key] = prompb.Label{
Name: sanitize(key),
Value: value.AsString(),
}
} else {
l[key] = prompb.Label{
Name: sanitizeLabels(key),
Value: value.AsString(),
}
}

return true
Expand Down Expand Up @@ -249,7 +263,7 @@ func validateMetrics(metric pdata.Metric) bool {
func addSingleNumberDataPoint(pt pdata.NumberDataPoint, resource pdata.Resource, metric pdata.Metric, settings Settings, tsMap map[string]*prompb.TimeSeries) {
// create parameters for addSample
name := getPromMetricName(metric, settings.Namespace)
labels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, nameStr, name)
labels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, settings.SanitizeLabel, nameStr, name)
sample := &prompb.Sample{
// convert ns to ms
Timestamp: convertTimeStamp(pt.Timestamp()),
Expand Down Expand Up @@ -281,7 +295,7 @@ func addSingleHistogramDataPoint(pt pdata.HistogramDataPoint, resource pdata.Res
sum.Value = math.Float64frombits(value.StaleNaN)
}

sumlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, nameStr, baseName+sumStr)
sumlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, settings.SanitizeLabel, nameStr, baseName+sumStr)
addSample(tsMap, sum, sumlabels, metric)

// treat count as a sample in an individual TimeSeries
Expand All @@ -293,7 +307,7 @@ func addSingleHistogramDataPoint(pt pdata.HistogramDataPoint, resource pdata.Res
count.Value = math.Float64frombits(value.StaleNaN)
}

countlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, nameStr, baseName+countStr)
countlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, settings.SanitizeLabel, nameStr, baseName+countStr)
addSample(tsMap, count, countlabels, metric)

// cumulative count for conversion to cumulative histogram
Expand All @@ -317,7 +331,7 @@ func addSingleHistogramDataPoint(pt pdata.HistogramDataPoint, resource pdata.Res
bucket.Value = math.Float64frombits(value.StaleNaN)
}
boundStr := strconv.FormatFloat(bound, 'f', -1, 64)
labels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, nameStr, baseName+bucketStr, leStr, boundStr)
labels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, settings.SanitizeLabel, nameStr, baseName+bucketStr, leStr, boundStr)
sig := addSample(tsMap, bucket, labels, metric)

bucketBounds = append(bucketBounds, bucketBoundsData{sig: sig, bound: bound})
Expand All @@ -332,7 +346,7 @@ func addSingleHistogramDataPoint(pt pdata.HistogramDataPoint, resource pdata.Res
cumulativeCount += pt.BucketCounts()[len(pt.BucketCounts())-1]
infBucket.Value = float64(cumulativeCount)
}
infLabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, nameStr, baseName+bucketStr, leStr, pInfStr)
infLabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, settings.SanitizeLabel, nameStr, baseName+bucketStr, leStr, pInfStr)
sig := addSample(tsMap, infBucket, infLabels, metric)

bucketBounds = append(bucketBounds, bucketBoundsData{sig: sig, bound: math.Inf(1)})
Expand Down Expand Up @@ -381,7 +395,7 @@ func addSingleSummaryDataPoint(pt pdata.SummaryDataPoint, resource pdata.Resourc
if pt.Flags().HasFlag(pdata.MetricDataPointFlagNoRecordedValue) {
sum.Value = math.Float64frombits(value.StaleNaN)
}
sumlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, nameStr, baseName+sumStr)
sumlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, settings.SanitizeLabel, nameStr, baseName+sumStr)
addSample(tsMap, sum, sumlabels, metric)

// treat count as a sample in an individual TimeSeries
Expand All @@ -392,7 +406,7 @@ func addSingleSummaryDataPoint(pt pdata.SummaryDataPoint, resource pdata.Resourc
if pt.Flags().HasFlag(pdata.MetricDataPointFlagNoRecordedValue) {
count.Value = math.Float64frombits(value.StaleNaN)
}
countlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, nameStr, baseName+countStr)
countlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, settings.SanitizeLabel, nameStr, baseName+countStr)
addSample(tsMap, count, countlabels, metric)

// process each percentile/quantile
Expand All @@ -406,7 +420,7 @@ func addSingleSummaryDataPoint(pt pdata.SummaryDataPoint, resource pdata.Resourc
quantile.Value = math.Float64frombits(value.StaleNaN)
}
percentileStr := strconv.FormatFloat(qt.Quantile(), 'f', -1, 64)
qtlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, nameStr, baseName, quantileStr, percentileStr)
qtlabels := createAttributes(resource, pt.Attributes(), settings.ExternalLabels, settings.SanitizeLabel, nameStr, baseName, quantileStr, percentileStr)
addSample(tsMap, quantile, qtlabels, metric)
}
}
Expand All @@ -431,6 +445,21 @@ func sanitize(s string) string {
return s
}

func sanitizeLabels(s string) string {
if len(s) == 0 {
return s
}

// Note: No length limit for label keys because Prometheus doesn't
// define a length limit, thus we should NOT be truncating label keys.
// labels that start with _ are not sanitized
s = strings.Map(sanitizeRune, s)
if unicode.IsDigit(rune(s[0])) {
s = keyStr + "_" + s
}
return s
}

// copied from prometheus-go-metric-exporter
// sanitizeRune converts anything that is not a letter or digit to an underscore
func sanitizeRune(r rune) rune {
Expand Down
31 changes: 30 additions & 1 deletion pkg/translator/prometheusremotewrite/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ func Test_createLabelSet(t *testing.T) {
resource pdata.Resource
orig pdata.AttributeMap
externalLabels map[string]string
sanitizeLabel bool
extras []string
want []prompb.Label
}{
Expand All @@ -192,6 +193,7 @@ func Test_createLabelSet(t *testing.T) {
getResource(map[string]pdata.AttributeValue{}),
lbs1,
map[string]string{},
true,
[]string{label31, value31, label32, value32},
getPromLabels(label11, value11, label12, value12, label31, value31, label32, value32),
},
Expand All @@ -203,6 +205,7 @@ func Test_createLabelSet(t *testing.T) {
}),
lbs1,
map[string]string{},
true,
[]string{label31, value31, label32, value32},
getPromLabels(label11, value11, label12, value12, label31, value31, label32, value32, "job", "prometheus", "instance", "127.0.0.1:8080"),
},
Expand All @@ -214,6 +217,7 @@ func Test_createLabelSet(t *testing.T) {
}),
lbs1,
map[string]string{},
true,
[]string{label31, value31, label32, value32},
getPromLabels(label11, value11, label12, value12, label31, value31, label32, value32, "job", "12345", "instance", "true"),
},
Expand All @@ -222,6 +226,7 @@ func Test_createLabelSet(t *testing.T) {
getResource(map[string]pdata.AttributeValue{}),
lbs1,
map[string]string{},
true,
[]string{label11, value31},
getPromLabels(label11, value31, label12, value12),
},
Expand All @@ -230,6 +235,7 @@ func Test_createLabelSet(t *testing.T) {
getResource(map[string]pdata.AttributeValue{}),
lbs1Dirty,
map[string]string{},
true,
[]string{label31 + dirty1, value31, label32, value32},
getPromLabels(label11+"_", value11, "key_"+label12, value12, label31+"_", value31, label32, value32),
},
Expand All @@ -238,6 +244,7 @@ func Test_createLabelSet(t *testing.T) {
getResource(map[string]pdata.AttributeValue{}),
pdata.NewAttributeMap(),
nil,
true,
[]string{label31, value31, label32, value32},
getPromLabels(label31, value31, label32, value32),
},
Expand All @@ -246,6 +253,7 @@ func Test_createLabelSet(t *testing.T) {
getResource(map[string]pdata.AttributeValue{}),
lbs1,
map[string]string{},
true,
[]string{"", ""},
getPromLabels(label11, value11, label12, value12, "", ""),
},
Expand All @@ -254,6 +262,7 @@ func Test_createLabelSet(t *testing.T) {
getResource(map[string]pdata.AttributeValue{}),
lbs1,
map[string]string{},
true,
[]string{label31, value31, label32},
getPromLabels(label11, value11, label12, value12, label31, value31),
},
Expand All @@ -262,6 +271,7 @@ func Test_createLabelSet(t *testing.T) {
getResource(map[string]pdata.AttributeValue{}),
lbs1,
exlbs1,
true,
[]string{label31, value31, label32, value32},
getPromLabels(label11, value11, label12, value12, label41, value41, label31, value31, label32, value32),
},
Expand All @@ -270,14 +280,33 @@ func Test_createLabelSet(t *testing.T) {
getResource(map[string]pdata.AttributeValue{}),
lbs1,
exlbs2,
true,
[]string{label31, value31, label32, value32},
getPromLabels(label11, value11, label12, value12, label31, value31, label32, value32),
},
{
"sanitize_labels_starts_with_underscore",
getResource(map[string]pdata.AttributeValue{}),
lbs3,
exlbs1,
true,
[]string{label31, value31, label32, value32},
getPromLabels(label11, value11, label12, value12, keyStr+label51, value51, label41, value41, label31, value31, label32, value32),
},
{
"donot_sanitize_labels_starts_with_underscore",
getResource(map[string]pdata.AttributeValue{}),
lbs3,
exlbs1,
false,
[]string{label31, value31, label32, value32},
getPromLabels(label11, value11, label12, value12, label51, value51, label41, value41, label31, value31, label32, value32),
},
}
// run tests
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.ElementsMatch(t, tt.want, createAttributes(tt.resource, tt.orig, tt.externalLabels, tt.extras...))
assert.ElementsMatch(t, tt.want, createAttributes(tt.resource, tt.orig, tt.externalLabels, tt.sanitizeLabel, tt.extras...))
})
}
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/translator/prometheusremotewrite/metrics_to_prw.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

// Deprecated: [0.45.0] use `prometheusremotewrite.FromMetrics`. It does not wrap the error as `NewPermanent`.
func MetricsToPRW(namespace string, externalLabels map[string]string, md pdata.Metrics) (map[string]*prompb.TimeSeries, int, error) {
tsMap, err := FromMetrics(md, Settings{Namespace: namespace, ExternalLabels: externalLabels})
tsMap, err := FromMetrics(md, Settings{Namespace: namespace, ExternalLabels: externalLabels, SanitizeLabel: true})
if err != nil {
err = consumererror.NewPermanent(err)
}
Expand All @@ -36,6 +36,7 @@ func MetricsToPRW(namespace string, externalLabels map[string]string, md pdata.M
type Settings struct {
Namespace string
ExternalLabels map[string]string
SanitizeLabel bool
}

// FromMetrics converts pdata.Metrics to prometheus remote write format.
Expand Down
3 changes: 3 additions & 0 deletions pkg/translator/prometheusremotewrite/testutils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ var (
value32 = "test_value32"
label41 = "__test_label41__"
value41 = "test_value41"
label51 = "_test_label51"
value51 = "test_value51"
dirty1 = "%"
dirty2 = "?"
traceIDValue1 = "traceID-value1"
Expand All @@ -55,6 +57,7 @@ var (

lbs1 = getAttributes(label11, value11, label12, value12)
lbs2 = getAttributes(label21, value21, label22, value22)
lbs3 = getAttributes(label11, value11, label12, value12, label51, value51)
lbs1Dirty = getAttributes(label11+dirty1, value11, dirty2+label12, value12)

exlbs1 = map[string]string{label41: value41}
Expand Down