Repository: open-telemetry/opentelemetry-operator

Issue:
Same target from two different jobs missing after targetallocator upgrade 0.121.0+
### Component(s)

target allocator

### What happened?

## Description
In a practical case, this happens when two different jobs point to the same target but each job has a different metrics_path. Now the target is only scraped for one of the jobs, whereas before it was scraped for both.

I was able to change of the unit tests to reproduce this and attach a debugger. I have narrowed it down to this PR: https://github.com/open-telemetry/opentelemetry-operator/pull/3832 that changes the hash for a target from `t.JobName + t.TargetURL + strconv.FormatUint(t.Labels.Hash(), 10)` to `t.Labels.Hash()`.

The `targetgroup` map coming from the Prometheus discovery manager does not contain job as a label when the targets are being processed by the Target Allocator and stored in the target list:

<img width="417" alt="Image" src="https://github.com/user-attachments/assets/ed0deca3-e76e-41c4-8ae3-da6fec6c8e65" />

`Job` is the key to a `targetgroup` object in the map, which does have the `address` as a label per target. This is also not added a label when processing the targets right before hashing them: https://github.com/swiatekm/opentelemetry-operator/blob/main/cmd/otel-allocator/internal/target/discovery.go#L195. The metrics path is also not a label at the time.

## Steps to Reproduce
Use a scrape config pointing to the same target from two different jobs for Target Allocator version 0.121.0 and up:

```yaml
config:
  scrape_configs:
  - job_name: prometheus
    static_configs:
    - targets: ["prom.domain:9001", "prom.domain:9002", "prom.domain:9003"]
  - job_name: prometheus2
    static_configs:
    - targets: ["prom.domain:9001"]
```

## Expected Result
Target `prom.domain:9001` will be allocated to a collector for both jobs `prometheus` or `prometheus2`.

## Actual Result
Target `prom.domain:9001` will be allocated to a collector for only one job of `prometheus` or `prometheus2`, but not both.


### Kubernetes Version

1.31.8

### Operator version

v0.121.0+

### Collector version

v0.121.0+

### Environment information

## Environment
OS: "Ubuntu 20.04"




### Log output

```shell

```

### Additional context

_No response_

Discussion:
@swiatekm: Are you sure this also happens if the metric paths are different? The logic behind #3832 was that all the data that used to go into the hash calculation was present in the labels anyway. And that is [definitely true](https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config) for `__address__` and `__metrics_path__`, but for the job name is only true when using Prometheus CRs. So there's definitely a bug here, and it should be resolved by adding the job name label for the purpose of hash calculation.

Thank you for the report and detailed reproduction!@gracewehner: Thanks @swiatekm for the quick reply. The above debugger screenshot is for the config:
```
config:
  scrape_configs:
  - job_name: prometheus
    metrics_path: /metrics1
    static_configs:
    - targets: ["prom.domain:9001", "prom.domain:9002", "prom.domain:9003"]
  - job_name: prometheus2
    metrics_path: /metrics2
    static_configs:
    - targets: ["prom.domain:9001"]
```

I was surprised to not see the `__metrics_path__` labels there too, along with job.

I looked into it more now just to double-check and the labels in the targetgroup and target returned by the discovery manager are the bare minimum with __address__ and some static labels. Then the scrape manager later takes these and combines these with more labels from the scrape config for job, __metrics_path__, etc: https://github.com/prometheus/prometheus/blob/main/scrape/target.go#L445

Discovery manager target groups: https://github.com/prometheus/prometheus/blob/main/discovery/targetgroup/targetgroup.go

I am happy to help with making a PR with a fix, but I'm not sure what strategy we want to take for the fix
@swiatekm: That's a bit annoying. I don't want to revert the entirety of https://github.com/open-telemetry/opentelemetry-operator/pull/3832 because it's conceptually sound and the performance improvement is significant. Unfortunately, Prometheus's `labels.Labels` don't let us pass in our own hasher. However, the implementation isn't [particularly complicated](https://github.com/prometheus/prometheus/blob/f1db64f4c513f9b0abaed6cac96646fadfe485ca/model/labels/labels.go#L72), so I think the simplest fix is to copy it and add the job name and target url at the end, therefore restoring previous behaviour. Afterwards, we can rethink whether we want to mirror Prometheus' label initialization logic or do something bespoke. Does that make sense?@gracewehner: Ok sounds good, thanks, that makes sense to me@gracewehner: Thanks @swiatekm, made an initial PR for this, let me know what you think

