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

pyroscope.scrape: single scrape pool #2928

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

korniltsev
Copy link
Contributor

@korniltsev korniltsev commented Mar 7, 2025

PR Description

This pr changes the pyroscope.scrape manager implementation to only handle one scrapeLook instead of a map of scrapeLoops of size = 1. This simplifies code and removes a goroutine for target sync.

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@korniltsev korniltsev requested a review from Copilot March 11, 2025 07:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR refactors the Pyroscope scraping logic to consolidate multiple scrape pools into a single scrape pool. The changes update the Manager to use a unified scrape pool, simplify target group handling in tests and components, and adjust the discovery package accordingly.

  • Refactors Manager in manager.go to remove multi-pool support and use a single scrape pool.
  • Updates manager_test.go to reflect the new API by switching from map-based target sets to slices.
  • Adjusts target group generation and component wiring in target.go and scrape.go for a single-job implementation.

Reviewed Changes

File Description
internal/component/pyroscope/scrape/manager_test.go Updates test logic to match the new single scrape pool API and channel type.
internal/component/discovery/target.go Adds a new helper for generating target groups for a single job and adjusts the return types.
internal/component/pyroscope/scrape/manager.go Consolidates multi-pool management into a single scrapePool and simplifies related methods.
internal/component/pyroscope/scrape/scrape.go Updates component initialization and target group generation to use the new single scrape pool design.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

internal/component/pyroscope/scrape/manager_test.go:22

  • Consider checking the error returned by NewManager instead of ignoring it, to catch any initialization issues during testing.
m, _ := NewManager(Options{}, NewDefaultArguments(), pyroscope.AppendableFunc(func(ctx context.Context, labels labels.Labels, samples []*pyroscope.RawSample) error {

@korniltsev korniltsev marked this pull request as ready for review March 11, 2025 07:42
@korniltsev korniltsev requested review from a team as code owners March 11, 2025 07:42
Copy link
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this optimization 👍

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.

2 participants