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

pkg/config: support seconds in pprof configuration #4623

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

alperkokmen
Copy link
Contributor

this updates the definition of PprofProfilingConfig such that a new integer field Seconds is added which can be set for profiles, in an attempt to resolve #2818.

additionally, it changes the behavior where setting any profiling configurations will end up overriding the default so merge will now become a replace.

seconds is accepted as a query parameter and with this change, when provided, it will be used as scraping duration rather than the scraping interval value. this allows having scraping where process being profiled isn't under constant profiling. this only applies to configurations that have delta enabled.

https://pkg.go.dev/net/http/pprof

@alperkokmen alperkokmen requested a review from a team as a code owner May 14, 2024 06:04
@alperkokmen
Copy link
Contributor Author

@brancz i have taken a stab at implementing the proposed solution in #2818. please take a look.

@alperkokmen
Copy link
Contributor Author

@brancz, i just wanted to bump this up to get some eyes on it; could you please take a look for add folks who would be able to review? thank you.

@brancz
Copy link
Member

brancz commented May 29, 2024

Apologies for missing this, this looks great!

@pre-commit-ci-lite pre-commit-ci-lite bot requested a review from a team as a code owner May 29, 2024 09:39
@brancz
Copy link
Member

brancz commented May 29, 2024

I'm really sorry, this needs a rebase, but otherwise lgtm! And sorry again for this slipping through the cracks, so thank you for pinging me again. In the future also feel free to ping me on the Parca Discord server if you want to make sure to get a faster review! :)

this updates the definition of `PprofProfilingConfig` such that a new
integer field `Seconds` is added which can be set for profiles.

additionally, it changes the behavior where setting any profiling
configurations will end up overriding the default so merge will now
become a replace.

seconds is accepted as a query parameter and with this change, when
provided, it will be used as scraping duration rather than the scraping
interval value. this allows having scraping where process being profiled
isn't under constant profiling. this only applies to configurations that
have delta enabled.

https://pkg.go.dev/net/http/pprof
@alperkokmen
Copy link
Contributor Author

thanks for taking a look, @brancz. i rebased and addressed the feedback. PTAL when you get a chance.

pkg/scrape/target_test.go Outdated Show resolved Hide resolved
@alperkokmen
Copy link
Contributor Author

@brancz i think i need one last approval before checks are executed?

@brancz brancz merged commit 7d32a91 into parca-dev:main Jun 4, 2024
34 checks passed
@brancz
Copy link
Member

brancz commented Jun 4, 2024

Thank you for the awesome contribution!

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.

pull pprof: allow infrequent profiling
2 participants