search

digitalocean / prometheus-client-c

A Prometheus Client in C
Other
158 stars 78 forks source link

#41 - process_cpu_seconds_total include utime + stime #45

Closed pavelmash closed 3 years ago

pavelmash commented 3 years ago

Calculate process_cpu_seconds_total as utime + stime + cutime + cstime instead of cutime + cstime.

This include an amount of time that this process has been scheduled in user and kernel modes (utime + stime) in addition to an amount of time that this process's waited-for children have been scheduled in user and kernel modes modes (cutime + cstime)

miroswan commented 3 years ago

@pavelmash After reading the man page for proc it's not directly clear to me that we'd want to include the the process time of the children of the target process. Can you provide some clarification for this PR?

pavelmash commented 3 years ago

The problem is what without this PR we include ONLY the process time of the children of the target process ( cutime + cstime). This is why prom_process_cpu_seconds_total metric is always 0 for /example/example.

I propose to include not only the the process time of the children but also a process time of the current process (a process libprom.so is loaded into) by adding utime + stime

Or, if you want to include only current process CPU usage, this metric can be calculated as utime + stime (but not cutime + cstime). See for example rust implementation

miroswan commented 3 years ago

Ah yes. This was a mistake. I much prefer that we only include utime + stime. Thank you for noticing this!

miroswan commented 3 years ago

@pavelmash What are your thoughts? Do you agree that it makes sense to only use (utime + stime) / sysconf(_SC_CLK_TCK)? See time.h

pavelmash commented 3 years ago

Yes, I agree - changed by [c96154f]

miroswan commented 3 years ago

Thanks!

MattIPv4 commented 3 years ago

Hey, @pavelmash - Thanks a ton for this sweet PR! 😄

Would you please shoot me an email when you get a chance? mcowley at digitalocean dot com 🎉