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

feat(pyroscope/scrape): add support for configuring CPU profile's duration scraped by pyroscope.scrape. #6827

Conversation

hainenber
Copy link
Contributor

PR Description

Which issue(s) this PR fixes

Fixes grafana/alloy#195

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

…ation scraped by `pyroscope.scrape`.

Signed-off-by: hainenber <dotronghai96@gmail.com>
@hainenber hainenber requested review from a team and clayton-cornell as code owners April 5, 2024 16:48
Comment on lines +391 to +392
automatically added to requests. The `seconds` used will be, by default to `scrape_interval - 1`
, or `profiling_duration` if specified.
Copy link
Contributor

@clayton-cornell clayton-cornell Apr 5, 2024

Choose a reason for hiding this comment

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

Suggested change
automatically added to requests. The `seconds` used will be, by default to `scrape_interval - 1`
, or `profiling_duration` if specified.
automatically added to requests.
The default value for the `seconds` query parameter is `scrape_interval - 1`.
If you set `profiling_duration`, then `seconds` is assigned the same value as `profiling_duration`.
For example, if you set `scrape_interval` to `"15s"`, then `seconds` defaults to `14s`.
If you set `profiling_duration` to `16s`, then `seconds is set to `16s` regardless of the `scrape_interval` value.

This probably isn't the final wording. I'm trying to understand the logic first... is this close?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the correct logic. I think your suggestion is sufficient but alas, there's a missing backtick here :D

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I've updated the suggestion a little. I fixed the missing backtick, and adjusted the wording slightly.. and included semantic linebreaks.

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@rfratto rfratto added variant/flow Relatd to Grafana Agent Flow. enhancement New feature or request labels Apr 9, 2024
@julienvey
Copy link

Hi 👋 Thank you for the work on this. Is this PR expected to be merged in grafana/agent or grafana/alloy ?

@hainenber
Copy link
Contributor Author

I'll need to switch the destination branch to be of grafana/alloy main branch and some corresponding refactors. I don't think I should be introduce new features into now deprecating grafana/agent. Pardon me if this feels icky on your side!

@hainenber hainenber closed this Apr 16, 2024
@hainenber hainenber deleted the support-for-user-configuration-of-pyroscope-scrape-seconds-query-params branch April 18, 2024 10:32
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label May 19, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. variant/flow Relatd to Grafana Agent Flow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a configuration option to pyroscope.scrape to define for how long a CPU profile should run
4 participants