CP-30614: Use RRD to update memory stats #3827
Conversation
|
I haven't looked at this in detail yet, but my initial thought is that the duplication among the three monitor files could be reduced by using a functor. This is a good exercise in OCaml as well :) |
ocaml/xapi/monitor_pvs_proxy.ml
Outdated
| @@ -60,7 +57,7 @@ let get_changes () = | |||
| | Rrd.VT_Float v -> int_of_float v | |||
| | Rrd.VT_Unknown -> -1 | |||
| in | |||
| if ds.Ds.ds_name = Xapi_globs.pvs_proxy_status_ds_name then | |||
| if ds.Ds.ds_name = "pvscache_status" then | |||
There was a problem hiding this comment.
Why would you "unglobal" this? I think that it is good practice to have constant strings factored out.
There was a problem hiding this comment.
To open a conversation ;)
It's not a "xapi global" constant string at all, it gives the wrong impression that is shared or reused, at this point having it as a global provides indirection without benefit.
It might make sense to put it into xcp-idl and share the value with the plugin that generates the data. Same with the other metrics-related string constants.
|
And don't forget the final piece of the puzzle: removing the actual |
|
mem_vms and pvs_proxy are structurally very similar, mem_host is quite different and I do not think it's worth to try to put them along the others, maybe reusing the file-reading code would be enough. |
psafont
changed the title
psafont
force-pushed
the
CP-30614
branch
2 times, most recently
from
373fd2c
to
7bcdc54
Compare
|
Is this ready to be merged? |
|
I think so, @robhoes might want to take another look at it |
ocaml/xapi/monitor_mem_host.ml
Outdated
| module Lstext = Stdext.Listext.List | ||
| module Mcache = Monitor_dbcalls_cache | ||
|
|
||
| module D = Debug.Make(struct let name = "monitor_mem_vms" end) |
There was a problem hiding this comment.
I think this looks good and am happy to merge this now. However, I think that we should optimise this a little further in a follow-up PR. The performance of these functions is important, because the run every 5 s.
For example, we now call Monitor_types.find_rrd_files three times, which does the same directory listing each time. I wonder if it is possible to make the three monitor files even more similar, and merge them into one. I know that, for example, the host-memory one is different in the way the cache works, because it is using single values rather than hash tables. However, we could still decide to use a hash table for those as well. I don't see many fundamental differences, especially in the get_changes () functions.
separate the metrics directory name from pvs proxy metrics. remove variable that's in practice internal to the filtering method for pvs. rename the pvsproxy variable so it is named like other metrics variables. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
on get_changes differentiate between filtering and transforming the data. On update consistently use pipelines instead of using variable declaration and using ifs. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
also removed opens from monitor_types Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This allows to share the cache between several readers Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
this way it can be shared with other dss file readers Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This remove a xenctrl usage from xapi. Both VM and Host statistics for memory got moved to their respective files. They more or less follow the same structure as monitor_pvs_proxy Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Memory statistics are now collected from rrd files in 2 new modules. These modules follow the same structure as
ocaml/xapi/monitor_pvs_proxy.mlI'd like to reduce the amount of repeated/shared code in them, I'm open to suggestions.Left to do: change how domain information is gathered from a string in
ocaml/xapi/monitor_types.mlDepends on xapi-project/xcp-rrdd#89