feat(pyroscope/scrape): add support for configuring CPU profile's duration scraped by pyroscope.scrape
#591
Conversation
…ation scraped by `pyroscope.scrape` Signed-off-by: hainenber <[email protected]>
Signed-off-by: hainenber <[email protected]>
|
Docs here include suggestions from the Agent PR, so all good from a doc standpoint. Over to the @grafana/grafana-alloy-maintainers team for code review |
clayton-cornell
added
the
type/docs
Co-authored-by: Paschalis Tsilias <[email protected]>
| * 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. |
There was a problem hiding this comment.
cc @simonswine @korniltsev does it make sense for the profiling_duration to ever be larger than the scrape_interval here? Or similar to the Prometheus scrape_timeout it should be always less than the scrape interval?
There was a problem hiding this comment.
It should always be less. If you try to profile a service which is already running a profile, you'll get an error
server returned HTTP status (500) Could not enable CPU profiling: cpu profiling already in use
I think we should either add some validation, or default back to min(scrape_interval - 1, profiling_duration)
There was a problem hiding this comment.
I think that makes sense as well, yeah cc @hainenber
There was a problem hiding this comment.
I've added the validation in the HEAD. Looks A-OK to you guys?
Co-authored-by: Paschalis Tsilias <[email protected]>
…filing_duration` Signed-off-by: hainenber <[email protected]>
Signed-off-by: hainenber <[email protected]>
…ientConfig error Signed-off-by: hainenber <[email protected]>
rfratto
added
the
backport-to-agent:no
| @@ -65,6 +65,7 @@ Name | Type | Description | |||
| `params` | `map(list(string))` | A set of query parameters with which the target is scraped. | | no | |||
| `scrape_interval` | `duration` | How frequently to scrape the targets of this scrape configuration. | `"15s"` | no | |||
| `scrape_timeout` | `duration` | The timeout for scraping targets of this configuration. Must be larger than `scrape_interval`. | `"18s"` | no | |||
| `profiling_duration`| `duration` | The duration for a delta profiling to be scraped. Must be larger than 1 second. | `"14s"` | no | |||
There was a problem hiding this comment.
Sorry, one last thing. Could we name this with a little more descriptive name?
| `profiling_duration`| `duration` | The duration for a delta profiling to be scraped. Must be larger than 1 second. | `"14s"` | no | |
| `delta_profiling_duration`| `duration` | The duration for a delta profiling to be scraped. Must be larger than 1 second. | `"14s"` | no |
There was a problem hiding this comment.
I've changed this in HEAD. PTAL
| if arg.ProfilingDuration.Seconds() >= arg.ScrapeInterval.Seconds() { | ||
| return fmt.Errorf("profiling_duration must be smaller than scrape_interval when using delta profiling") | ||
| } |
There was a problem hiding this comment.
Should this condition make sure that DeltaProfilingDuration is not only smaller, but also at least a second smaller?
|
Thanks for the contribution, this is a very useful knob to be able to turn 🙂 I am happy with this to go forward, once all the points @tpaschalis has raised are addressed. |
|
Oops, |
…ation scraped by `pyroscope.scrape` Signed-off-by: hainenber <[email protected]>
…sible `delta_profiling_duration` + stricter validation Signed-off-by: hainenber <[email protected]>
hainenber
force-pushed
the
support-for-user-configuration-of-pyroscope-scrape-seconds-query-params
branch
from
5be0053
to
f3d89f6
Compare
…scrape-seconds-query-params
|
@tpaschalis @simonswine Anything we can do to help move this forward ? I think @hainenber fixed all the previous comment |
…scrape-seconds-query-params
hainenber
deleted the
support-for-user-configuration-of-pyroscope-scrape-seconds-query-params
branch
|
Has this change been released? I don't see it in the docs or the changelogs. |
It is in the |
PR Description
Which issue(s) this PR fixes
Fixes #195
Notes to the Reviewer
PR Checklist