Skip to content

Commit

Permalink
[prometheusremotewriteexporter] dropping the condition to replace _ w…
Browse files Browse the repository at this point in the history
…ith key_ as __ label is reserved and _ is not (open-telemetry#7112)

feature gate changes to disable sanitization of labels that start with '_'

adding back the changes for feature gate

* moving feature gate to a different group to fix lint issue

* increasing ExpectedMaxRAM to 110 for Zipkin test TestTrace10kSPS

* added testcases for sanitization when sanitizeLabel is turned off

Co-authored-by: Alex Boten <[email protected]>
  • Loading branch information
saikrishna397 and Alex Boten committed Feb 15, 2022
1 parent 2a79d60 commit 293217b
Show file tree
Hide file tree
Showing 7 changed files with 118 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
- `tracegen`: Add option to pass in custom headers to export calls via command line (#7308)
- `tracegen`: Provide official container images (#7179)
- `scrapertest`: Add comparison function for pdata.Metrics (#7400)
- `prometheusremotewriteexporter` : Dropping the condition to replace _ with key_ as __ label is reserved and _ is not (#7112)

### 🛑 Breaking changes 🛑

Expand Down
12 changes: 12 additions & 0 deletions exporter/prometheusremotewriteexporter/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/confighttp"
"go.opentelemetry.io/collector/exporter/exporterhelper"
"go.opentelemetry.io/collector/service/featuregate"

"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/resourcetotelemetry"
)
Expand All @@ -29,6 +30,7 @@ type Config struct {
config.ExporterSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct
exporterhelper.TimeoutSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct.
exporterhelper.RetrySettings `mapstructure:"retry_on_failure"`
sanitizeLabel bool

// prefix attached to each exported metric name
// See: https://prometheus.io/docs/practices/naming/#metric-names
Expand Down Expand Up @@ -64,6 +66,16 @@ type RemoteWriteQueue struct {
NumConsumers int `mapstructure:"num_consumers"`
}

var dropSanitizationGate = featuregate.Gate{
ID: "exporter.prometheusremotewrite.PermissiveLabelSanitization",
Enabled: false,
Description: "Controls whether to change labels starting with '_' to 'key_'",
}

func init() {
featuregate.Register(dropSanitizationGate)
}

// TODO(jbd): Add capacity, max_samples_per_send to QueueConfig.

var _ config.Exporter = (*Config)(nil)
Expand Down
1 change: 1 addition & 0 deletions exporter/prometheusremotewriteexporter/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ func Test_loadConfig(t *testing.T) {
NumConsumers: 10,
},
Namespace: "test-space",
sanitizeLabel: false,
ExternalLabels: map[string]string{"key1": "value1", "key2": "value2"},
HTTPClientSettings: confighttp.HTTPClientSettings{
Endpoint: "localhost:8888",
Expand Down
19 changes: 14 additions & 5 deletions exporter/prometheusremotewriteexporter/exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ type prwExporter struct {

// newPRWExporter initializes a new prwExporter instance and sets fields accordingly.
func newPRWExporter(cfg *Config, set component.ExporterCreateSettings) (*prwExporter, error) {
sanitizedLabels, err := validateAndSanitizeExternalLabels(cfg.ExternalLabels)
sanitizedLabels, err := validateAndSanitizeExternalLabels(cfg)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -116,18 +116,27 @@ func (prwe *prwExporter) PushMetrics(ctx context.Context, md pdata.Metrics) erro
}
}

func validateAndSanitizeExternalLabels(externalLabels map[string]string) (map[string]string, error) {
func validateAndSanitizeExternalLabels(cfg *Config) (map[string]string, error) {
sanitizedLabels := make(map[string]string)
for key, value := range externalLabels {
for key, value := range cfg.ExternalLabels {
if key == "" || value == "" {
return nil, fmt.Errorf("prometheus remote write: external labels configuration contains an empty key or value")
}

// Sanitize label keys to meet Prometheus Requirements
// if sanitizeLabel is enabled, invoke sanitizeLabels else sanitize
if len(key) > 2 && key[:2] == "__" {
key = "__" + sanitize(key[2:])
if cfg.sanitizeLabel {
key = "__" + sanitizeLabels(key[2:])
} else {
key = "__" + sanitize(key[2:])
}
} else {
key = sanitize(key)
if cfg.sanitizeLabel {
key = sanitizeLabels(key)
} else {
key = sanitize(key)
}
}
sanitizedLabels[key] = value
}
Expand Down
74 changes: 73 additions & 1 deletion exporter/prometheusremotewriteexporter/exporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ func Test_NewPRWExporter(t *testing.T) {
Namespace: "",
ExternalLabels: map[string]string{},
HTTPClientSettings: confighttp.HTTPClientSettings{Endpoint: ""},
sanitizeLabel: true,
}
buildInfo := component.BuildInfo{
Description: "OpenTelemetry Collector",
Expand Down Expand Up @@ -680,6 +681,58 @@ func Test_validateAndSanitizeExternalLabels(t *testing.T) {
map[string]string{"__key1_key__": "val1"},
false,
},
{"labels_that_start_with_underscore",
map[string]string{"_key_": "val1"},
map[string]string{"_key_": "val1"},
false,
},
{"labels_that_start_with_digit",
map[string]string{"6key_": "val1"},
map[string]string{"key_6key_": "val1"},
false,
},
{"fail_case_empty_label",
map[string]string{"": "val1"},
map[string]string{},
true,
},
}
testsWithoutSanitizelabel := []struct {
name string
inputLabels map[string]string
expectedLabels map[string]string
returnErrorOnCreate bool
}{
{"success_case_no_labels",
map[string]string{},
map[string]string{},
false,
},
{"success_case_with_labels",
map[string]string{"key1": "val1"},
map[string]string{"key1": "val1"},
false,
},
{"success_case_2_with_labels",
map[string]string{"__key1__": "val1"},
map[string]string{"__key1__": "val1"},
false,
},
{"success_case_with_sanitized_labels",
map[string]string{"__key1.key__": "val1"},
map[string]string{"__key1_key__": "val1"},
false,
},
{"labels_that_start_with_underscore",
map[string]string{"_key_": "val1"},
map[string]string{"key_key_": "val1"},
false,
},
{"labels_that_start_with_digit",
map[string]string{"6key_": "val1"},
map[string]string{"key_6key_": "val1"},
false,
},
{"fail_case_empty_label",
map[string]string{"": "val1"},
map[string]string{},
Expand All @@ -688,8 +741,27 @@ func Test_validateAndSanitizeExternalLabels(t *testing.T) {
}
// run tests
for _, tt := range tests {
cfg := createDefaultConfig().(*Config)
cfg.sanitizeLabel = true
cfg.ExternalLabels = tt.inputLabels
t.Run(tt.name, func(t *testing.T) {
newLabels, err := validateAndSanitizeExternalLabels(cfg)
if tt.returnErrorOnCreate {
assert.Error(t, err)
return
}
assert.EqualValues(t, tt.expectedLabels, newLabels)
assert.NoError(t, err)
})
}

for _, tt := range testsWithoutSanitizelabel {
cfg := createDefaultConfig().(*Config)
//disable sanitizeLabel flag
cfg.sanitizeLabel = false
cfg.ExternalLabels = tt.inputLabels
t.Run(tt.name, func(t *testing.T) {
newLabels, err := validateAndSanitizeExternalLabels(tt.inputLabels)
newLabels, err := validateAndSanitizeExternalLabels(cfg)
if tt.returnErrorOnCreate {
assert.Error(t, err)
return
Expand Down
2 changes: 2 additions & 0 deletions exporter/prometheusremotewriteexporter/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/confighttp"
"go.opentelemetry.io/collector/exporter/exporterhelper"
"go.opentelemetry.io/collector/service/featuregate"

"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/resourcetotelemetry"
)
Expand Down Expand Up @@ -85,6 +86,7 @@ func createDefaultConfig() config.Exporter {
Namespace: "",
ExternalLabels: map[string]string{},
TimeoutSettings: exporterhelper.DefaultTimeoutSettings(),
sanitizeLabel: featuregate.IsEnabled(dropSanitizationGate.ID),
RetrySettings: exporterhelper.RetrySettings{
Enabled: true,
InitialInterval: 50 * time.Millisecond,
Expand Down
15 changes: 15 additions & 0 deletions exporter/prometheusremotewriteexporter/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,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

0 comments on commit 293217b

Please sign in to comment.