Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds procfs metric collection, in-memory storage, and aggregation/diff utilities so the master can periodically gather per-process procfs stats from segment agents and retain/compare snapshots over time.
Changes:
- Extend procfs proto fields (PID/counters) to
int64and update procfs conversion/tests accordingly. - Introduce
ProcfsStorage(time-series snapshot storage) plus procfs diff/grouping helpers and tests. - Add master-side procfs gatherer (
ProcfsGatherStorage) and wire it into the background refresh loop + config/app initialization.
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/utils/procfs.go |
Updates procfs→protobuf conversions to match new int64 proto fields. |
internal/utils/procfs_test.go |
Adjusts unit tests for ProcStat/ProcStatus field type changes. |
internal/utils/procfs_getprocinfo_test.go |
Adjusts GetProcInfo tests for int64 PID fields. |
internal/storage/procfs_storage.go |
Adds time-indexed procfs snapshot storage + nearest-interval lookup helpers. |
internal/storage/procfs_storage_test.go |
Adds extensive tests for procfs storage behaviors (nearest lookup, retention, etc.). |
internal/storage/procfs_group.go |
Adds procfs diffing and aggregation utilities (ProcStat/ProcIO grouping). |
internal/storage/procfs_group_test.go |
Adds tests for procfs diff logic and counter clamping. |
internal/storage/procfs_group_metrics_test.go |
Adds tests for procfs aggregation (including intermediate-results behavior). |
internal/storage/group.go |
Generalizes UpdateIntermediateKey to support additional numeric types. |
internal/storage/group_test.go |
Adds/updates tests around grouping logic (now also used by procfs aggregation). |
internal/master/procfs_gather.go |
Adds master-side procfs gatherer that issues gRPC requests and accumulates results. |
internal/master/procfs_gather_test.go |
Adds tests for procfs gatherer behavior (batching, errors, cancellation). |
internal/master/background.go |
Wires procfs refresh loop into background processing; adds procfs storage dependency. |
internal/grpc/testutils_test.go |
Updates background storage construction to include procfs storage. |
internal/grpc/set_get_query_info_parallel_test.go |
Updates background storage construction signature (procfs arg added). |
internal/grpc/actions_test.go |
Updates background storage construction signature (procfs arg added). |
internal/config/config.go |
Adds ProcfsRefreshInterval config with default value. |
internal/app/app.go |
Instantiates procfs storage and passes it into background storage. |
api/proto/common/yagpcc_metrics.proto |
Changes procfs PID/counter field types from int32 to int64. |
api/proto/common/yagpcc_metrics.pb.go |
Regenerates protobuf Go bindings for the procfs type changes. |
internal/storage/trace.log |
Listed in PR files; not enough context shown here to assess content/intent. |
Comments suppressed due to low confidence (2)
internal/master/procfs_gather.go:136
GatherProcfsStatvalidatesnPullersbut never uses it to limit concurrency; it spawns one goroutine per host inhostJobs. With many segment hosts this can create unbounded concurrent gRPC calls. Consider usingerrgroup.Group.SetLimit(nPullers)(or a semaphore) to enforce the intended parallelism limit.
internal/master/procfs_gather.go:35ProcfsGatherStorage.gatherTimeis stored but never used in this file. If it’s not needed, remove it; otherwise consider using it (e.g., as part of logging/metrics) to justify keeping it in the struct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+134
to
+135
| return p.procfsStat[minIndex].pidProcData, nil | ||
| } |
Comment on lines
+41
to
+59
| type ProcfsOption = func(*ProcfsStorage) | ||
|
|
||
| const ( | ||
| defaultStoredPoints = 30 | ||
| ) | ||
|
|
||
| func NewProcfsStorage() *ProcfsStorage { | ||
| return &ProcfsStorage{ | ||
| mx: &sync.RWMutex{}, | ||
| maximumStoredPoints: defaultStoredPoints, | ||
| procfsStat: make([]ProcfsStatType, 0, defaultStoredPoints), | ||
| } | ||
| } | ||
|
|
||
| func WithMaximumStoredPoints(maximumStoredPoints int) ProcfsOption { | ||
| return func(p *ProcfsStorage) { | ||
| p.maximumStoredPoints = maximumStoredPoints | ||
| } | ||
| } |
| ) | ||
| errG.Go(func() error { | ||
| err := backgroundStorage.RefreshProcfs(ctxI, cfg.ProcfsRefreshInterval, int(cfg.SegmentPullThreads), cfg.ListenPort, int(cfg.MaxMessageSize)) | ||
| l.Errorf("got %v refresh session and queries", err) |
Comment on lines
+137
to
+143
| func (p *ProcfsStorage) getNMin(d time.Duration) (ProcMap, ProcMap, error) { | ||
| nearest, err := p.GetNearestNTime(d) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("fail in get 5 minutes interval %v", err) | ||
| } | ||
| return nearest, p.procfsStat[len(p.procfsStat)-1].pidProcData, nil | ||
| } |
| func (p *ProcfsStorage) getNMin(d time.Duration) (ProcMap, ProcMap, error) { | ||
| nearest, err := p.GetNearestNTime(d) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("fail in get 5 minutes interval %v", err) |
This file contains hidden or 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
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.
Add procfs storage and aggregation methods