Skip to content

refactor metrics event processing #456

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

Merged
merged 8 commits into from
Aug 14, 2025
Merged

refactor metrics event processing #456

merged 8 commits into from
Aug 14, 2025

Conversation

harp-intel
Copy link
Contributor

@harp-intel harp-intel commented Aug 13, 2025

eliminate race condition in perf event processing

This pull request refactors how perf event output is collected and processed in the metrics package, shifting from line-by-line string handling to batching by interval using byte slices. The changes improve the accuracy and efficiency of event grouping, simplify channel usage, and add robust parsing and testing for interval extraction. Most changes are in cmd/metrics/metrics.go, with supporting updates to target and script streaming interfaces.

Perf event batching and processing improvements:

  • Refactored perf event output handling to batch events by interval using byte slices ([][]byte), replacing previous timer-based grouping and string channels. This ensures each batch corresponds to a specific perf interval, improving accuracy and reducing race conditions. (cmd/metrics/metrics.go, internal/target/target.go, internal/target/local_target.go, internal/target/remote_target.go, internal/target/helpers.go, internal/script/script.go) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]

  • Added the extractInterval function to parse the interval value from perf event JSON lines, with robust error handling for missing or malformed data. (cmd/metrics/event_frame.go)

  • Updated the perf output processing pipeline (processPerfOutput) to consume batches from a channel, process each batch per interval, and drain remaining batches after context cancellation, ensuring no events are lost. (cmd/metrics/metrics.go) [1] [2] [3] [4]

Testing and error handling enhancements:

  • Added comprehensive unit tests for the new extractInterval function, covering valid, missing, and malformed interval cases. (cmd/metrics/event_frame_test.go)

  • Improved error messages and parsing robustness for event JSON, and removed unnecessary error checks for CPU range parsing. (cmd/metrics/event_frame.go) [1] [2]

Supporting interface and signature changes:

  • Updated all relevant interfaces and method signatures to use chan []byte instead of chan string for stdout and stderr streaming, ensuring consistency across local and remote target command execution and script streaming. (internal/target/target.go, internal/target/local_target.go, internal/target/remote_target.go, internal/target/helpers.go, internal/script/script.go) [1] [2] [3] [4] [5] [6]

@harp-intel harp-intel requested a review from Copilot August 13, 2025 13:35
Copilot

This comment was marked as outdated.

Signed-off-by: Harper, Jason M <[email protected]>
@harp-intel harp-intel requested a review from Copilot August 13, 2025 17:50
Copilot

This comment was marked as outdated.

@harp-intel harp-intel requested a review from Copilot August 13, 2025 21:50
Copilot

This comment was marked as outdated.

harp-intel and others added 3 commits August 13, 2025 15:02
Signed-off-by: Harper, Jason M <[email protected]>
Signed-off-by: Harper, Jason M <[email protected]>
@harp-intel harp-intel requested a review from Copilot August 13, 2025 23:53
Copilot

This comment was marked as outdated.

@harp-intel harp-intel requested review from adgubrud and Copilot August 14, 2025 00:34
Copy link
Contributor

@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.

Pull Request Overview

This pull request refactors the metrics perf event processing system to eliminate race conditions and improve thread safety. The main change replaces a timer-based approach for batching perf events with an interval-based batching system that groups events by their interval values using channel-based communication.

  • Introduced interval-based event batching to replace timer-based frame detection
  • Changed channel types from string to []byte for better performance and memory management
  • Added comprehensive test coverage for the new interval extraction functionality

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/target/target.go Updated interface signature to use []byte channels instead of string
internal/target/remote_target.go Updated method signature to match new interface
internal/target/local_target.go Updated method signature to match new interface
internal/target/helpers.go Modified to send []byte data directly and create buffer copies to avoid race conditions
internal/script/script.go Updated function signature to use []byte channels
cmd/metrics/metrics.go Major refactor implementing interval-based batching with new channel communication and processing logic
cmd/metrics/event_frame_test.go Added comprehensive tests for the new extractInterval function
cmd/metrics/event_frame.go Added extractInterval function and cleaned up error handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@adgubrud adgubrud left a comment

Choose a reason for hiding this comment

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

Looks good to me

@harp-intel harp-intel merged commit fd5cd32 into main Aug 14, 2025
5 checks passed
@harp-intel harp-intel deleted the eventprocessing branch August 14, 2025 21:39
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