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

[receiver/prometheus] speed up tests by setting skipOffsetting #32341

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Apr 12, 2024

Fixes #32298

This PR modifies prometheusreceiver to allow to set the skipOffsetting option on its scrape option config. This option is private, so it can only be set by reflection.

This removes the random offset to start added to scraping prometheus metrics, so we may get faster CI builds.

@github-actions github-actions bot added the receiver/prometheus Prometheus receiver label Apr 12, 2024
@atoulme atoulme added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Apr 12, 2024
@atoulme atoulme marked this pull request as ready for review April 12, 2024 06:42
@atoulme atoulme requested a review from a team as a code owner April 12, 2024 06:42
@dashpole
Copy link
Contributor

how much of a speed increase did it result in for you locally?

@atoulme
Copy link
Contributor Author

atoulme commented Apr 12, 2024

I had 2 successive runs without errors (one failed on a different issue).

@atoulme
Copy link
Contributor Author

atoulme commented Apr 12, 2024

3 runs without failures.

@atoulme
Copy link
Contributor Author

atoulme commented Apr 12, 2024

how much of a speed increase did it result in for you locally?

I think I see overall a 20s reduction over the ~5minutes tests take. Given that we remove a random offset, it makes tests timing better. That said, I didn't manage to find why we wait 5s between starting the receiver and the first scrape.

@crobert-1
Copy link
Member

That said, I didn't manage to find why we wait 5s between starting the receiver and the first scrape.

I didn't get a chance to confirm in this receiver, but go's ticker will first tick after your defined duration, by default. Users have to implement their own functionality to have a tick at start time. I assume that's what's going on here.

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Any timing improvement here is helpful, as commented in the issue it looks like other-architecture builds are barely completing tests before the timeout 👍

Also, can you please add a reference to #32298 here? If the tests are consistently passing after this we'll be able to mark it resolved.

codeboten and others added 2 commits April 12, 2024 13:39
Co-authored-by: David Ashpole <dashpole@google.com>
@atoulme
Copy link
Contributor Author

atoulme commented Apr 12, 2024

Any timing improvement here is helpful, as commented in the issue it looks like other-architecture builds are barely completing tests before the timeout 👍

Yes, this is not a great improvement, but we can squeak by.

Also, can you please add a reference to #32298 here? If the tests are consistently passing after this we'll be able to mark it resolved.

Done

@atoulme
Copy link
Contributor Author

atoulme commented Apr 13, 2024

4th run, no errors.

@atoulme
Copy link
Contributor Author

atoulme commented Apr 13, 2024

5th run, no errors.

@crobert-1
Copy link
Member

Since the goal here in my mind is to reduce timeout errors, I'd prefer this go into main sooner rather than later, even if we could theoretically continue to see errors intermittently. I'm going to add ready to merge unless someone has a different opinion. 👍

@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Apr 15, 2024
@codeboten codeboten merged commit 8012101 into open-telemetry:main Apr 17, 2024
170 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 17, 2024
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this pull request May 8, 2024
…telemetry#32341)

Fixes open-telemetry#32298

This PR modifies prometheusreceiver to allow to set the skipOffsetting
option on its scrape option config. This option is private, so it can
only be set by reflection.

This removes the random offset to start added to scraping prometheus
metrics, so we may get faster CI builds.

---------

Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Co-authored-by: David Ashpole <dashpole@google.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/prometheus Prometheus receiver Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[receiver/prometheusreceiver] Tests hitting timeouts intermittently on actuated-arm64 runs
5 participants