cmonitor support for - Prometheus

[satyabratabharati]

Following changes have been done to cmonitor_collector :

1. Added prometheus_kpi.h : provides Prometheus APIs to create different metrics type Gauge|Counter

2. Changes to collector classes from where the KPIs are send to to Prometheus API.

• cgroups_processes.cpp
• cgroups_network.cpp
• cgroups_memory.cpp
• cgroups_cpuacct.cpp
  • system_cpu.cpp
  • system_disk.cpp
  • system_network.cpp
  • system_memory.cpp

3. Changes to - main.cpp for additional command line parameters for Prometheus and additional Labels.

prometheus_output.zip

Pending :

1. Inotify support to cmonitor in case container is deleted/restarted

[satyabratabharati]

@f18m , Kindly review the changes..!!

I have tried to send all the cgroup and system related KPIs to Prometheus.

However I am not quite sure about the various Labels added to the KPIs and need your input on the same.

Thank You.

[f18m]

Hi @satyabratabharati , I raised some cosmetic comments at some points of the new files but I'm raising in this commetn the most important review points:

• adding e.g. in side CMonitorSystem::sample_cpu_stat() a lot of Prometheus-specific code is not the way cmonitor is designed: it works in the way that each measurement is already sent to a CMonitorOutputFrontend. Today CMonitorOutputFrontend supports both direct JSON write and InfluxDB. We must add Prometheus support into CMonitorOutputFrontend
• I 'm not sure we need a CMonitorPromethues class: we could integrate the Prometheus::Exposer and prometheus::Registry directly into CMonitorOutputFrontend.
• We need to find the best Prometheus-native way to expose different classes of KPIs: those related to the whole system (CMonitorSystem), those related to cgroups only (CMonitorCgroups)

[f18m]

About this:

• We need to find the best Prometheus-native way to expose different classes of KPIs: those related to the whole system (CMonitorSystem), those related to cgroups only (CMonitorCgroups)

reading the https://prometheus.io/docs/practices/naming/ docs page it looks like that each metric "must have a single unit"; this means that statistics reported today by cmonitor-collector inside each sample:

• proc_loadavg.*
• cgroup_cpuacct_stats.*
• cgroup_memory_stats.*
• cgroup_network.* should all be different Prometheus metrics; e.g. cgroup_cpuacct_stats.cpu_tot.user has a "ratio" unit (i.e. a percentage) while e.g. cgroup_memory_stats.stat.rss has "bytes" units.

The creation of asample like those above typically happens with the following sequence of function calls to CMonitorOutputFrontend:

• psample_start
• psection_start
• pdouble
• psection_end
• push_current_sample we should find a way to map these to the right prometheus BuildCounter() call. I think the cmonitor sample "section" should define the Prometheus "metric" name. Then the cmonitor pdouble/phex/etc helpers calls inside the same section should define Prometheus "labels" attached to the same metric. That's because very often the values inside the same section have the same measurement unit.

e.g. the section "cgroup_cpuacct_stats.cpu_tot" has the "user" and "sys" fields with same unit. the section "cgroup_network.ethXXX" has "ibytes" and "obytes" with same unit, etc etc

[f18m]

To test the build system with Prometheus you can run:

cd <your-folder>t/cmonitor/collector
make -f <your-folder>/cmonitor/.copr/Makefile rpm outdir=/tmp/cmonitor-rpm

THis is the way COPR will start the build of cmonitor-collector. It will do 2 things

1. create the SOURCE RPM
2. use rpmbuild to build a BINARY RPM from the SOURCE RPM

Then we need to change the collector.spec file in its %build stage to fetch Prometheus library and then invoke the GNU make with PROMETHEUS_SUPPORT=1 flag

[f18m]

Hi

I didn't go too much in the details of the review, but I raise first few major comments:

1. the changes to CMonitorCgroups::sample_processes() I think are producing an invalid JSON output now with a structure like:

  "cgroup_tasks": {
     "process": {
         ...
     },
     "cpu": {
         ...
     },
     ..
     "process": {
         ...
     },
     "cpu": {
         ...
     },
     ..
  }

which does not fit a hierarchical logical division (the "cpu" section should be a sub-sub-section of the "process" part) and contains repeated keys (if there are 2 processes or 2 threads) since the "pid" has been removed from the subsection name. We must find a way to preserve the JSON output. I think a way coudl be:

  "cgroup_tasks": {
     "pid_1234": {
       "proc_info": {
         pid=1234,
         cmd="redis-server",
         ...
       }
       "cpu": {
         ...
       },
     },
     "pid_5678": {
       "cpu": {
         ...
       },
     }
  }

where the "pid_1234" sub-section is SKIPPED entirely for the PROMETHEUS output and is present only in the JSON output. Moreover the "proc_info" sub-sub-section would become, in case of the PROMETHEUS output, a set of labels to be attached to ALL other metrics produced for tha same PID. This means we should put some "if (output_type == PROMETHEUS) { do that } else { do something else}" in the cgroup_processes.cpp but right now I don't see many alternatives

2. in the generate_prometheus_metric() function the BuildGauge() method is invoked every time; this is wrong and should be done only at the start of the cmonitor_collector. Then the same "Gauge" instance should be jsut reused all the times. This requires probably creating a std::map<std::string /* metric name /, prometheus::Gauge /* the gauge */> inside CMonitorOutputFrontend. Every time we must update a Prometheus gauge, we first check if that Gauge already exists in such map, if not it's created with BuildGauge().

3. since Prometheus output takes doubles, we should have a new field of type "double" inside the CMonitorOutputMeasurement class to avoid converting integers/doubles to strings and then back to doubles using atof().

[satyabratabharati]

@f18, kindly have a short review.

I have reworked and implemented the 3 measure points that you pointed out in last review.

Thank You.

out_file.zip

[satyabratabharati]

@f18m , I have implemented the review comments.

Kindly have a short look .

Thanks.

[f18m]

Hi @satyabratabharati , sorry for the huge delay but finally I've done the PR review. I raised only minor review points. I think it's a great work so I'm happy to merge it as soon as these comments are addressed, thanks!

[satyabratabharati]

Hi @satyabratabharati , sorry for the huge delay but finally I've done the PR review. I raised only minor review points. I think it's a great work so I'm happy to merge it as soon as these comments are addressed, thanks!

@f18m , Many thanks for the review. Its a privilege and I am glad to contribute.

Thanks.