fix(target-allocator): sort labels in processTargetGroups to prevent hash collisions

.chloggen/fix-ta-sorted-labels.yaml

@@ -0,0 +1,19 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: target allocator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix silent target loss when group labels are present in static_configs by sorting labels globally in processTargetGroups.
+
+# One or more tracking issues related to the change
+issues: [4967]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: |
+  ScratchBuilder.Labels() serializes labels in insertion order. When group labels sort
+  alphabetically after target labels (e.g. vendor > __address__), Labels.Get() early
+  termination returns empty, causing hash collisions that silently drop targets.

cmd/otel-allocator/internal/target/discovery.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"hash"
 	"hash/fnv"
-	"maps"
 	"slices"
 	"sync"
 	"time"
@@ -207,40 +206,41 @@ func (m *Discoverer) processTargetGroups(jobName string, groups []*targetgroup.G
 	groupBuilder := labels.NewScratchBuilder(labelBuilderPreallocSize)
 
 	// a slice for sorting target label names, we allocate it here to avoid doing it in the hot loop
-	targetLabelNames := make([]string, 0, labelBuilderPreallocSize)
+	targetLabelNames := make([]model.LabelName, 0, labelBuilderPreallocSize)
 
 	begin := time.Now()
 	defer func() {
 		m.processTargetGroupsDuration.Record(context.Background(), time.Since(begin).Seconds(), metric.WithAttributes(attribute.String("job.name", jobName)))
 	}()
 	var count float64
 	index := 0
+	// Reusable slice for sorted group labels.
+	groupSlice := make([]labels.Label, 0, labelBuilderPreallocSize)
+	// Overwrite target for group labels — reuses internal buffer across groups.
+	var groupLabels labels.Labels
+
 	for _, tg := range groups {
 		groupBuilder.Reset()
 		for ln, lv := range tg.Labels {
 			groupBuilder.Add(string(ln), string(lv))
 		}
 		groupBuilder.Sort()
+		// Overwrite reuses the builder's internal buffer (no allocation after first group).
+		groupBuilder.Overwrite(&groupLabels)
+		groupSlice = groupSlice[:0]
+		groupLabels.Range(func(l labels.Label) {
+			groupSlice = append(groupSlice, l)
+		})
+
 		for _, t := range tg.Targets {
 			count++
-			// ScratchBuilder is a struct containing a slice of labels. By assigning to a new variable, we get a copy
-			// of the struct, with a new slice pointing to the same underlying array. As long as we don't mutate the
-			// original slice and only append to it, we can avoid copying the group labels.
-			targetBuilder := groupBuilder
+			// Reuse groupBuilder for per-target merged labels.
+			targetBuilder := &groupBuilder
+			targetBuilder.Reset()
+
 			targetLabelNames = targetLabelNames[:0]
+			mergeLabels(targetBuilder, groupSlice, t, targetLabelNames)
 
-			// We can't sort the whole builder slice, because that would modify the underlying groupBuilder. Instead,
-			// we sort the labels in a separate slice. As a result, the group labels and the target labels are sorted
-			// subslices of the builder slice, which is in itself not sorted. This is fine, as we don't care what the
-			// order of labels is - just that it's consistent, so the hash is always the same.
-			for ln := range maps.Keys(t) {
-				targetLabelNames = append(targetLabelNames, string(ln))
-			}
-			slices.Sort(targetLabelNames)
-			for _, ln := range targetLabelNames {
-				lv := t[model.LabelName(ln)]
-				targetBuilder.Add(ln, string(lv))
-			}
 			item := NewItem(jobName, string(t[model.AddressLabel]), targetBuilder.Labels(), "")
 			intoTargets[index] = item
 			index++
@@ -249,6 +249,39 @@ func (m *Discoverer) processTargetGroups(jobName string, groups []*targetgroup.G
 	m.targetsDiscovered.Record(context.Background(), count, metric.WithAttributes(attribute.String("job.name", jobName)))
 }
 
+// mergeLabels merges sorted group labels with target labels into the builder.
+// Target labels override group labels on name collision.
+func mergeLabels(builder *labels.ScratchBuilder, groupSlice []labels.Label, targetLabels model.LabelSet, targetLabelNamesBuf []model.LabelName) {
+	for ln := range targetLabels {
+		targetLabelNamesBuf = append(targetLabelNamesBuf, ln)
+	}
+	slices.Sort(targetLabelNamesBuf)
+
+	gi, ti := 0, 0
+	for gi < len(groupSlice) && ti < len(targetLabelNamesBuf) {
+		gn := groupSlice[gi].Name
+		tn := string(targetLabelNamesBuf[ti])
+		switch {
+		case gn < tn:
+			builder.Add(gn, groupSlice[gi].Value)
+			gi++
+		case gn > tn:
+			builder.Add(tn, string(targetLabels[targetLabelNamesBuf[ti]]))
+			ti++
+		default: // target label overrides group label
+			builder.Add(tn, string(targetLabels[targetLabelNamesBuf[ti]]))
+			gi++
+			ti++
+		}
+	}
+	for ; gi < len(groupSlice); gi++ {
+		builder.Add(groupSlice[gi].Name, groupSlice[gi].Value)
+	}
+	for ; ti < len(targetLabelNamesBuf); ti++ {
+		builder.Add(string(targetLabelNamesBuf[ti]), string(targetLabels[targetLabelNamesBuf[ti]]))
+	}
+}
+
 // Run receives and saves target set updates and triggers the scraping loops reloading.
 // Reloading happens in the background so that it doesn't block receiving targets updates.
 func (m *Discoverer) run(tsets <-chan map[string][]*targetgroup.Group) error {

cmd/otel-allocator/internal/target/merge_labels_test.go

@@ -0,0 +1,136 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package target
+
+import (
+	"testing"
+
+	"github.com/prometheus/common/model"
+	"github.com/prometheus/prometheus/model/labels"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMergeLabels(t *testing.T) {
+	tests := []struct {
+		name         string
+		groupLabels  []labels.Label
+		targetLabels model.LabelSet
+		expected     labels.Labels
+	}{
+		{
+			name:     "empty inputs",
+			expected: labels.EmptyLabels(),
+		},
+		{
+			name:        "only group labels",
+			groupLabels: []labels.Label{{Name: "env", Value: "prod"}, {Name: "region", Value: "us"}},
+			expected:    labels.FromStrings("env", "prod", "region", "us"),
+		},
+		{
+			name:         "only target labels",
+			targetLabels: model.LabelSet{"__address__": "localhost:9090"},
+			expected:     labels.FromStrings("__address__", "localhost:9090"),
+		},
+		{
+			name:         "interleaved merge",
+			groupLabels:  []labels.Label{{Name: "env", Value: "prod"}},
+			targetLabels: model.LabelSet{"__address__": "localhost:9090"},
+			expected:     labels.FromStrings("__address__", "localhost:9090", "env", "prod"),
+		},
+		{
+			name:         "group label sorts after target label (original bug scenario)",
+			groupLabels:  []labels.Label{{Name: "vendor", Value: "nginx"}},
+			targetLabels: model.LabelSet{"__address__": "https://target-alpha.example.com:8393/"},
+			expected:     labels.FromStrings("__address__", "https://target-alpha.example.com:8393/", "vendor", "nginx"),
+		},
+		{
+			name:         "target overrides group on collision",
+			groupLabels:  []labels.Label{{Name: "job", Value: "from-group"}},
+			targetLabels: model.LabelSet{"job": "from-target"},
+			expected:     labels.FromStrings("job", "from-target"),
+		},
+		{
+			name:        "multiple labels both sides",
+			groupLabels: []labels.Label{{Name: "env", Value: "prod"}, {Name: "namespace", Value: "monitoring"}},
+			targetLabels: model.LabelSet{
+				"__address__":      "localhost:9090",
+				"__metrics_path__": "/metrics",
+				"__scheme__":       "https",
+			},
+			expected: labels.FromStrings(
+				"__address__", "localhost:9090",
+				"__metrics_path__", "/metrics",
+				"__scheme__", "https",
+				"env", "prod",
+				"namespace", "monitoring",
+			),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			builder := labels.NewScratchBuilder(16)
+			targetLabelNamesBuf := make([]model.LabelName, 0, len(tt.targetLabels))
+			mergeLabels(&builder, tt.groupLabels, tt.targetLabels, targetLabelNamesBuf)
+			result := builder.Labels()
+			assert.Equal(t, tt.expected, result)
+
+			// Verify output is sorted
+			var prevName string
+			result.Range(func(l labels.Label) {
+				if prevName != "" {
+					require.Less(t, prevName, l.Name, "Labels must be sorted")
+				}
+				prevName = l.Name
+			})
+		})
+	}
+}
+
+// TestSortedLabelsBlackboxRelabeling verifies that when group labels sort
+// alphabetically after target labels (e.g. vendor > __address__), the merged
+// Labels are globally sorted. This ensures Labels.Get() (binary search) works
+// correctly, preventing hash collisions that silently drop targets.
+func TestSortedLabelsBlackboxScenario(t *testing.T) {
+	groupLabels := []labels.Label{{Name: "vendor", Value: "nginx"}}
+	addresses := []string{
+		"https://target-alpha.example.com:8393/",
+		"https://target-beta.example.com:8393/",
+	}
+
+	var items []*Item
+	targetLabelNamesBuf := make([]model.LabelName, 0, 1)
+	for _, addr := range addresses {
+		builder := labels.NewScratchBuilder(16)
+		targetLabels := model.LabelSet{model.AddressLabel: model.LabelValue(addr)}
+		targetLabelNamesBuf = targetLabelNamesBuf[:0]
+		mergeLabels(&builder, groupLabels, targetLabels, targetLabelNamesBuf)
+		items = append(items, NewItem("blackbox-test", addr, builder.Labels(), ""))
+	}
+
+	// Verify labels are sorted
+	for _, item := range items {
+		var prevName string
+		item.Labels.Range(func(l labels.Label) {
+			if prevName != "" {
+				assert.Less(t, prevName, l.Name, "Labels must be sorted")
+			}
+			prevName = l.Name
+		})
+	}
+
+	// Verify Get("__address__") works on each item (would fail on unsorted labels)
+	for i, item := range items {
+		addr := item.Labels.Get("__address__")
+		assert.Equal(t, addresses[i], addr, "Labels.Get must return the correct address")
+	}
+
+	// Verify unique hashes — identical hashes would mean target loss
+	hashes := make(map[ItemHash]bool)
+	for _, item := range items {
+		hashes[item.Hash()] = true
+	}
+	assert.Len(t, hashes, 2, "Each target must have a unique hash")
+}