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

os/bluestore: fix perf counters #13965

Merged
merged 8 commits into from Mar 25, 2017

Conversation

Projects
None yet
2 participants
@liewegas
Member

liewegas commented Mar 14, 2017

This is on top of wip-bluestore-dw (#13888); ignore the first bunch of commits

@liewegas liewegas added the bluestore label Mar 14, 2017

Show outdated Hide outdated src/pybind/ceph_daemon.py
@@ -187,7 +188,8 @@ def _print_headers(self, ostr):
sub_header = ""
for section_name, names in self._stats.items():
for stat_name, stat_nick in names.items():
for stat_name in self._stats_order[section_name]:

This comment has been minimized.

@dmick

dmick Mar 15, 2017

Member

Don't the headers also have to follow this order? And couldn't we get this effect by making the existing _stats an OrderedDict as well, and not need _stats_order?

@dmick

dmick Mar 15, 2017

Member

Don't the headers also have to follow this order? And couldn't we get this effect by making the existing _stats an OrderedDict as well, and not need _stats_order?

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 15, 2017

Member
Member

liewegas commented Mar 15, 2017

@@ -53,7 +53,7 @@ BlueFS::~BlueFS()
void BlueFS::_init_logger()
{
PerfCountersBuilder b(cct, "BlueFS",

This comment has been minimized.

@dmick

dmick Mar 15, 2017

Member

Commit message says "log", but this is just adding a nick, meaning it will show up in the schema and be grabbed by daemonperf (what you refer to as 'surface' in the next commit), right?

@dmick

dmick Mar 15, 2017

Member

Commit message says "log", but this is just adding a nick, meaning it will show up in the schema and be grabbed by daemonperf (what you refer to as 'surface' in the next commit), right?

This comment has been minimized.

@dmick

dmick Mar 24, 2017

Member

commit message is still misleading; this doesn't add any metrics, it just makes them have a nick (and changes the upper-level name to lowercase)

@dmick

dmick Mar 24, 2017

Member

commit message is still misleading; this doesn't add any metrics, it just makes them have a nick (and changes the upper-level name to lowercase)

@@ -179,6 +179,10 @@ class PerfCounters
m_name = s;
}
void set_suppress_nicks(bool b) {

This comment has been minimized.

@dmick

dmick Mar 15, 2017

Member

Why would one code this rather than just setting the nick to ""? If it were dynamic, that's one thing, but given you have to change the C++ code either way...?

@dmick

dmick Mar 15, 2017

Member

Why would one code this rather than just setting the nick to ""? If it were dynamic, that's one thing, but given you have to change the C++ code either way...?

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 15, 2017

Member
Member

liewegas commented Mar 15, 2017

badone pushed a commit to badone/ceph that referenced this pull request Mar 19, 2017

PGLog::rewind_divergent_log: fix rollback_info_trimmed_to before index()
Fixes: #13965
Signed-off-by: Samuel Just <sjust@redhat.com>
(cherry picked from commit 053ee91)

liewegas added some commits Mar 14, 2017

ceph: daemonperf: order metrics to match asok json dump
The daemons report this in a particular order; match that in the
daemonperf output.  This corresponds to the numeric value of the l_*
enum.

Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: log kv latencies
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore/BlueFS: log key bluefs metrics
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: surface key metrics for daemonperf
Signed-off-by: Sage Weil <sage@redhat.com>
common/perf_counters: allow perfcounters to be excluded from daemonperf
By omitting the 'nick' we exclude a whole group of metrics from the
daemonperf results.

Signed-off-by: Sage Weil <sage@redhat.com>
osd: exclude 'objecter' perfcounters from daemonperf
Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore: fix many perfcounter types
Most of these are counters, not gauges.

Signed-off-by: Sage Weil <sage@redhat.com>
os/bluestore/BlueFS: measure used bytes, not free bytes
This is more useful info for humans.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 21, 2017

Member

@dmick okay, rebased! agree the terminology is awkward. the 'daemonperf shows things with a nick' thing has always been goofy too, but until we come up with a better idea that's what we've got!

Member

liewegas commented Mar 21, 2017

@dmick okay, rebased! agree the terminology is awkward. the 'daemonperf shows things with a nick' thing has always been goofy too, but until we come up with a better idea that's what we've got!

@liewegas liewegas added the needs-qa label Mar 21, 2017

@dmick

This comment has been minimized.

Show comment
Hide comment
@dmick

dmick Mar 21, 2017

Member

I never really understood that anyway; why not just have a default list in daemonperf? Why require the cluster to control what its examination tool shows, either with nick changes or set_suppress_nick or whatever? It's more complex and more of a pain (no ad-hoc changes, c++ so requires rebuild, etc.)

Member

dmick commented Mar 21, 2017

I never really understood that anyway; why not just have a default list in daemonperf? Why require the cluster to control what its examination tool shows, either with nick changes or set_suppress_nick or whatever? It's more complex and more of a pain (no ad-hoc changes, c++ so requires rebuild, etc.)

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 24, 2017

Member

@dmick I think the idea is that the latest cost generally knows what hte most interesting set of metrics is, so that's a good place to start. The nick overloading is annoying, though, and it you should be able ot pick other metrics on the cmdline to watch.

Anyway, is this okay as is?

Member

liewegas commented Mar 24, 2017

@dmick I think the idea is that the latest cost generally knows what hte most interesting set of metrics is, so that's a good place to start. The nick overloading is annoying, though, and it you should be able ot pick other metrics on the cmdline to watch.

Anyway, is this okay as is?

@dmick

This comment has been minimized.

Show comment
Hide comment
@dmick

dmick Mar 24, 2017

Member

What does "latest cost" mean? Is 'cost' an autocorrect victim? code?

(from irc: yes, thinko/autocorrecto for 'code')

Member

dmick commented Mar 24, 2017

What does "latest cost" mean? Is 'cost' an autocorrect victim? code?

(from irc: yes, thinko/autocorrecto for 'code')

@liewegas liewegas merged commit d8d24be into ceph:master Mar 25, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@liewegas liewegas deleted the liewegas:wip-bluestore-pc branch Mar 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment