pyroscpoe.scrape: collect delta map sizes metric #2862
+271
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
PR Overview
This PR adds a new Prometheus metric to track the size of the delta map used in the delta profiling workflow. The key changes include:
- Introducing a new delta metric collector ("pyroscope_delta_map_size") registered with Prometheus.
- Updating the Manager to accept a Registerer and utilize the new collector.
- Enhancing tests to validate that the new delta map size metric is correctly reported.
Reviewed Changes
| File | Description |
|---|---|
| internal/component/pyroscope/scrape/delta_metric_collector.go | Added a collector for the delta map size metric. |
| internal/component/pyroscope/scrape/delta_profiles_manager_test.go | Updated tests to verify the new metric collection. |
| internal/component/pyroscope/scrape/internal/fastdelta/fd.go | Added a DeltaMapSize function with documentation. |
| internal/component/pyroscope/scrape/scrape_loop.go | Introduced activeScrapeLoops to iterate active scraping loops. |
| internal/component/pyroscope/scrape/manager.go | Refactored NewManager to accept a Registerer and register the new collector. |
| internal/component/pyroscope/scrape/manager_test.go | Updated test to support the new NewManager signature. |
| internal/component/pyroscope/scrape/delta_profiles.go | Updated the delta appender to store the current delta map size. |
| internal/component/pyroscope/scrape/scrape.go | Updated the NewManager invocation with the Registerer parameter. |
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
internal/component/pyroscope/scrape/delta_profiles.go:128
- [nitpick] Review the placement of updating lastDeltaMapSize before the initialization check. Ensure that storing the delta map size on the first sample (which is subsequently skipped) aligns with the intended metric reporting.
d.lastDeltaMapSize.Store(int64(d.delta.DeltaMapSize()))
korniltsev
force-pushed
the
pyroscope-scrape-collect-delta-map-sizes
branch
from
44365b0 to
b92f8cb
Compare
korniltsev
commented
Mar 3, 2025
| return &deltaMetricCollector{ | ||
| manager: manager, | ||
| desc: prometheus.NewDesc("pyroscope_delta_map_size", "Size of the DeltaMap", []string{"aggregation"}, nil), | ||
| label: "namespace", |
There was a problem hiding this comment.
TODO: discuss how to not hardcode this
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
Introduce new
pyroscope_delta_map_sizemetric reflecting the size of theDeltaMapsexample:
Which issue(s) this PR fixes
The metric will help investigating who's consuming memory #2840
Notes to the Reviewer
PR Checklist