scrape.Manager: Remove initial scrape delay #14073
Conversation
Signed-off-by: Lovro Mažgon <lovro.mazgon@gmail.com>
Signed-off-by: Lovro Mažgon <lovro.mazgon@gmail.com>
lovromazgon
force-pushed
the
remove-scrape-delay
branch
from
6618b51
to
582753d
Compare
There was a problem hiding this comment.
Thanks for this! I agree we can remove the initial delays. I was trying to achieve similar in #12687 but it got deprioritized.
However, it feels something is a bit off with this change, especially around tests (even before your change?).
The main issue is that with disabled reloader logic, do we have have update tests that actually tests updates with reloading and it's throttling mechanism? Essentially it feels TestManagerTargetsUpdates now tests some non production flows which essentially reduces our test coverage. I was also confused by reloader disable option (not mention in your PR desc).
Otherwise, I think I like the reloader change, but I would love to have test for it (:
NOTE: There is a consequence of Prometheus creating scrape loops for likely not-up-to-date discovery information on start, but I think that's fine.
The tests and optional removal of reloading feels wrong.. do you have time to step back a bit and check what Update related tests around scrape manager Run would make sense WHILE trying to not turn off important parts. Feel free to change the current tests, those are not perfect 🤗
|
Thanks for the quick response @bwplotka! I'll see if I can get some time to have a closer look at the tests and properly test throttling. Although, at the first look, I'm not sure if we can test it without disabling the reloader, at least with the way the signalling happens right now. The problem is that the reloader goroutine is reading from the same channel that |
| @@ -147,16 +150,16 @@ func (m *Manager) reloader() { | |||
| defer ticker.Stop() | |||
|
|
|||
| for { | |||
| select { | |||
There was a problem hiding this comment.
I am not sure about this change.
There was a problem hiding this comment.
Can you elaborate what exactly is your concern?
This simple change ensures that the initial scrape can be started without the 5 second delay. A nice side-effect is that tests in that package now complete 15 seconds faster.
Background: I'm working on a small CLI tool that embeds Prometheus and allows you to scrape and visualize metrics ad-hoc. The UX suffers because of the initial 5 second delay.