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: Expose rgw perf counters #21269

Merged
merged 2 commits into from Apr 9, 2018

Conversation

Projects
None yet
3 participants
@b-ranto
Copy link
Contributor

commented Apr 6, 2018

We are filtering rgw perf counters in mgr_module. This commit allows us
to expose perf counters to the mgr modules. This also directly exports
the rgw perf counters via prometheus module since the module calls the
get_all_perf_counters method to get all the counters and then exports them.

Signed-off-by: Boris Ranto branto@redhat.com

mgr: Expose rgw perf counters
We are filtering rgw perf counters in mgr_module. This commit allows us
to expose rgw perf counters to the mgr modules. This also directly
exports the rgw perf counters via prometheus module since the module
calls the get_all_perf_counters method to get all the perf counters and
then it exports them.

Signed-off-by: Boris Ranto <branto@redhat.com>

@b-ranto b-ranto force-pushed the b-ranto:wip-mgr-expose-rgw branch from 2584916 to f95f37c Apr 6, 2018

@b-ranto b-ranto requested review from jcsp, jan--f and pcuzner Apr 6, 2018

@b-ranto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

The following rgw perf counters are being exposed:

['client.rgw.node1.put_b', 'client.rgw.node1.get',
'client.rgw.node1.get_b', 'client.rgw.node1.cache_miss',
'client.rgw.node1.get_initial_lat', 'client.rgw.node1.cache_hit',
'client.rgw.node1.failed_req',
'client.rgw.node1.keystone_token_cache_hit', 'objecter.op_w',
'client.rgw.node1.qlen', 'client.rgw.node1.keystone_token_cache_miss',
'objecter.op_r', 'objecter.op_active', 'objecter.op_rmw',
'client.rgw.node1.put_initial_lat', 'client.rgw.node1.qactive',
'client.rgw.node1.req', 'client.rgw.node1.put']

This makes me a bit worried since it makes the metrics exported by prometheus look like this:

ceph_client_rgw_node1_get_b

We should probably change the prefix from client.rgw.<name> to just rgw.

rgw: Sanitize rgw perf counter names
The rgw perf counters are currently being named by the rgw names (i.e.
client.rgw.<name>.<something>). This changes their names to something
more sane like rgw.<something>. This also makes data exported by
prometheus ceph-mgr module a lot more useful as you do not need to use
the rgw name to get the metrics for an rgw.

Signed-off-by: Boris Ranto <branto@redhat.com>
@b-ranto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

I have pushed the commit that should sanitize the prefix that I was talking about in my previous comment.

@LenzGr LenzGr requested a review from votdev Apr 6, 2018

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

We should probably change the prefix from client.rgw. to just rgw.

is there still a way to differentiate which radosgw instance the counters are from, so we can have separate views for each gateway?

@b-ranto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

Yes, there is. When you are accessing the perf counters through cli you need to supply the admin socket to fetch the rgw perf counters data -- you just need to supply the rgw admin socket you are interested in.

In prometheus, you get ceph_daemon="rgw.<name>" label to know where these are coming from.

EDIT: btw: This is a standard naming scheme for osd/mon/others. AFAIK, no other perf counters put the daemon ids in the counter names.

@jcsp

jcsp approved these changes Apr 6, 2018

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

I agree that having the client name in the counter name was pretty weird!

@b-ranto b-ranto merged commit f29f45e into ceph:master Apr 9, 2018

4 of 5 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

@b-ranto b-ranto deleted the b-ranto:wip-mgr-expose-rgw branch Apr 9, 2018

@jcsp jcsp added feature mgr labels Apr 9, 2018

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

@b-ranto this probably should have gone through teuthology rados and rgw suites before merging (although it's unlikely to have broken anything)

@b-ranto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@jcsp I did not see any needs-qa or testing label so I went ahead and merged it, sorry about that. We can run it through the suites for the back-port PR #21303 ?

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

Ah, I see the misunderstanding -- needs-qa is often left off a PR until it has been reviewed (i.e. until it's ready for the tests to really be run), the absence doesn't mean the PR won't be tested. I should really have added the label when I reviewed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.