From ad69899407393b7c72b2420da8788cea9a2a0559 Mon Sep 17 00:00:00 2001 From: darshanime Date: Thu, 25 Jan 2024 19:50:28 +0530 Subject: [PATCH 01/10] Add warning log for same labelset after relabeling Signed-off-by: darshanime --- discovery/targetgroup/targetgroup.go | 2 +- scrape/manager.go | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/discovery/targetgroup/targetgroup.go b/discovery/targetgroup/targetgroup.go index e74870f046..cf9d8c6158 100644 --- a/discovery/targetgroup/targetgroup.go +++ b/discovery/targetgroup/targetgroup.go @@ -20,7 +20,7 @@ import ( "github.com/prometheus/common/model" ) -// Group is a set of targets with a common label set(production , test, staging etc.). +// Group is a set of targets with a common label set(production, test, staging etc). type Group struct { // Targets is a list of targets identified by a label set. Each target is // uniquely identifiable in the group by its address label. diff --git a/scrape/manager.go b/scrape/manager.go index 04da3162e6..259a645d79 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -197,6 +197,7 @@ func (m *Manager) reload() { } wg.Add(1) + // Run the sync in parallel as these take a while and at high load can't catch up. go func(sp *scrapePool, groups []*targetgroup.Group) { sp.Sync(groups) @@ -205,6 +206,21 @@ func (m *Manager) reload() { } m.mtxScrape.Unlock() wg.Wait() + + activeTargets := make(map[uint64]*Target) + for _, scrapePool := range m.scrapePools { + for _, target := range scrapePool.activeTargets { + if t, ok := activeTargets[target.labels.Hash()]; ok { + level.Warn(m.logger).Log( + "msg", "Found targets with same labels after relabelling", + "target", t.URL().String(), + "target", target.URL().String(), + ) + } + activeTargets[target.labels.Hash()] = target + } + } + } // setOffsetSeed calculates a global offsetSeed per server relying on extra label set. From 49e6bdade5cefd163ddcb06e0318feefc62b97f7 Mon Sep 17 00:00:00 2001 From: darshanime Date: Fri, 26 Jan 2024 12:23:50 +0530 Subject: [PATCH 02/10] Add test for same label set in targets Signed-off-by: darshanime --- scrape/manager.go | 5 ++--- scrape/manager_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/scrape/manager.go b/scrape/manager.go index 259a645d79..cba026c1b2 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -213,14 +213,13 @@ func (m *Manager) reload() { if t, ok := activeTargets[target.labels.Hash()]; ok { level.Warn(m.logger).Log( "msg", "Found targets with same labels after relabelling", - "target", t.URL().String(), - "target", target.URL().String(), + "target", t.DiscoveredLabels().Get(model.AddressLabel), + "target", target.DiscoveredLabels().Get(model.AddressLabel), ) } activeTargets[target.labels.Hash()] = target } } - } // setOffsetSeed calculates a global offsetSeed per server relying on extra label set. diff --git a/scrape/manager_test.go b/scrape/manager_test.go index 75ac9ea692..7d5e63e008 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -622,6 +622,32 @@ func TestManagerTargetsUpdates(t *testing.T) { } } +func TestManagerDuplicateAfterRelabellingWarning(t *testing.T) { + var output []interface{} + logger := log.Logger(log.LoggerFunc(func(keyvals ...interface{}) error { + output = keyvals + return nil + })) + + opts := Options{} + testRegistry := prometheus.NewRegistry() + m, err := NewManager(&opts, logger, nil, testRegistry) + require.NoError(t, err) + + m.scrapePools = map[string]*scrapePool{} + sp := &scrapePool{ + activeTargets: map[uint64]*Target{}, + } + sp.activeTargets[uint64(0)] = &Target{discoveredLabels: labels.FromStrings("__address__", "foo")} + sp.activeTargets[uint64(1)] = &Target{discoveredLabels: labels.FromStrings("__address__", "bar")} + m.scrapePools["default"] = sp + + m.reload() + require.Contains(t, output, "Found targets with same labels after relabelling") + require.Contains(t, output, "foo") + require.Contains(t, output, "bar") +} + func TestSetOffsetSeed(t *testing.T) { getConfig := func(prometheus string) *config.Config { cfgText := ` From cff15aed86242b94e3ff7818e2c01e85cfcc604e Mon Sep 17 00:00:00 2001 From: darshanime Date: Tue, 30 Jan 2024 07:47:22 +0530 Subject: [PATCH 03/10] Extract to separate method Signed-off-by: darshanime --- scrape/manager.go | 9 +++++++-- scrape/manager_test.go | 11 +++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/scrape/manager.go b/scrape/manager.go index cba026c1b2..ef15e647ad 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -207,14 +207,19 @@ func (m *Manager) reload() { m.mtxScrape.Unlock() wg.Wait() + m.warnIfTargetsRelabelledToSameLabels() +} + +func (m *Manager) warnIfTargetsRelabelledToSameLabels() { activeTargets := make(map[uint64]*Target) for _, scrapePool := range m.scrapePools { for _, target := range scrapePool.activeTargets { if t, ok := activeTargets[target.labels.Hash()]; ok { level.Warn(m.logger).Log( "msg", "Found targets with same labels after relabelling", - "target", t.DiscoveredLabels().Get(model.AddressLabel), - "target", target.DiscoveredLabels().Get(model.AddressLabel), + "target_one", t.DiscoveredLabels().Get(model.AddressLabel), + "target_two", target.DiscoveredLabels().Get(model.AddressLabel), + "labels", target.labels.String(), ) } activeTargets[target.labels.Hash()] = target diff --git a/scrape/manager_test.go b/scrape/manager_test.go index 7d5e63e008..47701bd1b6 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -638,14 +638,21 @@ func TestManagerDuplicateAfterRelabellingWarning(t *testing.T) { sp := &scrapePool{ activeTargets: map[uint64]*Target{}, } - sp.activeTargets[uint64(0)] = &Target{discoveredLabels: labels.FromStrings("__address__", "foo")} - sp.activeTargets[uint64(1)] = &Target{discoveredLabels: labels.FromStrings("__address__", "bar")} + sp.activeTargets[uint64(0)] = &Target{ + discoveredLabels: labels.FromStrings("__address__", "foo"), + labels: labels.FromStrings("label_key", "label_value"), + } + sp.activeTargets[uint64(1)] = &Target{ + discoveredLabels: labels.FromStrings("__address__", "bar"), + labels: labels.FromStrings("label_key", "label_value"), + } m.scrapePools["default"] = sp m.reload() require.Contains(t, output, "Found targets with same labels after relabelling") require.Contains(t, output, "foo") require.Contains(t, output, "bar") + require.Contains(t, output, `{label_key="label_value"}`) } func TestSetOffsetSeed(t *testing.T) { From d0849f45ffb73c31201b31f71f69296d9ded7a52 Mon Sep 17 00:00:00 2001 From: darshanime Date: Sat, 9 Mar 2024 15:21:18 +0530 Subject: [PATCH 04/10] Invert predicate to use !ok Signed-off-by: darshanime --- scrape/manager.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/scrape/manager.go b/scrape/manager.go index ef15e647ad..e2dc9198a1 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -214,15 +214,17 @@ func (m *Manager) warnIfTargetsRelabelledToSameLabels() { activeTargets := make(map[uint64]*Target) for _, scrapePool := range m.scrapePools { for _, target := range scrapePool.activeTargets { - if t, ok := activeTargets[target.labels.Hash()]; ok { - level.Warn(m.logger).Log( - "msg", "Found targets with same labels after relabelling", - "target_one", t.DiscoveredLabels().Get(model.AddressLabel), - "target_two", target.DiscoveredLabels().Get(model.AddressLabel), - "labels", target.labels.String(), - ) + t, ok := activeTargets[target.labels.Hash()] + if !ok { + activeTargets[target.labels.Hash()] = target + continue } - activeTargets[target.labels.Hash()] = target + level.Warn(m.logger).Log( + "msg", "Found targets with same labels after relabelling", + "target_one", t.DiscoveredLabels().Get(model.AddressLabel), + "target_two", target.DiscoveredLabels().Get(model.AddressLabel), + "labels", target.labels.String(), + ) } } } From 0e098c68f7368508f237fb605674867624c8cdc9 Mon Sep 17 00:00:00 2001 From: darshanime Date: Sat, 4 May 2024 14:15:34 +0530 Subject: [PATCH 05/10] Add benchmark for warnIfTargetsRelabelledToSameLabels Signed-off-by: darshanime --- scrape/manager.go | 6 ++++-- scrape/manager_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/scrape/manager.go b/scrape/manager.go index e2dc9198a1..effcdfd6ed 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -169,6 +169,7 @@ func (m *Manager) reloader() { func (m *Manager) reload() { m.mtxScrape.Lock() var wg sync.WaitGroup + for setName, groups := range m.targetSets { if _, ok := m.scrapePools[setName]; !ok { scrapeConfig, ok := m.scrapeConfigs[setName] @@ -214,9 +215,10 @@ func (m *Manager) warnIfTargetsRelabelledToSameLabels() { activeTargets := make(map[uint64]*Target) for _, scrapePool := range m.scrapePools { for _, target := range scrapePool.activeTargets { - t, ok := activeTargets[target.labels.Hash()] + lHash := target.labels.Hash() + t, ok := activeTargets[lHash] if !ok { - activeTargets[target.labels.Hash()] = target + activeTargets[lHash] = target continue } level.Warn(m.logger).Log( diff --git a/scrape/manager_test.go b/scrape/manager_test.go index 47701bd1b6..8852704346 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -622,6 +622,31 @@ func TestManagerTargetsUpdates(t *testing.T) { } } +func BenchmarkManagerReload(b *testing.B) { + opts := Options{} + testRegistry := prometheus.NewRegistry() + m, err := NewManager(&opts, nil, nil, testRegistry) + require.NoError(b, err) + + m.scrapePools = map[string]*scrapePool{} + sp := &scrapePool{ + activeTargets: map[uint64]*Target{}, + } + + for i := 0; i < b.N; i++ { + sp.activeTargets[uint64(i)] = &Target{ + discoveredLabels: labels.FromStrings("__address__", fmt.Sprintf("foo-%d", i)), + labels: labels.FromStrings("label_key", fmt.Sprintf("foo-%d", i)), + } + } + + m.scrapePools["default"] = sp + b.ResetTimer() + for i := 0; i < b.N; i++ { + m.reload() + } +} + func TestManagerDuplicateAfterRelabellingWarning(t *testing.T) { var output []interface{} logger := log.Logger(log.LoggerFunc(func(keyvals ...interface{}) error { From b3ba91f16f5707763364f16e0b63e5d3c15382d5 Mon Sep 17 00:00:00 2001 From: darshanime Date: Fri, 10 May 2024 10:18:55 +0530 Subject: [PATCH 06/10] Preallocate map with required size Signed-off-by: darshanime --- scrape/manager.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/scrape/manager.go b/scrape/manager.go index effcdfd6ed..62a57d08bf 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -212,7 +212,15 @@ func (m *Manager) reload() { } func (m *Manager) warnIfTargetsRelabelledToSameLabels() { - activeTargets := make(map[uint64]*Target) + m.mtxScrape.Lock() + defer m.mtxScrape.Unlock() + + totalTargets := 0 + for _, scrapePool := range m.scrapePools { + totalTargets += len(scrapePool.activeTargets) + } + + activeTargets := make(map[uint64]*Target, totalTargets) for _, scrapePool := range m.scrapePools { for _, target := range scrapePool.activeTargets { lHash := target.labels.Hash() From e441baa93e14929781884b872a936d31804a1512 Mon Sep 17 00:00:00 2001 From: darshanime Date: Mon, 13 May 2024 22:48:53 +0530 Subject: [PATCH 07/10] Use label bytes as key Signed-off-by: darshanime --- scrape/manager.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/scrape/manager.go b/scrape/manager.go index 62a57d08bf..04397ff7dd 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -220,13 +220,14 @@ func (m *Manager) warnIfTargetsRelabelledToSameLabels() { totalTargets += len(scrapePool.activeTargets) } - activeTargets := make(map[uint64]*Target, totalTargets) + activeTargets := make(map[string]*Target, totalTargets) + buf := [1024]byte{} for _, scrapePool := range m.scrapePools { for _, target := range scrapePool.activeTargets { - lHash := target.labels.Hash() - t, ok := activeTargets[lHash] + lStr := string(target.labels.Bytes(buf[:])) + t, ok := activeTargets[lStr] if !ok { - activeTargets[lHash] = target + activeTargets[lStr] = target continue } level.Warn(m.logger).Log( From 36d92e57384afd8c338cd37820656f1a65674a69 Mon Sep 17 00:00:00 2001 From: darshanime Date: Mon, 13 May 2024 22:56:57 +0530 Subject: [PATCH 08/10] Fix the target count to 10k Signed-off-by: darshanime --- scrape/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scrape/manager_test.go b/scrape/manager_test.go index 8852704346..85aaef7612 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -633,7 +633,7 @@ func BenchmarkManagerReload(b *testing.B) { activeTargets: map[uint64]*Target{}, } - for i := 0; i < b.N; i++ { + for i := 0; i < 10000; i++ { sp.activeTargets[uint64(i)] = &Target{ discoveredLabels: labels.FromStrings("__address__", fmt.Sprintf("foo-%d", i)), labels: labels.FromStrings("label_key", fmt.Sprintf("foo-%d", i)), From db7a60f162dc6d176593bfbf9c430d1d2279223a Mon Sep 17 00:00:00 2001 From: darshanime Date: Mon, 13 May 2024 23:08:43 +0530 Subject: [PATCH 09/10] Revert unrelated changes Signed-off-by: darshanime --- discovery/targetgroup/targetgroup.go | 2 +- scrape/manager.go | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/discovery/targetgroup/targetgroup.go b/discovery/targetgroup/targetgroup.go index cf9d8c6158..e74870f046 100644 --- a/discovery/targetgroup/targetgroup.go +++ b/discovery/targetgroup/targetgroup.go @@ -20,7 +20,7 @@ import ( "github.com/prometheus/common/model" ) -// Group is a set of targets with a common label set(production, test, staging etc). +// Group is a set of targets with a common label set(production , test, staging etc.). type Group struct { // Targets is a list of targets identified by a label set. Each target is // uniquely identifiable in the group by its address label. diff --git a/scrape/manager.go b/scrape/manager.go index 04397ff7dd..5bbc9871d7 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -169,7 +169,6 @@ func (m *Manager) reloader() { func (m *Manager) reload() { m.mtxScrape.Lock() var wg sync.WaitGroup - for setName, groups := range m.targetSets { if _, ok := m.scrapePools[setName]; !ok { scrapeConfig, ok := m.scrapeConfigs[setName] @@ -198,7 +197,6 @@ func (m *Manager) reload() { } wg.Add(1) - // Run the sync in parallel as these take a while and at high load can't catch up. go func(sp *scrapePool, groups []*targetgroup.Group) { sp.Sync(groups) From 354a993f3b4ce2d4e3a5bf5a66bcd0c64d268166 Mon Sep 17 00:00:00 2001 From: darshanime Date: Fri, 3 Jan 2025 15:35:53 +0530 Subject: [PATCH 10/10] add warn under feature flag Signed-off-by: darshanime --- CHANGELOG.md | 1 + cmd/prometheus/main.go | 3 +++ scrape/manager.go | 16 ++++++++---- scrape/manager_test.go | 59 ++++++++++++++---------------------------- 4 files changed, 34 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0a7ef6611..dea615fb1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## unreleased + * [ENHANCEMENT] Scraping: add warning if targets relabel to same labels. This is enabled under the feature-flag `warn-if-targets-relabelled-to-same-labels`. #9589 ## 3.1.0 / 2025-01-02 diff --git a/cmd/prometheus/main.go b/cmd/prometheus/main.go index 06f46f8d72..9c3cfc9261 100644 --- a/cmd/prometheus/main.go +++ b/cmd/prometheus/main.go @@ -264,6 +264,9 @@ func (c *flagConfig) setFeatureListOptions(logger *slog.Logger) error { config.DefaultConfig.GlobalConfig.ScrapeProtocols = config.DefaultProtoFirstScrapeProtocols config.DefaultGlobalConfig.ScrapeProtocols = config.DefaultProtoFirstScrapeProtocols logger.Info("Experimental created timestamp zero ingestion enabled. Changed default scrape_protocols to prefer PrometheusProto format.", "global.scrape_protocols", fmt.Sprintf("%v", config.DefaultGlobalConfig.ScrapeProtocols)) + case "warn-if-targets-relabelled-to-same-labels": + c.scrape.EnableWarnIfTargetsRelabelledToSameLabels = true + logger.Info("Enabled warning if targets relabelled to same labels") case "delayed-compaction": c.tsdb.EnableDelayedCompaction = true logger.Info("Experimental delayed compaction is enabled.") diff --git a/scrape/manager.go b/scrape/manager.go index 5bbc9871d7..977b32b31f 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -90,6 +90,9 @@ type Options struct { // Optional HTTP client options to use when scraping. HTTPClientOptions []config_util.HTTPClientOption + // Option to warn if targets relabelled to same labels + EnableWarnIfTargetsRelabelledToSameLabels bool + // private option for testability. skipOffsetting bool } @@ -206,7 +209,9 @@ func (m *Manager) reload() { m.mtxScrape.Unlock() wg.Wait() - m.warnIfTargetsRelabelledToSameLabels() + if m.opts.EnableWarnIfTargetsRelabelledToSameLabels { + m.warnIfTargetsRelabelledToSameLabels() + } } func (m *Manager) warnIfTargetsRelabelledToSameLabels() { @@ -220,6 +225,7 @@ func (m *Manager) warnIfTargetsRelabelledToSameLabels() { activeTargets := make(map[string]*Target, totalTargets) buf := [1024]byte{} + builder := labels.NewBuilder(labels.EmptyLabels()) for _, scrapePool := range m.scrapePools { for _, target := range scrapePool.activeTargets { lStr := string(target.labels.Bytes(buf[:])) @@ -228,10 +234,10 @@ func (m *Manager) warnIfTargetsRelabelledToSameLabels() { activeTargets[lStr] = target continue } - level.Warn(m.logger).Log( - "msg", "Found targets with same labels after relabelling", - "target_one", t.DiscoveredLabels().Get(model.AddressLabel), - "target_two", target.DiscoveredLabels().Get(model.AddressLabel), + m.logger.Warn( + "Found targets with same labels after relabelling", + "target_one", t.DiscoveredLabels(builder).Get(model.AddressLabel), + "target_two", target.DiscoveredLabels(builder).Get(model.AddressLabel), "labels", target.labels.String(), ) } diff --git a/scrape/manager_test.go b/scrape/manager_test.go index 85aaef7612..606e6456ba 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -622,62 +622,41 @@ func TestManagerTargetsUpdates(t *testing.T) { } } -func BenchmarkManagerReload(b *testing.B) { - opts := Options{} - testRegistry := prometheus.NewRegistry() - m, err := NewManager(&opts, nil, nil, testRegistry) - require.NoError(b, err) - - m.scrapePools = map[string]*scrapePool{} - sp := &scrapePool{ - activeTargets: map[uint64]*Target{}, - } - - for i := 0; i < 10000; i++ { - sp.activeTargets[uint64(i)] = &Target{ - discoveredLabels: labels.FromStrings("__address__", fmt.Sprintf("foo-%d", i)), - labels: labels.FromStrings("label_key", fmt.Sprintf("foo-%d", i)), - } - } - - m.scrapePools["default"] = sp - b.ResetTimer() - for i := 0; i < b.N; i++ { - m.reload() - } -} - func TestManagerDuplicateAfterRelabellingWarning(t *testing.T) { - var output []interface{} - logger := log.Logger(log.LoggerFunc(func(keyvals ...interface{}) error { - output = keyvals - return nil - })) + var buf bytes.Buffer + writer := &buf + logger := promslog.New(&promslog.Config{Writer: writer}) - opts := Options{} + opts := Options{EnableWarnIfTargetsRelabelledToSameLabels: true} testRegistry := prometheus.NewRegistry() - m, err := NewManager(&opts, logger, nil, testRegistry) + m, err := NewManager(&opts, logger, nil, nil, testRegistry) require.NoError(t, err) m.scrapePools = map[string]*scrapePool{} sp := &scrapePool{ activeTargets: map[uint64]*Target{}, } + targetScrapeCfg := &config.ScrapeConfig{ + Scheme: "https", + MetricsPath: "/metrics", + JobName: "job", + ScrapeInterval: model.Duration(time.Second), + ScrapeTimeout: model.Duration(time.Second), + } sp.activeTargets[uint64(0)] = &Target{ - discoveredLabels: labels.FromStrings("__address__", "foo"), - labels: labels.FromStrings("label_key", "label_value"), + scrapeConfig: targetScrapeCfg, + tLabels: map[model.LabelName]model.LabelValue{model.AddressLabel: "foo"}, } sp.activeTargets[uint64(1)] = &Target{ - discoveredLabels: labels.FromStrings("__address__", "bar"), - labels: labels.FromStrings("label_key", "label_value"), + scrapeConfig: targetScrapeCfg, + tLabels: map[model.LabelName]model.LabelValue{model.AddressLabel: "bar"}, } m.scrapePools["default"] = sp m.reload() - require.Contains(t, output, "Found targets with same labels after relabelling") - require.Contains(t, output, "foo") - require.Contains(t, output, "bar") - require.Contains(t, output, `{label_key="label_value"}`) + require.Contains(t, buf.String(), "Found targets with same labels after relabelling") + require.Contains(t, buf.String(), "foo") + require.Contains(t, buf.String(), "bar") } func TestSetOffsetSeed(t *testing.T) {