-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[Enhancement] Implement metrics reporting for MemTrackerManager
#68170
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
base: main
Are you sure you want to change the base?
[Enhancement] Implement metrics reporting for MemTrackerManager
#68170
Conversation
Signed-off-by: arin-mirza <[email protected]>
Signed-off-by: arin-mirza <[email protected]>
|
@cursor review |
Signed-off-by: arin-mirza <[email protected]>
|
@cursor review |
Signed-off-by: arin-mirza <[email protected]>
|
@alvin-celerdata @kevincai I closed the previous PR where you were reviewers, can I get a review for this one, please? :) |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7a13d9f6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: arin-mirza <[email protected]>
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 57 / 68 (83.82%) file detail
|
|
@alvin-celerdata Can this PR be merged? Is there anything else that needs to be done? |
There was a problem hiding this 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 PR implements metrics reporting for MemTrackerManager to expose memory pool usage statistics. Previously, there were no backend metrics for memory pools. The implementation adds four new metrics: mem_pool_mem_limit_bytes, mem_pool_mem_usage_bytes, mem_pool_mem_usage_ratio, and mem_pool_workgroup_count. The implementation follows the established locking and metrics registration patterns from WorkGroupManager to avoid deadlocks with the metrics collector.
Changes:
- Added metrics infrastructure to
MemTrackerManagerwith thread-safe registration and update mechanisms - Updated
list_mem_trackers()to exclude the default memory pool, improving consistency - Added documentation in English, Chinese, and Japanese for the new metrics
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| be/src/exec/workgroup/mem_tracker_manager.h | Added MemTrackerMetrics struct, metrics-related private methods, and mutex for thread synchronization |
| be/src/exec/workgroup/mem_tracker_manager.cpp | Implemented metrics registration, update logic, and modified list_mem_trackers() to exclude default pool |
| be/test/exec/workgroup/work_group_manager_test.cpp | Updated test expectations to reflect that default memory pool is no longer included in the list, removed unnecessary sleep |
| docs/en/administration/management/monitoring/metrics.md | Added English documentation for the four new metrics |
| docs/zh/administration/management/monitoring/metrics.md | Added Chinese documentation for the four new metrics |
| docs/ja/administration/management/monitoring/metrics.md | Added Japanese documentation for the four new metrics |
Why I'm doing:
There are currently no backend metrics reporting for memory pools.
I previously tried to add them by extending the workgroup metrics, but this turned out to be an incorrect approach:
What I'm doing:
This PR implements metric reporting for
MemTrackerManagerand adds the following new metrics:The implementation follows the same locking structure that is present in
WorkGroupManager.MemTrackerManagerbecause the update_metrics callback hook passed toMetricRegistryneeds to be a closure which captures a write lock.WorkGroupManager.Minor: Changed
list_mem_trackers()method to not return the default memory pool name.Tests and Docs
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: