-
Notifications
You must be signed in to change notification settings - Fork 74
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
Reduce NPD full config #520
Conversation
/gcbrun |
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.
Should we change the config from "all" then? What if we want to enable the default, full NPD list in the future?
case "all": |
"disk/avg_queue_len": {"disk/avg_queue_len"}, | ||
"disk/bytes_used": {"disk/bytes_used"}, | ||
"disk/percent_used": {"disk/percent_used"}, | ||
"disk/io_time": {"disk/io_time"}, | ||
"disk/merged_operation_count": {"disk/merged_operation_count"}, | ||
"disk/operation_bytes_count": {"disk/operation_bytes_count"}, | ||
"disk/operation_count": {"disk/operation_count"}, | ||
"disk/operation_time": {"disk/operation_time"}, | ||
"disk/weighted_io": {"disk/weighted_io"}, |
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.
All of these seem useful for disk bound workloads.
"memory/anonymous_used": {"memory/anonymous_used"}, | ||
"memory/bytes_used": {"memory/bytes_used"}, | ||
"memory/dirty_used": {"memory/dirty_used"}, | ||
"memory/page_cache_used": {"memory/page_cache_used"}, | ||
"memory/unevictable_used": {"memory/unevictable_used"}, | ||
"memory/percent_used": {"memory/percent_used"}, |
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.
These also seem useful, except perhaps unevictable?
My intention was for the "all" term to describe the full set of metrics that CS supports, not necessarily everything supported by NPD. So if in the future we add support for more metrics, the definition would naturally change with it (and we would clearly document this change for the new image). IMO creating a dedicated label for this specific set of metrics going forward is harder to maintain, and it's difficult to think of an appropriate name since it's just what we think is useful at the moment. We could change it to something like "all-supported" for more clarity/flexibility, though. WDYT? |
/gcbrun |
Based on security review recommendations, we should be conservative about which metrics we enable in NPD. This reduces the full list of supported metrics to a smaller one.