move validate and sanitize external labels to exporter (#2071)

* export validate and sanitize config function * added comment for why validateandsanitizeexternallabels is exported * added more detailed reason for why function is exported * moved validateAndSanitizeLabels to within exporter * increasing test cov * fix test case
open-telemetry · Nov 6, 2020 · 6c8a896 · 6c8a896
1 parent c6b8f28
commit 6c8a896
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 79 deletions.
diff --git a/exporter/prometheusremotewriteexporter/exporter.go b/exporter/prometheusremotewriteexporter/exporter.go
@@ -55,14 +55,19 @@ func NewPrwExporter(namespace string, endpoint string, client *http.Client, exte
 		return nil, errors.New("http client cannot be nil")
 	}
 
+	sanitizedLabels, err := validateAndSanitizeExternalLabels(externalLabels)
+	if err != nil {
+		return nil, err
+	}
+
 	endpointURL, err := url.ParseRequestURI(endpoint)
 	if err != nil {
 		return nil, errors.New("invalid endpoint")
 	}
 
 	return &PrwExporter{
 		namespace:      namespace,
-		externalLabels: externalLabels,
+		externalLabels: sanitizedLabels,
 		endpointURL:    endpointURL,
 		client:         client,
 		wg:             new(sync.WaitGroup),
@@ -146,6 +151,25 @@ func (prwe *PrwExporter) PushMetrics(ctx context.Context, md pdata.Metrics) (int
 	}
 }
 
+func validateAndSanitizeExternalLabels(externalLabels map[string]string) (map[string]string, error) {
+	sanitizedLabels := make(map[string]string)
+	for key, value := range 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 len(key) > 2 && key[:2] == "__" {
+			key = "__" + sanitize(key[2:])
+		} else {
+			key = sanitize(key)
+		}
+		sanitizedLabels[key] = value
+	}
+
+	return sanitizedLabels, nil
+}
+
 // handleScalarMetric processes data points in a single OTLP scalar metric by adding the each point as a Sample into
 // its corresponding TimeSeries in tsMap.
 // tsMap and metric cannot be nil, and metric must have a non-nil descriptor

diff --git a/exporter/prometheusremotewriteexporter/exporter_test.go b/exporter/prometheusremotewriteexporter/exporter_test.go
@@ -76,6 +76,15 @@ func Test_NewPrwExporter(t *testing.T) {
 			nil,
 			true,
 		},
+		{
+			"invalid_labels_case",
+			config,
+			"test",
+			"http://some.url:9411/api/prom/push",
+			map[string]string{"Key1": ""},
+			http.DefaultClient,
+			true,
+		},
 		{
 			"success_case",
 			config,
@@ -85,6 +94,15 @@ func Test_NewPrwExporter(t *testing.T) {
 			http.DefaultClient,
 			false,
 		},
+		{
+			"success_case_no_labels",
+			config,
+			"test",
+			"http://some.url:9411/api/prom/push",
+			map[string]string{},
+			http.DefaultClient,
+			false,
+		},
 	}
 
 	for _, tt := range tests {
@@ -679,3 +697,50 @@ func Test_PushMetrics(t *testing.T) {
 		})
 	}
 }
+
+func Test_validateAndSanitizeExternalLabels(t *testing.T) {
+	tests := []struct {
+		name           string
+		inputLabels    map[string]string
+		expectedLabels map[string]string
+		returnError    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,
+		},
+		{"fail_case_empty_label",
+			map[string]string{"": "val1"},
+			map[string]string{},
+			true,
+		},
+	}
+	// run tests
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			newLabels, err := validateAndSanitizeExternalLabels(tt.inputLabels)
+			if tt.returnError {
+				assert.Error(t, err)
+				return
+			}
+			assert.EqualValues(t, tt.expectedLabels, newLabels)
+			assert.NoError(t, err)
+		})
+	}
+}
diff --git a/exporter/prometheusremotewriteexporter/factory.go b/exporter/prometheusremotewriteexporter/factory.go
@@ -17,7 +17,6 @@ package prometheusremotewriteexporter
 import (
 	"context"
 	"errors"
-	"fmt"
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/config/confighttp"
@@ -44,10 +43,6 @@ func createMetricsExporter(_ context.Context, params component.ExporterCreatePar
 	if !ok {
 		return nil, errors.New("invalid configuration")
 	}
-	err := validateAndSanitizeExternalLabels(prwCfg)
-	if err != nil {
-		return nil, err
-	}
 
 	client, err := prwCfg.HTTPClientSettings.ToClient()
 
@@ -95,23 +90,3 @@ func createDefaultConfig() configmodels.Exporter {
 		},
 	}
 }
-
-func validateAndSanitizeExternalLabels(cfg *Config) error {
-	sanitizedLabels := make(map[string]string)
-	for key, value := range cfg.ExternalLabels {
-		if key == "" || value == "" {
-			return fmt.Errorf("prometheus remote write: external labels configuration contains an empty key or value")
-		}
-
-		// Sanitize label keys to meet Prometheus Requirements
-		if len(key) > 2 && key[:2] == "__" {
-			key = "__" + sanitize(key[2:])
-		} else {
-			key = sanitize(key)
-		}
-		sanitizedLabels[key] = value
-	}
-
-	cfg.ExternalLabels = sanitizedLabels
-	return nil
-}
diff --git a/exporter/prometheusremotewriteexporter/factory_test.go b/exporter/prometheusremotewriteexporter/factory_test.go
@@ -89,56 +89,3 @@ func Test_createMetricsExporter(t *testing.T) {
 		})
 	}
 }
-
-func Test_validateAndSanitizeExternalLabels(t *testing.T) {
-	validConfig := createDefaultConfig().(*Config)
-
-	validConfigWithLabels := createDefaultConfig().(*Config)
-	validConfigWithLabels.ExternalLabels["key1"] = "val1"
-
-	validConfigWithLabelsToSanitize := createDefaultConfig().(*Config)
-	validConfigWithLabelsToSanitize.ExternalLabels["__key1.key__"] = "val1"
-
-	invalidConfigWithLabels := createDefaultConfig().(*Config)
-	invalidConfigWithLabels.ExternalLabels[""] = "val1"
-
-	tests := []struct {
-		name           string
-		cfg            *Config
-		expectedLabels map[string]string
-		returnError    bool
-	}{
-		{"success_case_no_labels",
-			validConfig,
-			map[string]string{},
-			false,
-		},
-		{"success_case_with_labels",
-			validConfigWithLabels,
-			map[string]string{"key1": "val1"},
-			false,
-		},
-		{"success_case_with_sanitized_labels",
-			validConfigWithLabelsToSanitize,
-			map[string]string{"__key1_key__": "val1"},
-			false,
-		},
-		{"fail_case_empty_label",
-			invalidConfigWithLabels,
-			map[string]string{},
-			true,
-		},
-	}
-	// run tests
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			err := validateAndSanitizeExternalLabels(tt.cfg)
-			if tt.returnError {
-				assert.Error(t, err)
-				return
-			}
-			assert.EqualValues(t, tt.expectedLabels, tt.cfg.ExternalLabels)
-			assert.NoError(t, err)
-		})
-	}
-}