mgr/dashboard: Refactor multiple duplicates of get_rate()
#21022
Conversation
sebastian-philipp
requested review from
tspmelo,
votdev,
dillaman,
Devp00l and
jcsp
| o['stats'][prop] = self.get_rate(osd_spec, s) | ||
| o['stats_history'][prop] = self.get_counter(osd_spec, s) | ||
| o['stats'][prop] = CephService.get_rate('osd', osd_spec, s) | ||
| o['stats_history'][prop] = mgr.get_counter('osd', osd_spec, s) |
There was a problem hiding this comment.
It needs to be o['stats_history'][prop] = mgr.get_counter('osd', osd_spec, s)[s] with [s] at the end to get the same return as from the local method before.
There was a problem hiding this comment.
right. looks like we need a teuthology test for that, too.
There was a problem hiding this comment.
fixed. Also added a new API test
sebastian-philipp
force-pushed
the
dashboard_get_range
branch
2 times, most recently
from
30dbc1e
to
90b64a1
Compare
| @@ -12,6 +12,8 @@ | |||
| @AuthRequired() | |||
| class TcmuIscsi(RESTController): | |||
| def _get_rate(self, daemon_type, daemon_name, stat): | |||
| # Also see ..services.ceph_service.CephService#get_rate | |||
There was a problem hiding this comment.
is this comment relevant for the implementation of this function? if not I would remove it.
There was a problem hiding this comment.
This function and .services.ceph_service.CephService#get_rate are very similar and share a common history. In contrast to .services.ceph_service.CephService#get_rate, this function calculates the differentiation for all inputs, instead of just the last one. In the long term, we should IMO aim for unifying both functions. E.g. using this get_rate also in other places.
There was a problem hiding this comment.
Ok, in that case I would remove the comment, and open an issue in tracker.ceph.com to keep track of that enhancement.
There was a problem hiding this comment.
With this comment, the relationship between both get_rates is also clear in the git history. Why do you think this comment is harmful?
There was a problem hiding this comment.
I don't think it's harmful, (it's just a comment), but usually comments explain or alert to some part of the code that is unclear, while this comment feels more like a "TODO" item, that should be tracked in the tracker rather than in the code.
There was a problem hiding this comment.
Note that I had changed this function after I noticed “get_rate” is broken throughout the dashboard [1]
|
dashboard QA tests were successful: http://pulpito.ceph.com/rdias-2018-03-27_15:04:07-rados:mgr-wip-rdias-testing-distro-basic-smithi/ |
sebastian-philipp
force-pushed
the
dashboard_get_range
branch
from
90b64a1
to
97cbce1
Compare
... And `get_latest()`. * OSD Controller: `.stats_history` now returns the derivative. Fixes https://tracker.ceph.com/issues/23389 Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
sebastian-philipp
force-pushed
the
dashboard_get_range
branch
from
97cbce1
to
54d31a9
Compare
Refactored `OsdTest` to make use of `self.assertSchema()` Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
sebastian-philipp
force-pushed
the
dashboard_get_range
branch
from
54d31a9
to
012be1d
Compare
|
QA tests run successfully: http://pulpito.ceph.com/rdias-2018-04-11_08:29:22-rados:mgr-wip-rdias-testing-distro-basic-mira/ |
... And
get_latest().I've removed all duplicates of
get_ratewith the exception of the iSCSI controller and moved it toCephService.Also, I've moved
get_latest()toMgrModule, as it is already used there. And there are two other mgr modules make use ofget_latest.Also affected by #21021
EDIT:
.stats_historynow returns thederivative. Fixes https://tracker.ceph.com/issues/23389
Signed-off-by: Sebastian Wagner sebastian.wagner@suse.com