Nir Soffer has posted comments on this change. Change subject: rpc: Log important info from VM stats ......................................................................
Patch Set 14: (2 comments) https://gerrit.ovirt.org/#/c/58465/14/vdsm/API.py File vdsm/API.py: Line 1200: Line 1201: class Global(APIBase): Line 1202: ctorArgs = [] Line 1203: Line 1204: _allvmstats_log = PeriodicLog() > - We should call it _allvmstats_log, because we can't share the instance am Having one periodic logger per verb does not scale. We need a solution which can be use for any verb, so this is not the place to keep it. We should not use the log instance of this class, we should have special logger for periodic logs, so we can control this log separately. This eliminate the need to pass the log instance to the log method. Using defaults is not a way to control logging in multiple locations, since some caller may use the defaults, and some not. If you want to central control, don't provide any defaults when creating the verb. If you want different defaults for each verb, define the values when you created the logger. In any case, the settings for different verbs should be in one location (maybe configurable?). I think what we need is a periodiclog.py module, with this api: def add_logger(name, interval, timeout): """ Add periodic logger, logger call once for each interval calls, or every timeout seconds (lower). """ def log(name, fmt, *args): """ Format and log message for name. Raises RuntimeError if logger for this name does not exist. """ Code that want to log such message will configure the periodic logger on startup: from vdsm import periodiclogger ... periodiclog.add_logger("getAllVmStats", 50, 300) And will call the logger when needed: periodiclog.log("getAllVmStats", "current vm stats: %s", AllVmStats(stats)) Line 1205: Line 1206: def __init__(self): Line 1207: APIBase.__init__(self) Line 1208: Line 1346: statsList = self._cif.getAllVmStats() Line 1347: statsList = hooks.after_get_all_vm_stats(statsList) Line 1348: self._allvmstats_log.log(self.log, logging.DEBUG, Line 1349: "Current getAllVmStats: %s", Line 1350: AllVmStatsValue(statsList)) > We don't want to perform the extraction in case the data is not going to be Right, lets keep it then. Line 1351: return {'status': doneCode, 'statsList': LargeValue(statsList)} Line 1352: Line 1353: def hostdevListByCaps(self, caps=None): Line 1354: devices = hostdev.list_by_caps(caps) -- To view, visit https://gerrit.ovirt.org/58465 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifcbac615323b62fb9a27e5c0f5a4e98990076146 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.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