Solution patch:
diff --git a/.chloggen/fix-ocp-finalizers.yaml b/.chloggen/fix-ocp-finalizers.yaml
new file mode 100644
index 00000000..689332f5
--- /dev/null
+++ b/.chloggen/fix-ocp-finalizers.yaml
@@ -0,0 +1,18 @@
+# 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: collector, target allocator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix operator RBAC for managing targetallocator finalizers on OpenShift.
+
+# One or more tracking issues related to the change
+issues: [4069]
+
+# (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: | 
+  On OpenShift `OwnerReferencesPermissionEnforcement` is enabled by default, which requires the operator to have permissions to remove finalizers from resources it owns.
+  https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement
diff --git a/.chloggen/labelshash.yaml b/.chloggen/labelshash.yaml
new file mode 100644
index 00000000..9374cf89
--- /dev/null
+++ b/.chloggen/labelshash.yaml
@@ -0,0 +1,16 @@
+# 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: 'Fixes an issue where the same target from two different jobs was being allocated for only one job'
+
+# One or more tracking issues related to the change
+issues: [4044]
+
+# (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:
diff --git a/bundle/community/manifests/opentelemetry-operator.clusterserviceversion.yaml b/bundle/community/manifests/opentelemetry-operator.clusterserviceversion.yaml
index 715530b0..f5240414 100644
--- a/bundle/community/manifests/opentelemetry-operator.clusterserviceversion.yaml
+++ b/bundle/community/manifests/opentelemetry-operator.clusterserviceversion.yaml
@@ -99,7 +99,7 @@ metadata:
     categories: Logging & Tracing,Monitoring
     certified: "false"
     containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator
-    createdAt: "2025-05-29T15:24:23Z"
+    createdAt: "2025-06-03T17:44:53Z"
     description: Provides the OpenTelemetry components, including the Collector
     operators.operatorframework.io/builder: operator-sdk-v1.29.0
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
@@ -436,6 +436,7 @@ spec:
           resources:
           - opampbridges
           - targetallocators
+          - targetallocators/finalizers
           verbs:
           - create
           - delete
diff --git a/bundle/openshift/manifests/opentelemetry-operator.clusterserviceversion.yaml b/bundle/openshift/manifests/opentelemetry-operator.clusterserviceversion.yaml
index aab87683..39a7a093 100644
--- a/bundle/openshift/manifests/opentelemetry-operator.clusterserviceversion.yaml
+++ b/bundle/openshift/manifests/opentelemetry-operator.clusterserviceversion.yaml
@@ -99,7 +99,7 @@ metadata:
     categories: Logging & Tracing,Monitoring
     certified: "false"
     containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator
-    createdAt: "2025-05-29T15:24:23Z"
+    createdAt: "2025-06-03T17:44:53Z"
     description: Provides the OpenTelemetry components, including the Collector
     operators.operatorframework.io/builder: operator-sdk-v1.29.0
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
@@ -436,6 +436,7 @@ spec:
           resources:
           - opampbridges
           - targetallocators
+          - targetallocators/finalizers
           verbs:
           - create
           - delete
diff --git a/cmd/otel-allocator/internal/target/target.go b/cmd/otel-allocator/internal/target/target.go
index f2bd756d..ca59cdcc 100644
--- a/cmd/otel-allocator/internal/target/target.go
+++ b/cmd/otel-allocator/internal/target/target.go
@@ -4,6 +4,7 @@
 package target
 
 import (
+	"github.com/cespare/xxhash/v2"
 	"github.com/prometheus/prometheus/model/labels"
 )
 
@@ -33,7 +34,7 @@ type Item struct {
 
 func (t *Item) Hash() ItemHash {
 	if t.hash == 0 {
-		t.hash = ItemHash(t.Labels.Hash())
+		t.hash = ItemHash(LabelsHashWithJobName(t.Labels, t.JobName))
 	}
 	return t.hash
 }
@@ -64,3 +65,41 @@ func NewItem(jobName string, targetURL string, labels labels.Labels, collectorNa
 		CollectorName: collectorName,
 	}
 }
+
+// LabelsHashWithJobName computes a hash of the labels and the job name.
+// Same logic as Prometheus labels.Hash: https://github.com/prometheus/prometheus/blob/8fd46f74aa0155e4d5aa30654f9c02e564e03743/model/labels/labels.go#L72
+// but adds in the job name since this is not in the labelset from the discovery manager.
+// The scrape manager adds it later. Address is already included in the labels, so it is not needed here.
+func LabelsHashWithJobName(ls labels.Labels, jobName string) uint64 {
+	var sep byte = '\xff'
+	var seps = []byte{sep}
+
+	// Use xxhash.Sum64(b) for fast path as it's faster.
+	b := make([]byte, 0, 1024)
+
+	// Differs from Prometheus implementation by adding job name.
+	b = append(b, jobName...)
+	b = append(b, sep)
+
+	for i, v := range ls {
+		if len(b)+len(v.Name)+len(v.Value)+2 >= cap(b) {
+			// If labels entry is 1KB+ do not allocate whole entry.
+			h := xxhash.New()
+			_, _ = h.Write(b)
+			for _, v := range ls[i:] {
+				_, _ = h.WriteString(v.Name)
+				_, _ = h.Write(seps)
+				_, _ = h.WriteString(v.Value)
+				_, _ = h.Write(seps)
+			}
+			return h.Sum64()
+		}
+
+		b = append(b, v.Name...)
+		b = append(b, sep)
+		b = append(b, v.Value...)
+		b = append(b, sep)
+	}
+
+	return xxhash.Sum64(b)
+}
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
index a03aeb18..9c5b3a20 100644
--- a/config/rbac/role.yaml
+++ b/config/rbac/role.yaml
@@ -136,6 +136,7 @@ rules:
   resources:
   - opampbridges
   - targetallocators
