Merge pull request #927 from rassvetalov/fix/vm-seriesbytag-name-to-graphite

victoriametrics: fix seriesByTag name tag mapping and quote-aware tag splitting

Author: deniszh

zipper/protocols/prometheus/helpers/helpers.go

@@ -160,7 +160,7 @@ func PromethizeTagValue(tagValue string) (string, types.Tag) {
 	case idx+1 == len(tagValue): // != or = with empty value
 		t.OP += "="
 	case tagValue[idx+1] == '~':
-		if len(t.OP) > 0 { // !=~
+		if t.OP != "" { // !=~
 			t.OP += "~"
 		} else { // =~
 			t.OP = "=~"
@@ -177,12 +177,37 @@ func PromethizeTagValue(tagValue string) (string, types.Tag) {
 	return tagName, t
 }
 
-// SplitTagValues - For given tag-value list converts it to more usable map[string]Tag, where string is TagName
+// splitWithQuotes splits s by delimiter, but skips delimiters inside single- or double-quoted regions.
+// This is needed because seriesByTag arguments are quoted ('tag=value') and tag values themselves may
+// contain commas which must not be treated as argument separators.
+func splitWithQuotes(s string, delimiter rune) []string {
+	var result []string
+	start := 0
+	var quoteChar rune // 0 = outside quotes
+	for i, c := range s {
+		switch {
+		case c == quoteChar:
+			quoteChar = 0
+		case quoteChar == 0 && (c == '\'' || c == '"'):
+			quoteChar = c
+		case quoteChar == 0 && c == delimiter:
+			result = append(result, s[start:i])
+			start = i + 1
+		}
+	}
+	return append(result, s[start:])
+}
+
+// SplitTagValues - For given tag-value list converts it to more usable map[string]Tag, where string is TagName.
+// Splits by comma but respects commas inside quoted strings (e.g. seriesByTag('tag=value','other=val,ue')).
 func SplitTagValues(query string) map[string]types.Tag {
-	tags := strings.Split(query, ",")
+	tags := splitWithQuotes(query, ',')
 	result := make(map[string]types.Tag)
 	for _, tvString := range tags {
 		tvString = strings.TrimSpace(tvString)
+		if len(tvString) < 2 {
+			continue
+		}
 		name, tag := PromethizeTagValue(tvString[1 : len(tvString)-1])
 		result[name] = tag
 	}
@@ -210,13 +235,37 @@ func PromMetricToGraphite(metric map[string]string) string {
 	return res.String()
 }
 
-// SeriesByTagToPromQL converts graphite SeriesByTag to PromQL
-// will return step if __step__ is passed
+// SeriesByTagToPromQL converts graphite SeriesByTag to PromQL.
+// Returns step (possibly overridden by __step__ tag) and the PromQL selector string.
 func SeriesByTagToPromQL(step, target string) (string, string) {
+	return convertSeriesByTagToPromQL(step, target, nil)
+}
+
+// SeriesByTagToPromQLWithRenames is like SeriesByTagToPromQL but applies tagRenames
+// to tag names after parsing and before building the PromQL selector. For example,
+// passing map[string]string{"name": "__graphite__"} renames the Graphite "name" tag
+// to the VictoriaMetrics "__graphite__" label.
+//
+// Note: if both the old and new tag names exist in the query, the renamed tag
+// overwrites the existing one (last-write-wins).
+func SeriesByTagToPromQLWithRenames(step, target string, tagRenames map[string]string) (string, string) {
+	return convertSeriesByTagToPromQL(step, target, tagRenames)
+}
+
+func convertSeriesByTagToPromQL(step, target string, tagRenames map[string]string) (string, string) {
 	firstTag := true
 	var queryBuilder strings.Builder
 	tagsString := target[len("seriesByTag(") : len(target)-1]
 	tvs := SplitTagValues(tagsString)
+
+	// Apply tag renames (e.g. "name" -> "__graphite__" for VictoriaMetrics).
+	for oldName, newName := range tagRenames {
+		if v, ok := tvs[oldName]; ok {
+			delete(tvs, oldName)
+			tvs[newName] = v
+		}
+	}
+
 	// It's ok to have empty "__name__"
 	if v, ok := tvs["__name__"]; ok {
 		if v.OP == "=" {
@@ -229,7 +278,15 @@ func SeriesByTagToPromQL(step, target string) (string, string) {
 
 		delete(tvs, "__name__")
 	}
-	for tagName, t := range tvs {
+	// Sort tag names to produce deterministic PromQL output.
+	tagNames := make([]string, 0, len(tvs))
+	for tagName := range tvs {
+		tagNames = append(tagNames, tagName)
+	}
+	sort.Strings(tagNames)
+
+	for _, tagName := range tagNames {
+		t := tvs[tagName]
 		if tagName == "__step__" {
 			step = t.TagValue
 			continue

zipper/protocols/prometheus/helpers/helpers_test.go

@@ -250,3 +250,122 @@ func TestPromethizeTagValue(t *testing.T) {
 	}
 
 }
+
+func TestSplitTagValues(t *testing.T) {
+	tests := []struct {
+		name  string
+		query string
+		want  map[string]types.Tag
+	}{
+		{
+			name:  "simple two tags",
+			query: "'env=prod','host=web1'",
+			want: map[string]types.Tag{
+				"env":  {OP: "=", TagValue: "prod"},
+				"host": {OP: "=", TagValue: "web1"},
+			},
+		},
+		{
+			name:  "comma inside quoted value",
+			query: "'env=prod','desc=hello, world'",
+			want: map[string]types.Tag{
+				"env":  {OP: "=", TagValue: "prod"},
+				"desc": {OP: "=", TagValue: "hello, world"},
+			},
+		},
+		{
+			name:  "empty input",
+			query: "",
+			want:  map[string]types.Tag{},
+		},
+		{
+			name:  "single tag",
+			query: "'name=cpu.load'",
+			want: map[string]types.Tag{
+				"name": {OP: "=", TagValue: "cpu.load"},
+			},
+		},
+		{
+			name:  "trailing comma produces empty segment (skipped)",
+			query: "'env=prod',",
+			want: map[string]types.Tag{
+				"env": {OP: "=", TagValue: "prod"},
+			},
+		},
+		{
+			name:  "whitespace around tags",
+			query: " 'env=prod' , 'host=web1' ",
+			want: map[string]types.Tag{
+				"env":  {OP: "=", TagValue: "prod"},
+				"host": {OP: "=", TagValue: "web1"},
+			},
+		},
+		{
+			name:  "double-quoted values",
+			query: `"env=prod","host=web1"`,
+			want: map[string]types.Tag{
+				"env":  {OP: "=", TagValue: "prod"},
+				"host": {OP: "=", TagValue: "web1"},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := SplitTagValues(tt.query)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestSeriesByTagToPromQLWithRenames(t *testing.T) {
+	tests := []struct {
+		name       string
+		step       string
+		target     string
+		renames    map[string]string
+		wantStep   string
+		wantPromQL string
+	}{
+		{
+			name:       "VM: name renamed to __graphite__",
+			step:       "30",
+			target:     "seriesByTag('name=servers.web.cpu','env=prod')",
+			renames:    map[string]string{"name": "__graphite__"},
+			wantStep:   "30",
+			wantPromQL: `{__graphite__="servers.web.cpu", env="prod"}`,
+		},
+		{
+			name:       "VM: hostname not affected by name rename",
+			step:       "30",
+			target:     "seriesByTag('hostname=web1','env=prod')",
+			renames:    map[string]string{"name": "__graphite__"},
+			wantStep:   "30",
+			wantPromQL: `{env="prod", hostname="web1"}`,
+		},
+		{
+			name:       "no renames, same as SeriesByTagToPromQL",
+			step:       "60",
+			target:     "seriesByTag('__name__=~cpu.*','env=prod')",
+			renames:    nil,
+			wantStep:   "60",
+			wantPromQL: `{__name__=~"cpu.*", env="prod"}`,
+		},
+		{
+			name:       "rename collision: existing tag overwritten",
+			step:       "30",
+			target:     "seriesByTag('name=metric.path','__graphite__=old')",
+			renames:    map[string]string{"name": "__graphite__"},
+			wantStep:   "30",
+			wantPromQL: `{__graphite__="metric.path"}`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotStep, gotPromQL := SeriesByTagToPromQLWithRenames(tt.step, tt.target, tt.renames)
+			assert.Equal(t, tt.wantStep, gotStep)
+			assert.Equal(t, tt.wantPromQL, gotPromQL)
+		})
+	}
+}

zipper/protocols/victoriametrics/fetch.go

@@ -33,7 +33,7 @@ func (c *VictoriaMetricsGroup) Fetch(ctx context.Context, request *protov3.Multi
 	logger := c.logger.With(zap.String("type", "fetch"), zap.String("request", request.String()))
 	stats := &types.Stats{}
 	var serverUrl string
-	if len(c.vmClusterTenantID) > 0 {
+	if c.vmClusterTenantID != "" {
 		serverUrl = fmt.Sprintf("http://127.0.0.1/select/%s/prometheus/api/v1/query_range", c.vmClusterTenantID)
 	} else {
 		serverUrl = "http://127.0.0.1/api/v1/query_range"
@@ -75,8 +75,11 @@ func (c *VictoriaMetricsGroup) Fetch(ctx context.Context, request *protov3.Multi
 			// Make local copy
 			stepLocalStr := target.step
 			if strings.HasPrefix(target.name, "seriesByTag") {
-				target.name = strings.ReplaceAll(target.name, "'name=", "'__name__=")
-				stepLocalStr, target.name = helpers.SeriesByTagToPromQL(stepLocalStr, target.name)
+				// VictoriaMetrics stores Graphite metric paths in the __graphite__ label,
+				// so rename the Graphite "name" tag accordingly before building PromQL.
+				stepLocalStr, target.name = helpers.SeriesByTagToPromQLWithRenames(stepLocalStr, target.name, map[string]string{
+					"name": "__graphite__",
+				})
 			} else {
 				target.name = fmt.Sprintf("{__graphite__=%q}", target.name)
 			}