Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scrape: Added option to scrape without initial jitter and on shutdown. #12687

Closed
wants to merge 1 commit into from

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Aug 14, 2023

To experiment with Prometheus pull methond on semi-short lived serverless environments we (Google Cloud) want to attempt init and shutdown scrapes to have some basic data from short lived workloads. This has little sense for Prometheus scrape manager, but it might be useful for external importers like OpenTelemetry and distributions, which can be deployed in various configurations (e.g. sidecar with just single target).

It's up to us to merge it or close. I would be fine maintaining on upstream, but we might as well keep it in our fork to experiment on this--and perhaps merge once official contrib Otel decides to use those options.

NOTE: Also added high level scrape manager test. I think it was kind of bad we never had test integrating all scrape manager pieces. Can add that in separate PR as well.

Alternatives attempted:

  • Manager Option for scrape on shutdown. This was not trivial due to 3 different context we pass. We would need to disconnect them from parent context (sometimes) for scrapeAndReport. Intrusive and prone to mistaken cancelations.
  • ForceScrape method. This is not trivial as the scrape would need to be additionally locked. Plus semantics on what to do after (continue interval?) is not clear. We can scope this problem down to stopAndScrape semantics.

@bwplotka bwplotka force-pushed the feature/shutdownscrape branch 2 times, most recently from 1f448b5 to 38a6fee Compare August 14, 2023 19:19
@bwplotka
Copy link
Member Author

bwplotka commented Aug 14, 2023

EDIT: I am stupid, it comments on files in diff view. Thanks @MichaHoffmann for pointing!

Lint fails, but no idea on which files

image

Anyone knows how to check it - it's super unhelpful (run golang ci lint locally with exactly same settings?)

https://github.com/prometheus/prometheus/actions/runs/5859745162/job/15886373920?pr=12687

@bwplotka bwplotka force-pushed the feature/shutdownscrape branch 2 times, most recently from 0614c2f to 6541886 Compare August 14, 2023 19:42
To experiment with Prometheus pull methond on semi-short lived serverless environments
we (Google Cloud) want to attempt init and shutdown scrapes to have some basic data from
short lived workloads. This has little sense for Prometheus scrape manager,
but it might be useful for external importers like OpenTelemetry and distributions,
which can be deployed in various configurations (e.g. sidecar with just single target).

It's up to us to merge it or close. I would be fine maintaining on upstream, but we
might as well keep it in our fork to experiment on this--and perhaps merge once official
contrib Otel decides to use those options.

NOTE: Also added high level scrape manager test. I think it was kind of bad we never had test integrating
all scrape manager pieces. Can add that in separate PR as well.

Alternatives attempted:

* Manager Option for scrape on shutdown. This was not trivial due to 3 different context we pass. We would need to disconnect them from parent context (sometimes) for scrapeAndReport. Intrusive and prone to mistaken cancelations.
* ForceScrape method. This is not trivia as the scrape would need to be additionally locked. Plus semantics on what to do after (continue interval?) is not clear. We can scope this problem down to stopAndScrape semantics.

Signed-off-by: bwplotka <bwplotka@gmail.com>
@bboreham
Copy link
Member

There seem to be a number of things going on here - refactoring, adding tests, adding new behaviour.
For a 300+ line PR I find this difficult to follow; could you split them into separate commits?
Maybe also two PRs, since some of the refactoring seems uncontraversial.

@bwplotka
Copy link
Member Author

bwplotka commented Aug 18, 2023

That's great idea. I'm on longer paternity leave, so unlikely to have time to split it before mid-September. Anyone is welcome to take it over/copy etc. Otherwise will do in September.

Also cc @ArthurSens who wanted similar test for his contribution. 🤗

@bwplotka
Copy link
Member Author

We are evolving this a bit internally, so let's close this for now (fixing tests and some race conditions in some edge cases).

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.

None yet

2 participants