Rebuild targets on scrape config regex-only changes (open-telemetry#2171)

Author: swiatekm

.chloggen/fix_ta-regex-hashing.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. operator, target allocator, 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: Rebuild targets on scrape config regex-only changes
+
+# One or more tracking issues related to the change
+issues: [1358,1926]
+
+# (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:

cmd/otel-allocator/target/discovery.go

@@ -15,14 +15,17 @@
 package target
 
 import (
+	"hash"
+	"hash/fnv"
+
 	"github.com/go-logr/logr"
-	"github.com/mitchellh/hashstructure"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"github.com/prometheus/common/model"
 	"github.com/prometheus/prometheus/config"
 	"github.com/prometheus/prometheus/discovery"
 	"github.com/prometheus/prometheus/model/relabel"
+	"gopkg.in/yaml.v3"
 
 	allocatorWatcher "github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/watcher"
 )
@@ -40,7 +43,7 @@ type Discoverer struct {
 	close                chan struct{}
 	configsMap           map[allocatorWatcher.EventSource]*config.Config
 	hook                 discoveryHook
-	scrapeConfigsHash    uint64
+	scrapeConfigsHash    hash.Hash
 	scrapeConfigsUpdater scrapeConfigsUpdater
 }
 
@@ -82,7 +85,7 @@ func (m *Discoverer) ApplyConfig(source allocatorWatcher.EventSource, cfg *confi
 		}
 	}
 
-	hash, err := hashstructure.Hash(jobToScrapeConfig, nil)
+	hash, err := getScrapeConfigHash(jobToScrapeConfig)
 	if err != nil {
 		return err
 	}
@@ -131,3 +134,23 @@ func (m *Discoverer) Watch(fn func(targets map[string]*Item)) error {
 func (m *Discoverer) Close() {
 	close(m.close)
 }
+
+// Calculate a hash for a scrape config map.
+// This is done by marshaling to YAML because it's the most straightforward and doesn't run into problems with unexported fields.
+func getScrapeConfigHash(jobToScrapeConfig map[string]*config.ScrapeConfig) (hash.Hash64, error) {
+	var err error
+	hash := fnv.New64()
+	yamlEncoder := yaml.NewEncoder(hash)
+	for jobName, scrapeConfig := range jobToScrapeConfig {
+		_, err = hash.Write([]byte(jobName))
+		if err != nil {
+			return nil, err
+		}
+		err = yamlEncoder.Encode(scrapeConfig)
+		if err != nil {
+			return nil, err
+		}
+	}
+	yamlEncoder.Close()
+	return hash, err
+}

cmd/otel-allocator/target/discovery_test.go

@@ -17,6 +17,7 @@ package target
 import (
 	"context"
 	"errors"
+	"hash"
 	"sort"
 	"testing"
 	"time"
@@ -252,6 +253,33 @@ func TestDiscovery_ScrapeConfigHashing(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "different regex",
+			cfg: &promconfig.Config{
+				ScrapeConfigs: []*promconfig.ScrapeConfig{
+					{
+						JobName:         "serviceMonitor/testapp/testapp/1",
+						HonorTimestamps: false,
+						ScrapeTimeout:   model.Duration(30 * time.Second),
+						MetricsPath:     "/metrics",
+						Scheme:          "http",
+						HTTPClientConfig: commonconfig.HTTPClientConfig{
+							FollowRedirects: true,
+						},
+						RelabelConfigs: []*relabel.Config{
+							{
+								SourceLabels: model.LabelNames{model.LabelName("job")},
+								Separator:    ";",
+								Regex:        relabel.MustNewRegexp("(.+)"),
+								TargetLabel:  "__tmp_prometheus_job_name",
+								Replacement:  "$$1",
+								Action:       relabel.Replace,
+							},
+						},
+					},
+				},
+			},
+		},
 		{
 			description: "mock error on update - no hash update",
 			cfg: &promconfig.Config{
@@ -263,39 +291,9 @@ func TestDiscovery_ScrapeConfigHashing(t *testing.T) {
 			},
 			expectErr: true,
 		},
-		// {
-		// TODO: fix handler logic so this test passes.
-		// This test currently fails due to the regexp struct not having any
-		// exported fields for the hashing algorithm to hash on, causing the
-		// hashes to be the same even though the data is different.
-		// 	description: "different regex",
-		// 	cfg: &promconfig.Config{
-		// 		ScrapeConfigs: []*promconfig.ScrapeConfig{
-		// 			{
-		// 				JobName:         "serviceMonitor/testapp/testapp/1",
-		// 				HonorTimestamps: false,
-		// 				ScrapeTimeout:   model.Duration(30 * time.Second),
-		// 				MetricsPath:     "/metrics",
-		// 				HTTPClientConfig: commonconfig.HTTPClientConfig{
-		// 					FollowRedirects: true,
-		// 				},
-		// 				RelabelConfigs: []*relabel.Config{
-		// 					{
-		// 						SourceLabels: model.LabelNames{model.LabelName("job")},
-		// 						Separator:    ";",
-		// 						Regex:        relabel.MustNewRegexp("something else"),
-		// 						TargetLabel:  "__tmp_prometheus_job_name",
-		// 						Replacement:  "$$1",
-		// 						Action:       relabel.Replace,
-		// 					},
-		// 				},
-		// 			},
-		// 		},
-		// 	},
-		// },
 	}
 	var (
-		lastValidHash   uint64
+		lastValidHash   hash.Hash
 		expectedConfig  map[string]*promconfig.ScrapeConfig
 		lastValidConfig map[string]*promconfig.ScrapeConfig
 	)