Skip to content

fix(discovery): make discovery manager notify consumers of dropped targets for still defined jobs#13147

Merged
machine424 merged 1 commit into
prometheus:mainfrom
machine424:drop-targets
Aug 28, 2024
Merged

fix(discovery): make discovery manager notify consumers of dropped targets for still defined jobs#13147
machine424 merged 1 commit into
prometheus:mainfrom
machine424:drop-targets

Conversation

@machine424

@machine424 machine424 commented Nov 15, 2023

Copy link
Copy Markdown
Member

See #12858 (comment) for details (will add that to the commit message)

I added a test that reproduces: #12858 (comment) and it's failing with the code in main: https://github.com/prometheus/prometheus/actions/runs/6876250042/job/18701456148?pr=13147#step:6:378

I pushed a draft fix (see comments in the code)

fixes #12858


>>>>>>> NOTE:

This fix concerns the SD manager behind the flag new-service-discovery-manager which is the manager used by most third parties but not the one used by default by Prometheus itself, as of now, Prometheus is still using the legacymanager

package legacymanager
by default. The plan is to switch to the new manager in 3.0 #13609


@machine424

Copy link
Copy Markdown
Member Author

Comment thread scrape/manager_test.go Outdated
@machine424 machine424 changed the title scrape/manager_test.go: add a test to check that the manager gets notified for targets that got dropped by discovery. make discovery manager notify consumers of dropped targets for a still existing job Nov 15, 2023
Comment thread discovery/manager.go Outdated
Comment thread discovery/manager.go Outdated
Comment thread discovery/manager.go Outdated
Comment thread discovery/manager.go Outdated
@roidelapluie

Copy link
Copy Markdown
Member

Thanks a lot for picking up this!

@machine424

Copy link
Copy Markdown
Member Author

I don't think this will solve all cases as the scrape manager e.g. only reloads every reloadIntervalDuration (which is customizable) and during that time the targetsgroup with the empty targets we send here in cleaner maybe superseded by another one sent by another stale provider through cleaner or another new provider through sender...

So I'll need to think about another solution (that doesn't require any acking from scrape manager to keep things simple)

Comment thread discovery/manager.go Outdated
Comment thread scrape/manager.go Outdated
}

