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

mgr: apply a threshold to perf counter prios #16699

Merged
merged 2 commits into from Sep 15, 2017

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Jul 31, 2017

...so that we can control the level of load
we're putting on ceph-mgr with perf counters. Don't collect
anything below PRIO_USEFUL by default.

Signed-off-by: John Spray john.spray@redhat.com

static const int COMPAT_VERSION = 1;

public:
uint32_t stats_period;
uint32_t stats_threshold;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe assign a safe default value to it here?

@@ -227,9 +227,17 @@ void MgrClient::send_report()
pcc->with_counters([this, report](
const PerfCountersCollection::CounterMap &by_path)
{
auto include_counter = [this](const PerfCounters::perf_counter_data_any_d &ctr){
if (ctr.prio >= (int)stats_threshold) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, just

  return ctr.prio >= (int)stats_threshold;

@liewegas
Copy link
Member

echo kefu's nits, otherwise lgtm!

John Spray added 2 commits August 31, 2017 12:16
...so that we can control the level of load
we're putting on ceph-mgr with perf counters.  Don't collect
anything below PRIO_USEFUL by default.

Signed-off-by: John Spray <john.spray@redhat.com>
ceph-mgr has missed out on the `config set` command
that the other daemons got recently: add it here
and hook it all up to the stats period and threshold
settings.

Signed-off-by: John Spray <john.spray@redhat.com>
@jcsp
Copy link
Contributor Author

jcsp commented Aug 31, 2017

Fixed the nits and added runtime updates to the config (realised mgr never got the config set command, so now it has it), should be good to go

@tchaikov
Copy link
Contributor

might want to have a simple test in suite/rados/mgr.

@tchaikov tchaikov merged commit d154a9d into ceph:master Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants