Re-introduce per-process CPU collection #1146
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
blt
changed the base branch from
blt/don_t_coerce_counters_to_floats_avoid_metrics_handles
to
graphite-base/1146
This commit re-introduces per-process CPU data from /proc/{pid}/stat reusing the
same naming scheme from the cgroup sourced data, offset with stat. to avoid
summing issues in aggregations.
I have removed much of the stat code from the main procfs sample loop. The data
was either no longer used or is duplicated elsewhere. Some data could be
re-introduced if we desire by extending the poll loop in stat.rs.
I am continuing to remove more and more of our procfs crate integration. I
think, ultimately, we should be able to parse directly without the need of a
third-party dependency. That removal is near done now.
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
blt
force-pushed
the
blt/re-introduce_per-process_cpu_collection
branch
from
d71e2e6 to
c5f8569
Compare
blt
force-pushed
the
graphite-base/1146
branch
from
3b7ce8e to
4344486
Compare
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
blt
force-pushed
the
blt/re-introduce_per-process_cpu_collection
branch
from
c5f8569 to
5001c72
Compare
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
lading/src/observer/linux/procfs.rs
Outdated
| @@ -65,6 +77,8 @@ impl Sampler { | |||
| clippy::cast_possible_wrap | |||
| )] | |||
| pub(crate) async fn poll(&mut self) -> Result<(), Error> { | |||
| let mut proccess_info: FxHashMap<i32, ProcessInfo> = FxHashMap::default(); | |||
There was a problem hiding this comment.
I might be missing something in the control flow here, but it looks like this needs to be held across poll calls.
Related, once it is held for the duration of lading, expiration may be an issue. PID reuse seems unlikely, but memory bloat is a concern in cases where there are many short lived processes.
There was a problem hiding this comment.
No I just noticed that myself. Commit b84ec4a fixes this.
Expiration was a problem previously as well that we didn't solve, so we're no worse than before. But it is a potential problem I agree.
There was a problem hiding this comment.
For each poll would it be wrong to remove pid entries in the process_info map whose pids were not seen in the current poll to solve the expiration problem?
There was a problem hiding this comment.
No that would work. You could do it pretty cheaply by having two hashmaps, move entries from one to the other each loop and clear out the map that was being moved from.
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Merge activity
|
What does this PR do?
This commit re-introduces per-process CPU data from /proc/{pid}/stat reusing the
same naming scheme from the cgroup sourced data, offset with stat. to avoid
summing issues in aggregations.
I have removed much of the stat code from the main procfs sample loop. The data
was either no longer used or is duplicated elsewhere. Some data could be
re-introduced if we desire by extending the poll loop in stat.rs.
I am continuing to remove more and more of our procfs crate integration. I
think, ultimately, we should be able to parse directly without the need of a
third-party dependency. That removal is near done now.