From 12e42379973a4f388b18b7b824ffb82a3b627630 Mon Sep 17 00:00:00 2001 From: Chris Clayton <43845484+ccgovuk@users.noreply.github.com> Date: Mon, 17 Jun 2024 10:23:41 +0100 Subject: [PATCH] divide long functions, replace strings.builder with bytes.buffer. (#13) Co-authored-by: Chris Clayton --- monitoring/helpers.go | 208 ++++++++++++++++++++++++------------- monitoring/helpers_test.go | 97 +++++++++++++++-- 2 files changed, 227 insertions(+), 78 deletions(-) diff --git a/monitoring/helpers.go b/monitoring/helpers.go index 136769a..cc9aa10 100644 --- a/monitoring/helpers.go +++ b/monitoring/helpers.go @@ -1,129 +1,193 @@ package libpack_monitoring import ( + "bytes" "fmt" "os" "sort" "strings" + "sync" "unicode" libpack_config "github.com/lukaszraczylo/graphql-monitoring-proxy/config" ) -func (ms *MetricsSetup) get_metrics_name(name string, labels map[string]string) (complete_name string) { +// Cache for sorted label keys to avoid repeated sorting +var sortedLabelKeysCache = struct { + sync.RWMutex + m map[string][]string +}{m: make(map[string][]string)} + +func (ms *MetricsSetup) get_metrics_name(name string, labels map[string]string) string { const unknownPodName = "unknown" - var sb strings.Builder + var buf bytes.Buffer // Prepare default labels without initializing a new map - podName := unknownPodName - if hn, err := os.Hostname(); err == nil { - podName = hn - } + podName := getPodName() if labels == nil { - labels = map[string]string{ - "microservice": libpack_config.PKG_NAME, - "pod": podName, - } + labels = defaultLabels(podName) } else { - if _, exists := labels["microservice"]; !exists { - labels["microservice"] = libpack_config.PKG_NAME - } - if _, exists := labels["pod"]; !exists { - labels["pod"] = podName - } + ensureDefaultLabels(&labels, podName) } // Prefix handling if ms.metrics_prefix != "" { - sb.WriteString(ms.metrics_prefix) - sb.WriteString("_") + buf.WriteString(ms.metrics_prefix) + buf.WriteString("_") } - sb.WriteString(name) + buf.WriteString(name) // Append labels if any if len(labels) > 0 { - sb.WriteString("{") + buf.WriteString("{") + appendSortedLabels(&buf, labels) + buf.WriteString("}") + } - keys := make([]string, 0, len(labels)) + return buf.String() +} + +func getPodName() string { + const unknownPodName = "unknown" + if hn, err := os.Hostname(); err == nil { + return hn + } + return unknownPodName +} + +func defaultLabels(podName string) map[string]string { + return map[string]string{ + "microservice": libpack_config.PKG_NAME, + "pod": podName, + } +} + +func ensureDefaultLabels(labels *map[string]string, podName string) { + if *labels == nil { + *labels = make(map[string]string) + } + if _, exists := (*labels)["microservice"]; !exists { + (*labels)["microservice"] = libpack_config.PKG_NAME + } + if _, exists := (*labels)["pod"]; !exists { + (*labels)["pod"] = podName + } +} + +func appendSortedLabels(buf *bytes.Buffer, labels map[string]string) { + keys := getSortedKeys(labels) + for i, k := range keys { + if i > 0 { + buf.WriteString(",") + } + buf.WriteString(k) + buf.WriteString("=\"") + buf.WriteString(labels[k]) + buf.WriteString("\"") + } +} + +func getSortedKeys(labels map[string]string) []string { + labelsKey := labelsToString(labels) + + sortedLabelKeysCache.RLock() + keys, exists := sortedLabelKeysCache.m[labelsKey] + sortedLabelKeysCache.RUnlock() + + if !exists { + keys = make([]string, 0, len(labels)) for k := range labels { keys = append(keys, k) } sort.Strings(keys) - for i, k := range keys { - if i > 0 { - sb.WriteString(",") - } - sb.WriteString(k) - sb.WriteString("=\"") - sb.WriteString(labels[k]) - sb.WriteString("\"") - } - sb.WriteString("}") + sortedLabelKeysCache.Lock() + sortedLabelKeysCache.m[labelsKey] = keys + sortedLabelKeysCache.Unlock() } + return keys +} + +func labelsToString(labels map[string]string) string { + var sb strings.Builder + for k, v := range labels { + sb.WriteString(k) + sb.WriteString("=") + sb.WriteString(v) + sb.WriteString(";") + } return sb.String() } // validate_metrics_name validates the name of the metric to adhere to the Prometheus naming conventions // https://prometheus.io/docs/practices/naming/ func validate_metrics_name(name string) error { - var sb strings.Builder // Use strings.Builder for efficient string concatenation + cleanedName := clean_metric_name(name) - // Track if the last character was an underscore to avoid duplicate underscores - lastWasUnderscore := false - - for _, r := range name { - // Convert spaces to underscores and skip non-alphanumeric characters except underscores - if r == ' ' || (unicode.IsLetter(r) || unicode.IsDigit(r) || r == '_') { - if r == ' ' || r == '_' { - if lastWasUnderscore { - continue // Skip if the previous character was also an underscore - } - r = '_' // Convert spaces to underscores - lastWasUnderscore = true - } else { - lastWasUnderscore = false - } - sb.WriteRune(r) // Add valid characters to the builder - } - } // Trim leading and trailing underscores - name_new := strings.Trim(sb.String(), "_") + finalName := strings.Trim(cleanedName, "_") // Check if the processed name matches the original input - if name_new != name { - return fmt.Errorf("Invalid metric name: %s, expected %s", name, name_new) + if finalName != name { + return fmt.Errorf("Invalid metric name: %s, expected %s", name, finalName) } return nil } -func compile_metrics_with_labels(name string, labels map[string]string) string { - var totalLength int - totalLength += len(name) - for k, v := range labels { - totalLength += len(k) + len(v) + 2 +// clean_metric_name processes the metric name according to Prometheus naming conventions +func clean_metric_name(name string) string { + var buf bytes.Buffer + lastWasUnderscore := false + + for _, r := range name { + if is_allowed_rune(r) { + if is_special_rune(r) { + if lastWasUnderscore { + continue // Skip if the previous character was also an underscore + } + r = '_' // Convert spaces and special characters to underscores + lastWasUnderscore = true + } else { + lastWasUnderscore = false + } + buf.WriteRune(r) + } else if !lastWasUnderscore { + buf.WriteRune('_') + lastWasUnderscore = true + } } - var sb strings.Builder - sb.Grow(totalLength + 1) + // Remove trailing underscore + result := buf.String() + return strings.Trim(result, "_") +} - sb.WriteString(name) +// is_allowed_rune checks if the rune is allowed in the metric name +func is_allowed_rune(r rune) bool { + return unicode.IsLetter(r) || unicode.IsDigit(r) || r == ' ' || r == '_' +} + +// is_special_rune checks if the rune is a space or an underscore +func is_special_rune(r rune) bool { + return r == ' ' || r == '_' +} + +func compile_metrics_with_labels(name string, labels map[string]string) string { + var buf bytes.Buffer + + buf.WriteString(name) // Collect keys and sort them - keys := make([]string, 0, len(labels)) - for k := range labels { - keys = append(keys, k) - } - sort.Strings(keys) + keys := getSortedKeys(labels) - // Append sorted key-value pairs to the builder + // Append sorted key-value pairs to the buffer for _, k := range keys { - sb.WriteString("_") - sb.WriteString(k) - sb.WriteString("_") - sb.WriteString(labels[k]) + buf.WriteString("_") + buf.WriteString(k) + buf.WriteString("_") + buf.WriteString(labels[k]) } - return sb.String() + return buf.String() } diff --git a/monitoring/helpers_test.go b/monitoring/helpers_test.go index 7530118..c937362 100644 --- a/monitoring/helpers_test.go +++ b/monitoring/helpers_test.go @@ -1,7 +1,6 @@ package libpack_monitoring import ( - "os" "testing" libpack_config "github.com/lukaszraczylo/graphql-monitoring-proxy/config" @@ -134,10 +133,96 @@ func TestValidateMetricsName(t *testing.T) { } } -func getPodName() string { - podName, err := os.Hostname() - if err != nil { - return "unknown" +func TestCleanMetricName(t *testing.T) { + tests := []struct { + input string + expected string + }{ + {"valid metric name", "valid_metric_name"}, + {"valid@metric#name!", "valid_metric_name"}, + {"__valid__metric__name__", "valid_metric_name"}, + {" valid metric name ", "valid_metric_name"}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + assert.Equal(t, tt.expected, clean_metric_name(tt.input)) + }) + } +} + +func TestDefaultLabels(t *testing.T) { + podName := "test-pod" + libpack_config.PKG_NAME = "example_microservice" + expected := map[string]string{ + "microservice": "example_microservice", + "pod": podName, + } + + assert.Equal(t, expected, defaultLabels(podName)) +} + +func TestEnsureDefaultLabels(t *testing.T) { + podName := "test-pod" + libpack_config.PKG_NAME = "example_microservice" + + tests := []struct { + name string + inputLabels map[string]string + expectedLabels map[string]string + }{ + { + name: "Nil labels", + inputLabels: nil, + expectedLabels: map[string]string{"microservice": "example_microservice", "pod": podName}, + }, + { + name: "Empty labels", + inputLabels: map[string]string{}, + expectedLabels: map[string]string{"microservice": "example_microservice", "pod": podName}, + }, + { + name: "Partial labels", + inputLabels: map[string]string{"microservice": "test_service"}, + expectedLabels: map[string]string{"microservice": "test_service", "pod": podName}, + }, + { + name: "Complete labels", + inputLabels: map[string]string{"microservice": "test_service", "pod": "custom_pod"}, + expectedLabels: map[string]string{"microservice": "test_service", "pod": "custom_pod"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ensureDefaultLabels(&tt.inputLabels, podName) + assert.Equal(t, tt.expectedLabels, tt.inputLabels) + }) + } +} + +func TestLabelsToString(t *testing.T) { + tests := []struct { + labels map[string]string + expected string + }{ + { + labels: map[string]string{"key1": "value1", "key2": "value2"}, + expected: "key1=value1;key2=value2;", + }, + { + labels: map[string]string{"a": "1", "b": "2"}, + expected: "a=1;b=2;", + }, + { + labels: map[string]string{}, + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.expected, func(t *testing.T) { + assert.Equal(t, tt.expected, labelsToString(tt.labels)) + }) } - return podName }