func (m *Manager) reloader() {
// TODO: OK?

@machine424 machine424 Nov 24, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be useful to speed up tests, WDYT? (I don't see why we silently limit it currently)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useful, we want to throttle that by 5 seconds

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t catch your comments earlier.
It seems that the default is already set to 5s

a.Flag("scrape.discovery-reload-interval", "Interval used by scrape manager to throttle target groups updates.").
Hidden().Default("5s").SetValue(&cfg.scrape.DiscoveryReloadInterval)
.
I’m not sure if there’s a specific reason for capping the value in this manner. If it could potentially cause issues, it might be best to explicitly mention this.
Users are generally allowed to customize other parameters, so it would make sense to allow them to adjust this one as well, provided they understand the implications.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this conversation resolved? If so, please remove the TODO.

Comment thread scrape/manager_test.go Outdated
Comment thread scrape/manager_test.go Outdated
Comment thread scrape/manager_test.go Outdated
Comment thread scrape/manager_test.go
@machine424

Copy link
Copy Markdown
Member Author

I'll need to deal with the races in tests.

@machine424
machine424 marked this pull request as draft November 24, 2023 16:31
@machine424 machine424 changed the title make discovery manager notify consumers of dropped targets for a still existing job make discovery manager notify consumers of dropped targets for still defined jobs Nov 25, 2023
@machine424
machine424 marked this pull request as ready for review November 25, 2023 15:17
@machine424

machine424 commented Nov 25, 2023

Copy link
Copy Markdown
Member Author

I'll need to deal with the races in tests.

Done, you can take a look now.

cc @roidelapluie

Still getting some weird yaml loading issues on windows, will take a look.
Tell me if you prefer putting the config files in testdata/, I'll also see if we can merge the tests.

@machine424
machine424 marked this pull request as draft January 4, 2024 19:42
@machine424

Copy link
Copy Markdown
Member Author

Done.
The remaining failing test TestOnlyProviderStaleTargetsAreDropped is due to #13312.

@machine424
machine424 marked this pull request as ready for review January 5, 2024 12:29
Comment thread discovery/manager.go
@beorn7

beorn7 commented Feb 27, 2024

Copy link
Copy Markdown
Contributor

@roidelapluie are you able to finish the review on this?

@machine424
machine424 force-pushed the drop-targets branch 3 times, most recently from f7fd9a0 to 916a441 Compare July 10, 2024 16:00
roidelapluie
roidelapluie previously approved these changes Aug 9, 2024
@roidelapluie

Copy link
Copy Markdown
Member

LGTM

@bboreham bboreham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks fine to me; one question about the TODO.

Comment thread scrape/manager.go Outdated
}

func (m *Manager) reloader() {
// TODO: OK?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this conversation resolved? If so, please remove the TODO.

@roidelapluie

Copy link
Copy Markdown
Member

cc @machine424 have you seen Bryan's comment?

@machine424 machine424 changed the title make discovery manager notify consumers of dropped targets for still defined jobs fix(discovery): make discovery manager notify consumers of dropped targets for still defined jobs Aug 20, 2024
@machine424

Copy link
Copy Markdown
Member Author

cc @machine424 have you seen Bryan's comment?

I haven't looked at this in a while and need a refresher. I'll revisit it this week.

…rgets for still defined jobs

scrape/manager_test.go: add a test to check that the manager gets notified
for targets that got dropped by discovery to reproduce: prometheus#12858 (comment)

Signed-off-by: machine424 <[email protected]>
@machine424

Copy link
Copy Markdown
Member Author

Ready now.

@machine424

Copy link
Copy Markdown
Member Author

Once this is merged, I'll revive #13622

@bboreham bboreham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@machine424
machine424 merged commit c586c15 into prometheus:main Aug 28, 2024
avilevy18 added a commit to avilevy18/prometheus that referenced this pull request Mar 20, 2026
This adds a SkipInitialWait option to the discovery Manager, allowing consumers sensitive to startup latency to receive the first batch of discovered targets immediately instead of waiting for the updatert ticker.

To support this without breaking the immediate dropped target notifications introduced in prometheus#13147, ApplyConfig now uses a keep flag to only trigger immediate downstream syncs for obsolete or updated providers. This prevents sending premature empty target groups for brand-new providers on initial startup.

Additionally, the scrape manager's reloader loop is updated to process the initial triggerReload immediately, ensuring the end-to-end pipeline processes initial targets without artificial delays.

Signed-off-by: avilevy <[email protected]>
bwplotka added a commit that referenced this pull request Apr 3, 2026
…tes; add DiscoveryReloadOnStartup option for short-lived environments (#18187)

* Adding scape on shutdown

Signed-off-by: avilevy <[email protected]>

* scrape: replace skipOffsetting to make the test offset deterministic instead of skipping it entirely

Signed-off-by: avilevy <[email protected]>

* renamed calculateScrapeOffset to getScrapeOffset

Signed-off-by: avilevy <[email protected]>

* discovery: Add skipStartupWait to bypass initial discovery delay

In short-lived environments like agent mode or serverless, the
Prometheus process may only execute for a few seconds. Waiting for
the default 5-second `updatert` ticker before sending the first
target groups means the process could terminate before collecting
any metrics at all.

This commit adds a `skipStartupWait` option to the Discovery Manager
to bypass this initial delay. When enabled, the sender uses an
unthrottled startup loop that instantly forwards all triggers. This
ensures both the initial empty update from `ApplyConfig` and the
first real targets from discoverers are passed downstream immediately.

After the first ticker interval elapses, the sender cleanly breaks out
of the startup phase, resets the ticker, and resumes standard
operations.

Signed-off-by: avilevy <[email protected]>

* scrape: Bypass initial reload delay for ScrapeOnShutdown

In short-lived environments like agent mode or serverless, the default
5-second `DiscoveryReloadInterval` can cause the process to terminate
before the scrape manager has a chance to process targets and collect
any metrics.

Because the discovery manager sends an initial empty update upon
configuration followed rapidly by the actual targets, simply waiting
for a single reload trigger is insufficient—the real targets would
still get trapped behind the ticker delay.

This commit introduces an unthrottled startup loop in the `reloader`
when `ScrapeOnShutdown` is enabled. It processes all incoming
`triggerReload` signals immediately during the first interval. Once
the initial tick fires, the `reloader` resets the ticker and falls
back into its standard throttled loop, ensuring short-lived processes
can discover and scrape targets instantly.

Signed-off-by: avilevy <[email protected]>

* test(scrape): refactor time-based manager tests to use synctest

Addresses PR feedback to remove flaky, time-based sleeping in the scrape manager tests.

Add TestManager_InitialScrapeOffset and TestManager_ScrapeOnShutdown to use the testing/synctest package, completely eliminating real-world time.Sleep delays and making the assertions 100% deterministic.

- Replaced httptest.Server with net.Pipe and a custom startFakeHTTPServer helper to ensure all network I/O remains durably blocked inside the synctest bubble.
- Leveraged the skipOffsetting option to eliminate random scrape jitter, making the time-travel math exact and predictable.
- Using skipOffsetting also safely bypasses the global singleflight DNS lookup in setOffsetSeed, which previously caused cross-bubble panics in synctest.
- Extracted shared boilerplate into a setupSynctestManager helper to keep the test cases highly readable and data-driven.

Signed-off-by: avilevy <[email protected]>

* Clarify use cases in InitialScrapeOffset comment

Signed-off-by: avilevy <[email protected]>

* test(scrape): use httptest for mock server to respect context cancellation

- Replaced manual HTTP string formatting over `net.Pipe` with `httptest.NewUnstartedServer`.
- Implemented an in-memory `pipeListener` to allow the server to handle `net.Pipe` connections directly. This preserves `synctest` time isolation without opening real OS ports.
- Added explicit `r.Context().Done()` handling in the mock HTTP handler to properly simulate aborted requests and scrape timeouts.
- Validates that the request context remains active and is not prematurely cancelled during `ScrapeOnShutdown` scenarios.
- Renamed `skipOffsetting` to `skipJitterOffsetting`.
- Addressed other PR comments.

Signed-off-by: avilevy <[email protected]>

* tmp

Signed-off-by: bwplotka <[email protected]>

* exp2

Signed-off-by: bwplotka <[email protected]>

* fix

Signed-off-by: bwplotka <[email protected]>

* scrape: fix scrapeOnShutdown context bug and refactor test helpers
The scrapeOnShutdown feature was failing during manager shutdown because
the scrape pool context was being cancelled before the final shutdown
scrapes could execute. Fix this by delaying context cancellation
in scrapePool.stop() until after all scrape loops have stopped.
In addition:
- Added test cases to verify scrapeOnShutdown works with InitialScrapeOffset.
- Refactored network test helper functions from manager_test.go to
  helpers_test.go.
- Addressed other comments.

Signed-off-by: avilevy <[email protected]>

* Update scrape/scrape.go

Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: avilevy18 <[email protected]>

* feat(discovery): add SkipInitialWait to bypass initial startup delay

This adds a SkipInitialWait option to the discovery Manager, allowing consumers sensitive to startup latency to receive the first batch of discovered targets immediately instead of waiting for the updatert ticker.

To support this without breaking the immediate dropped target notifications introduced in #13147, ApplyConfig now uses a keep flag to only trigger immediate downstream syncs for obsolete or updated providers. This prevents sending premature empty target groups for brand-new providers on initial startup.

Additionally, the scrape manager's reloader loop is updated to process the initial triggerReload immediately, ensuring the end-to-end pipeline processes initial targets without artificial delays.

Signed-off-by: avilevy <[email protected]>

* scrape: Add TestManagerReloader and refactor discovery triggerSync

Adds a new TestManagerReloader test suite using synctest to assert
behavior of target updates, discovery reload ticker intervals, and
ScrapeOnShutdown flags.

Updates setupSynctestManager to allow skipping initial config setup by
passing an interval of 0.

Also renames the 'keep' variable to 'triggerSync' in ApplyConfig inside
discovery/manager.go for clarity, and adds a descriptive comment.

Signed-off-by: avilevy <[email protected]>

* feat(discovery,scrape): rename startup wait options and add DiscoveryReloadOnStartup

- discovery: Rename `SkipInitialWait` to `SkipStartupWait` for clarity.
- discovery: Pass `context.Context` to `flushUpdates` to handle cancellation and avoid leaks.
- scrape: Add `DiscoveryReloadOnStartup` to `Options` to decouple startup discovery from `ScrapeOnShutdown`.
- tests: Refactor `TestTargetSetTargetGroupsPresentOnStartup` and `TestManagerReloader` to use table-driven tests and `synctest` for better stability and coverage.

Signed-off-by: avilevy <[email protected]>

* feat(discovery,scrape): importing changes proposed in 043d710

- Refactor sender to use exponential backoff
- Replaces `time.NewTicker` in `sender()` with an exponential backoff
  to prevent panics on non-positive intervals and better throttle updates.
- Removes obsolete `skipStartupWait` logic.
- Refactors `setupSynctestManager` to use an explicit `initConfig` argument

Signed-off-by: avilevy <[email protected]>

* fix: updating go mod

Signed-off-by: avilevy <[email protected]>

* fixing merge

Signed-off-by: avilevy <[email protected]>

* fixing issue: 2 variables but NewTestMetrics returns 1 value

Signed-off-by: avilevy <[email protected]>

* Update discovery/manager.go

Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: avilevy18 <[email protected]>

* Refactor setupSynctestManager initConfig into a separate function

Signed-off-by: avilevy <[email protected]>

---------

Signed-off-by: avilevy <[email protected]>
Signed-off-by: bwplotka <[email protected]>
Signed-off-by: avilevy18 <[email protected]>
Co-authored-by: bwplotka <[email protected]>
rbizos pushed a commit to rbizos/prometheus that referenced this pull request Apr 29, 2026
…tes; add DiscoveryReloadOnStartup option for short-lived environments (prometheus#18187)

* Adding scape on shutdown

Signed-off-by: avilevy <[email protected]>

* scrape: replace skipOffsetting to make the test offset deterministic instead of skipping it entirely

Signed-off-by: avilevy <[email protected]>

* renamed calculateScrapeOffset to getScrapeOffset

Signed-off-by: avilevy <[email protected]>

* discovery: Add skipStartupWait to bypass initial discovery delay

In short-lived environments like agent mode or serverless, the
Prometheus process may only execute for a few seconds. Waiting for
the default 5-second `updatert` ticker before sending the first
target groups means the process could terminate before collecting
any metrics at all.

This commit adds a `skipStartupWait` option to the Discovery Manager
to bypass this initial delay. When enabled, the sender uses an
unthrottled startup loop that instantly forwards all triggers. This
ensures both the initial empty update from `ApplyConfig` and the
first real targets from discoverers are passed downstream immediately.

After the first ticker interval elapses, the sender cleanly breaks out
of the startup phase, resets the ticker, and resumes standard
operations.

Signed-off-by: avilevy <[email protected]>

* scrape: Bypass initial reload delay for ScrapeOnShutdown

In short-lived environments like agent mode or serverless, the default
5-second `DiscoveryReloadInterval` can cause the process to terminate
before the scrape manager has a chance to process targets and collect
any metrics.

Because the discovery manager sends an initial empty update upon
configuration followed rapidly by the actual targets, simply waiting
for a single reload trigger is insufficient—the real targets would
still get trapped behind the ticker delay.

This commit introduces an unthrottled startup loop in the `reloader`
when `ScrapeOnShutdown` is enabled. It processes all incoming
`triggerReload` signals immediately during the first interval. Once
the initial tick fires, the `reloader` resets the ticker and falls
back into its standard throttled loop, ensuring short-lived processes
can discover and scrape targets instantly.

Signed-off-by: avilevy <[email protected]>

* test(scrape): refactor time-based manager tests to use synctest

Addresses PR feedback to remove flaky, time-based sleeping in the scrape manager tests.

Add TestManager_InitialScrapeOffset and TestManager_ScrapeOnShutdown to use the testing/synctest package, completely eliminating real-world time.Sleep delays and making the assertions 100% deterministic.

- Replaced httptest.Server with net.Pipe and a custom startFakeHTTPServer helper to ensure all network I/O remains durably blocked inside the synctest bubble.
- Leveraged the skipOffsetting option to eliminate random scrape jitter, making the time-travel math exact and predictable.
- Using skipOffsetting also safely bypasses the global singleflight DNS lookup in setOffsetSeed, which previously caused cross-bubble panics in synctest.
- Extracted shared boilerplate into a setupSynctestManager helper to keep the test cases highly readable and data-driven.

Signed-off-by: avilevy <[email protected]>

* Clarify use cases in InitialScrapeOffset comment

Signed-off-by: avilevy <[email protected]>

* test(scrape): use httptest for mock server to respect context cancellation

- Replaced manual HTTP string formatting over `net.Pipe` with `httptest.NewUnstartedServer`.
- Implemented an in-memory `pipeListener` to allow the server to handle `net.Pipe` connections directly. This preserves `synctest` time isolation without opening real OS ports.
- Added explicit `r.Context().Done()` handling in the mock HTTP handler to properly simulate aborted requests and scrape timeouts.
- Validates that the request context remains active and is not prematurely cancelled during `ScrapeOnShutdown` scenarios.
- Renamed `skipOffsetting` to `skipJitterOffsetting`.
- Addressed other PR comments.

Signed-off-by: avilevy <[email protected]>

* tmp

Signed-off-by: bwplotka <[email protected]>

* exp2

Signed-off-by: bwplotka <[email protected]>

* fix

Signed-off-by: bwplotka <[email protected]>

* scrape: fix scrapeOnShutdown context bug and refactor test helpers
The scrapeOnShutdown feature was failing during manager shutdown because
the scrape pool context was being cancelled before the final shutdown
scrapes could execute. Fix this by delaying context cancellation
in scrapePool.stop() until after all scrape loops have stopped.
In addition:
- Added test cases to verify scrapeOnShutdown works with InitialScrapeOffset.
- Refactored network test helper functions from manager_test.go to
  helpers_test.go.
- Addressed other comments.

Signed-off-by: avilevy <[email protected]>

* Update scrape/scrape.go

Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: avilevy18 <[email protected]>

* feat(discovery): add SkipInitialWait to bypass initial startup delay

This adds a SkipInitialWait option to the discovery Manager, allowing consumers sensitive to startup latency to receive the first batch of discovered targets immediately instead of waiting for the updatert ticker.

To support this without breaking the immediate dropped target notifications introduced in prometheus#13147, ApplyConfig now uses a keep flag to only trigger immediate downstream syncs for obsolete or updated providers. This prevents sending premature empty target groups for brand-new providers on initial startup.

Additionally, the scrape manager's reloader loop is updated to process the initial triggerReload immediately, ensuring the end-to-end pipeline processes initial targets without artificial delays.

Signed-off-by: avilevy <[email protected]>

* scrape: Add TestManagerReloader and refactor discovery triggerSync

Adds a new TestManagerReloader test suite using synctest to assert
behavior of target updates, discovery reload ticker intervals, and
ScrapeOnShutdown flags.

Updates setupSynctestManager to allow skipping initial config setup by
passing an interval of 0.

Also renames the 'keep' variable to 'triggerSync' in ApplyConfig inside
discovery/manager.go for clarity, and adds a descriptive comment.

Signed-off-by: avilevy <[email protected]>

* feat(discovery,scrape): rename startup wait options and add DiscoveryReloadOnStartup

- discovery: Rename `SkipInitialWait` to `SkipStartupWait` for clarity.
- discovery: Pass `context.Context` to `flushUpdates` to handle cancellation and avoid leaks.
- scrape: Add `DiscoveryReloadOnStartup` to `Options` to decouple startup discovery from `ScrapeOnShutdown`.
- tests: Refactor `TestTargetSetTargetGroupsPresentOnStartup` and `TestManagerReloader` to use table-driven tests and `synctest` for better stability and coverage.

Signed-off-by: avilevy <[email protected]>

* feat(discovery,scrape): importing changes proposed in 043d710

- Refactor sender to use exponential backoff
- Replaces `time.NewTicker` in `sender()` with an exponential backoff
  to prevent panics on non-positive intervals and better throttle updates.
- Removes obsolete `skipStartupWait` logic.
- Refactors `setupSynctestManager` to use an explicit `initConfig` argument

Signed-off-by: avilevy <[email protected]>

* fix: updating go mod

Signed-off-by: avilevy <[email protected]>

* fixing merge

Signed-off-by: avilevy <[email protected]>

* fixing issue: 2 variables but NewTestMetrics returns 1 value

Signed-off-by: avilevy <[email protected]>

* Update discovery/manager.go

Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: avilevy18 <[email protected]>

* Refactor setupSynctestManager initConfig into a separate function

Signed-off-by: avilevy <[email protected]>

---------

Signed-off-by: avilevy <[email protected]>
Signed-off-by: bwplotka <[email protected]>
Signed-off-by: avilevy18 <[email protected]>
Co-authored-by: bwplotka <[email protected]>
Signed-off-by: Raphael Bizos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kubernetes_sd does not remove existing targets when an existing config is modified to produce an empty result

4 participants