diff --git a/ocaml/libs/xapi-rrd/lib/rrd.ml b/ocaml/libs/xapi-rrd/lib/rrd.ml index c9d646345c..75610964fc 100644 --- a/ocaml/libs/xapi-rrd/lib/rrd.ml +++ b/ocaml/libs/xapi-rrd/lib/rrd.ml @@ -621,9 +621,9 @@ let rrd_add_ds_unsafe rrd timestamp newds = rrd.rrd_rras } -(** Add in a new DS into a pre-existing RRD. Preserves data of all the other archives - and fills the new one full of NaNs. Note that this doesn't fill in the CDP values - correctly at the moment! +(** Add in a new DS into a pre-existing RRD. Preserves data of all the other + archives and fills the new one full of NaNs. Note that this doesn't fill + in the CDP values correctly at the moment! *) let rrd_add_ds rrd timestamp newds = @@ -632,28 +632,27 @@ let rrd_add_ds rrd timestamp newds = else rrd_add_ds_unsafe rrd timestamp newds -(** Remove the named DS from an RRD. Removes all of the data associated with it, too *) +(** Remove the named DS from an RRD. Removes all of the data associated with + it, too. THe function is idempotent. *) let rrd_remove_ds rrd ds_name = - let n = - Utils.array_index ds_name (Array.map (fun ds -> ds.ds_name) rrd.rrd_dss) - in - if n = -1 then - raise (Invalid_data_source ds_name) - else - { - rrd with - rrd_dss= Utils.array_remove n rrd.rrd_dss - ; rrd_rras= - Array.map - (fun rra -> - { - rra with - rra_data= Utils.array_remove n rra.rra_data - ; rra_cdps= Utils.array_remove n rra.rra_cdps - } - ) - rrd.rrd_rras - } + match Utils.find_index (fun ds -> ds.ds_name = ds_name) rrd.rrd_dss with + | None -> + rrd + | Some n -> + { + rrd with + rrd_dss= Utils.array_remove n rrd.rrd_dss + ; rrd_rras= + Array.map + (fun rra -> + { + rra with + rra_data= Utils.array_remove n rra.rra_data + ; rra_cdps= Utils.array_remove n rra.rra_cdps + } + ) + rrd.rrd_rras + } (** Find the RRA with a particular CF that contains a particular start time, and also has a minimum pdp_cnt. If it can't find an @@ -698,18 +697,17 @@ let find_best_rras rrd pdp_interval cf start = List.filter (contains_time newstarttime) rras let query_named_ds rrd as_of_time ds_name cf = - let n = - Utils.array_index ds_name (Array.map (fun ds -> ds.ds_name) rrd.rrd_dss) - in - if n = -1 then - raise (Invalid_data_source ds_name) - else - let rras = find_best_rras rrd 0 (Some cf) (Int64.of_float as_of_time) in - match rras with - | [] -> - raise No_RRA_Available - | rra :: _ -> - Fring.peek rra.rra_data.(n) 0 + match Utils.find_index (fun ds -> ds.ds_name = ds_name) rrd.rrd_dss with + | None -> + raise (Invalid_data_source ds_name) + | Some n -> ( + let rras = find_best_rras rrd 0 (Some cf) (Int64.of_float as_of_time) in + match rras with + | [] -> + raise No_RRA_Available + | rra :: _ -> + Fring.peek rra.rra_data.(n) 0 + ) (******************************************************************************) (* Marshalling/Unmarshalling functions *) @@ -876,30 +874,26 @@ let from_xml input = (* Purge any repeated data sources from the RRD *) let ds_names = ds_names rrd in - let ds_names_set = Utils.setify ds_names in - let ds_name_counts = - List.map - (fun name -> - let x, _ = List.partition (( = ) name) ds_names in - (name, List.length x) - ) - ds_names_set - in - let removals_required = - List.filter (fun (_, x) -> x > 1) ds_name_counts - in - List.fold_left - (fun rrd (name, n) -> - (* Remove n-1 lots of this data source *) - let rec inner rrd n = - if n = 1 then - rrd - else - inner (rrd_remove_ds rrd name) (n - 1) - in - inner rrd n - ) - rrd removals_required + List.sort_uniq String.compare ds_names + |> List.filter_map (fun name -> + match List.filter (String.equal name) ds_names with + | [] | [_] -> + None + | x -> + Some (name, List.length x) + ) + |> List.fold_left + (fun rrd (name, n) -> + (* Remove n-1 lots of this data source *) + let rec inner rrd n = + if n = 1 then + rrd + else + inner (rrd_remove_ds rrd name) (n - 1) + in + inner rrd n + ) + rrd ) input diff --git a/ocaml/libs/xapi-rrd/lib/rrd_utils.ml b/ocaml/libs/xapi-rrd/lib/rrd_utils.ml index c0863d0175..aa959a042d 100644 --- a/ocaml/libs/xapi-rrd/lib/rrd_utils.ml +++ b/ocaml/libs/xapi-rrd/lib/rrd_utils.ml @@ -47,13 +47,13 @@ end let isnan x = match classify_float x with FP_nan -> true | _ -> false -let array_index e a = +let find_index f a = let len = Array.length a in let rec check i = if len <= i then - -1 - else if a.(i) = e then - i + None + else if f a.(i) then + Some i else check (i + 1) in @@ -62,23 +62,6 @@ let array_index e a = let array_remove n a = Array.append (Array.sub a 0 n) (Array.sub a (n + 1) (Array.length a - n - 1)) -let filter_map f list = - let rec inner acc l = - match l with - | [] -> - List.rev acc - | x :: xs -> - let acc = match f x with Some res -> res :: acc | None -> acc in - inner acc xs - in - inner [] list - -let rec setify = function - | [] -> - [] - | x :: xs -> - if List.mem x xs then setify xs else x :: setify xs - (** C# and JS representation of special floats are 'NaN' and 'Infinity' which are different from ocaml's native representation. Caml is fortunately more forgiving when doing a float_of_string, and can cope with these forms, so diff --git a/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml b/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml index c46a33d6f9..3decd26067 100644 --- a/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml +++ b/ocaml/xcp-rrdd/bin/rrdd/rrdd_monitor.ml @@ -51,10 +51,10 @@ let merge_new_dss rrdi dss = !Rrdd_shared.enable_all_dss || ds.ds_default in let default_dss = StringMap.filter should_enable_ds dss in - (* NOTE: It's enough to check if all the default datasources have been added - to the RRD_INFO, because if a non-default one has been enabled at runtime, - it's added to the RRD immediately and we don't need to bother *) - let new_dss = + (* NOTE: Only add enabled dss to the live rrd, ignoring non-default ones. + This is because non-default ones are added to the RRD when they are + enabled. *) + let new_enabled_dss = StringMap.filter (fun ds_name _ -> not (StringMap.mem ds_name rrdi.dss)) default_dss @@ -73,7 +73,7 @@ let merge_new_dss rrdi dss = rrd_add_ds_unsafe rrd timestamp (Rrd.ds_create ds.ds_name ds.Ds.ds_type ~mrhb:300.0 Rrd.VT_Unknown) ) - new_dss rrdi.rrd + new_enabled_dss rrdi.rrd ) module OwnerMap = Map.Make (struct diff --git a/ocaml/xcp-rrdd/bin/rrdd/rrdd_server.ml b/ocaml/xcp-rrdd/bin/rrdd/rrdd_server.ml index f8f3c99bf8..d9d41114e0 100644 --- a/ocaml/xcp-rrdd/bin/rrdd/rrdd_server.ml +++ b/ocaml/xcp-rrdd/bin/rrdd/rrdd_server.ml @@ -347,7 +347,7 @@ let send_host_rrd_to_master master_address = let fail_missing name = raise (Rrdd_error (Datasource_missing name)) (** {add_ds rrdi ds_name} creates a new time series (rrd) in {rrdi} with the - name {ds_name}. The operation fails if rrdi does not contain any live + name {ds_name}. The operation fails if rrdi does not contain any datasource with the name {ds_name} *) let add_ds ~rrdi ~ds_name = match Rrd.StringMap.find_opt ds_name rrdi.dss with diff --git a/ocaml/xcp-rrdd/bin/rrdd/rrdd_shared.ml b/ocaml/xcp-rrdd/bin/rrdd/rrdd_shared.ml index 08807e39b7..883f9844cb 100644 --- a/ocaml/xcp-rrdd/bin/rrdd/rrdd_shared.ml +++ b/ocaml/xcp-rrdd/bin/rrdd/rrdd_shared.ml @@ -77,10 +77,13 @@ let mutex = Mutex.create () type rrd_info = { rrd: Rrd.rrd + (** Contains the live metrics, i.e. The datasources that are enabled + and being collected .*) ; mutable dss: (float * Ds.ds) Rrd.StringMap.t - (* Important: this must contain the entire list of datasources associated - with the RRD, even the ones disabled by default, as rrd_add_ds calls - can enable DSs at runtime *) + (** Important: this must contain the entire list of datasources + associated with the RRD, even the ones disabled by default, because + functions like rrd_add_ds or rrd_remove_ds expect disabled + datasources to be present. e.g. to enable DSs at runtime *) ; mutable domid: int }