Francesco Romani has posted comments on this change. Change subject: Adding report_stats to virt.stats ......................................................................
Patch Set 7: (5 comments) looks generally ok, few comments inside. -1 for visibility. https://gerrit.ovirt.org/#/c/59066/7/lib/vdsm/virt/vmstats.py File lib/vdsm/virt/vmstats.py: PS7, Line 206: rx perhaps rename to rx_bytes ? same for 'tx' PS7, Line 207: rx_errors perhaps rx.errors rx.dropped ... (same for tx) what do you think? PS7, Line 209: report[netprefix + '.rx_rate'] = if_info['rxRate'] this and txRate are obsolete: https://gerrit.ovirt.org/#/c/59544/ Line 209: report[netprefix + '.rx_rate'] = if_info['rxRate'] Line 210: report[netprefix + '.tx'] = if_info['tx'] Line 211: report[netprefix + '.tx_errors'] = if_info['txErrors'] Line 212: report[netprefix + '.tx_dropped'] = if_info['txDropped'] Line 213: report[netprefix + '.tx_rate'] = if_info['txRate'] worth reporting iops as someone suggested perhaps in the users ML. Line 214: Line 215: reports.send(report) Line 216: except KeyError: Line 217: logging.exception('Report vm stats failed') PS7, Line 215: reports.send(report) you followed the same pattern everywhere, so if we agree about this, let's change all at once in a later patch. I think here you should just prepare the data to send, and actually send (and import `reports'|`metrics') elsewhere. No big deal however. -- To view, visit https://gerrit.ovirt.org/59066 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idf494c6a3087d04c12731c587b619f253dd51165 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