+  - targetallocators/finalizers
   verbs:
   - create
   - delete
diff --git a/internal/controllers/opentelemetrycollector_controller.go b/internal/controllers/opentelemetrycollector_controller.go
index afca35b7..ab97d74b 100644
--- a/internal/controllers/opentelemetrycollector_controller.go
+++ b/internal/controllers/opentelemetrycollector_controller.go
@@ -225,6 +225,7 @@ func NewReconciler(p Params) *OpenTelemetryCollectorReconciler {
 // +kubebuilder:rbac:groups=opentelemetry.io,resources=opentelemetrycollectors/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=opentelemetry.io,resources=opentelemetrycollectors/finalizers,verbs=get;update;patch
 // +kubebuilder:rbac:groups=opentelemetry.io,resources=targetallocators,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=opentelemetry.io,resources=targetallocators/finalizers,verbs=get;list;watch;create;update;patch;delete
 
 // Reconcile the current state of an OpenTelemetry collector resource with the desired state.
 func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {



Test patch:
diff --git a/cmd/otel-allocator/internal/target/discovery_test.go b/cmd/otel-allocator/internal/target/discovery_test.go
index c2fbc087..cf59a7a7 100644
--- a/cmd/otel-allocator/internal/target/discovery_test.go
+++ b/cmd/otel-allocator/internal/target/discovery_test.go
@@ -74,12 +74,12 @@ func TestDiscovery(t *testing.T) {
 	defer cancelFunc()
 
 	go func() {
-		err := d.Run()
-		assert.Error(t, err)
+		runErr := d.Run()
+		assert.Error(t, runErr)
 	}()
 	go func() {
-		err := manager.Run()
-		assert.NoError(t, err)
+		runErr := manager.Run()
+		assert.NoError(t, runErr)
 	}()
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -341,6 +341,106 @@ func TestDiscovery_ScrapeConfigHashing(t *testing.T) {
 	}
 }
 
+func TestDiscoveryTargetHashing(t *testing.T) {
+	tests := []struct {
+		description string
+		cfg         *promconfig.Config
+	}{
+		{
+			description: "same targets in two different jobs",
+			cfg: &promconfig.Config{
+				ScrapeConfigs: []*promconfig.ScrapeConfig{
+					{
+						JobName:         "prometheus",
+						HonorTimestamps: true,
+						ScrapeInterval:  model.Duration(30 * time.Second),
+						ScrapeProtocols: defaultScrapeProtocols,
+						ScrapeTimeout:   model.Duration(30 * time.Second),
+						MetricsPath:     "/metrics",
+						Scheme:          "http",
+						ServiceDiscoveryConfigs: discovery.Configs{
+							discovery.StaticConfig{
+								{
+									Targets: []model.LabelSet{
+										{"__address__": "prom.domain:9001"},
+										{"__address__": "prom.domain:9002"},
+										{"__address__": "prom.domain:9003"},
+									},
+								},
+							},
+						},
+					},
+					{
+						JobName:         "prometheus2",
+						HonorTimestamps: true,
+						ScrapeInterval:  model.Duration(30 * time.Second),
+						ScrapeProtocols: defaultScrapeProtocols,
+						ScrapeTimeout:   model.Duration(30 * time.Second),
+						MetricsPath:     "/metrics2",
+						Scheme:          "http",
+						ServiceDiscoveryConfigs: discovery.Configs{
+							discovery.StaticConfig{
+								{
+									Targets: []model.LabelSet{
+										{"__address__": "prom.domain:9001"},
+										{"__address__": "prom.domain:9002"},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	scu := &mockScrapeConfigUpdater{}
+	ctx, cancelFunc := context.WithCancel(context.Background())
+	registry := prometheus.NewRegistry()
+	sdMetrics, err := discovery.CreateAndRegisterSDMetrics(registry)
+	require.NoError(t, err)
+	d := discovery.NewManager(ctx, config.NopLogger, registry, sdMetrics)
+	results := make(chan []*Item)
+	manager := NewDiscoverer(ctrl.Log.WithName("test"), d, nil, scu, func(targets []*Item) {
+		var result []*Item
+		result = append(result, targets...)
+		results <- result
+	})
+
+	defer manager.Close()
+	defer cancelFunc()
+
+	go func() {
+		runErr := d.Run()
+		assert.Error(t, runErr)
+	}()
+	go func() {
+		runErr := manager.Run()
+		assert.NoError(t, runErr)
+	}()
+
+	for _, tt := range tests {
+		t.Run(tt.description, func(t *testing.T) {
+			assert.NoError(t, err)
+			assert.True(t, len(tt.cfg.ScrapeConfigs) > 0)
+			err = manager.ApplyConfig(allocatorWatcher.EventSourcePrometheusCR, tt.cfg.ScrapeConfigs)
+			assert.NoError(t, err)
+
+			gotTargets := <-results
+
+			// Verify that all targets have different hashes
+			targetHashes := make(map[ItemHash]bool)
+			for _, target := range gotTargets {
+				hash := target.Hash()
+				if _, exists := targetHashes[hash]; exists {
+					t.Errorf("Duplicate hash %d found for target %s (%s)", hash, target.TargetURL, target.JobName)
+				}
+				targetHashes[hash] = true
+			}
+			assert.Equal(t, len(gotTargets), len(targetHashes), "Number of unique hashes should match number of targets")
+		})
+	}
+}
+
 func TestDiscovery_NoConfig(t *testing.T) {
 	scu := &mockScrapeConfigUpdater{mockCfg: map[string]*promconfig.ScrapeConfig{}}
 	ctx, cancelFunc := context.WithCancel(context.Background())
diff --git a/tests/e2e/operator-metrics/00-assert-operator.yaml b/tests/e2e/operator-metrics/00-assert-operator.yaml
deleted file mode 100644
index 62e378bd..00000000
--- a/tests/e2e/operator-metrics/00-assert-operator.yaml
+++ /dev/null
@@ -1,17 +0,0 @@
-apiVersion: v1
-kind: Pod
-metadata:
-  namespace: opentelemetry-operator-system
-  labels:
-    app.kubernetes.io/name: opentelemetry-operator
-    control-plane: controller-manager
-  namespace: opentelemetry-operator-system
-status:
-  containerStatuses:
-  - name: kube-rbac-proxy
-    ready: true
-    started: true
-  - name: manager
-    ready: true
-    started: true
-  phase: Running
diff --git a/tests/e2e/operator-metrics/assert-operator.yaml b/tests/e2e/operator-metrics/assert-operator.yaml
new file mode 100644
index 00000000..a61e4395
--- /dev/null
+++ b/tests/e2e/operator-metrics/assert-operator.yaml
@@ -0,0 +1,34 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  namespace: opentelemetry-operator-system
+  labels:
+    app.kubernetes.io/name: opentelemetry-operator
+    control-plane: controller-manager
+  namespace: ($otelnamespace)
+status:
+  containerStatuses:
+  - name: kube-rbac-proxy
+    ready: true
+    started: true
+  - name: manager
+    ready: true
+    started: true
+  phase: Running
+
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: opentelemetry-operator-controller-manager-metrics-service
+  namespace: ($otelnamespace)
+spec:
+  ports:
+  - name: https
+    port: 8443
+    protocol: TCP
+    targetPort: https
+  selector:
+    app.kubernetes.io/name: opentelemetry-operator
+    control-plane: controller-manager
+  type: ClusterIP
diff --git a/tests/e2e/operator-metrics/chainsaw-test.yaml b/tests/e2e/operator-metrics/chainsaw-test.yaml
index ac052308..bef56eb9 100644
--- a/tests/e2e/operator-metrics/chainsaw-test.yaml
+++ b/tests/e2e/operator-metrics/chainsaw-test.yaml
@@ -4,19 +4,33 @@ metadata:
   name: operator-metrics
 spec:
   steps:
-    - name: step-00
+    - name: Assert operator metrics.
       try:
+        - command:
+            entrypoint: kubectl
+            args:
+            - get
+            - pods
+            - -A
+            - -l control-plane=controller-manager
+            - -l app.kubernetes.io/name=opentelemetry-operator
+            - -o 
+            - jsonpath={.items[0].metadata.namespace}
+            outputs:
+            - name: otelnamespace
+              value: ($stdout)
         - assert:
-            file: 00-assert-operator.yaml
-    - name: proxy request for operator metrics
-      try:
+            file: assert-operator.yaml
         - command:
+            env:
+              - name: otelnamespace
+                value: ($otelnamespace)
             entrypoint: kubectl
             args:
               - get
               - service
               - -n
-              - opentelemetry-operator-system
+              - $otelnamespace
               - -l
               - control-plane=controller-manager
               - -o