Add logical processors to custom field #271
Conversation
justinbarclay
force-pushed
the
jb/split-count-and-threads
branch
from
b8fe02e to
6391fb5
Compare
unix/src/machine_stats/__init__.py
Outdated
| @@ -222,6 +221,9 @@ def v2_runner_on_ok(self, result): | |||
| "storage_allocated_gb": storage_allocated_gb(facts), | |||
| "storage_used_gb": storage_used_gb(facts), | |||
| "cpu_count": cpu_count(facts), | |||
| "custom_fields": { | |||
| "CPU_LogicalProcessors": cpu_logical_processors(facts) | |||
There was a problem hiding this comment.
Aside: how would you feel about making this a default server field?
…r than overwriting them
There was a problem hiding this comment.
Thanks for taking a look at this Justin, I think it's really important that our discovery tools have a unified approach to the data we're collecting.
I've done some testing on this today and it seems like there are still a few kinks to work out.
Firstly, in MS4W cpu_count refers to physical cores, where as in MS4U it appears to refer to CPU sockets. Which one is the intention here?
Also, in my testing I found that MS4W is incorrectly counting logical cores on the m5d.metal instance. It's actually counting physical cores. What's strange is that for a VM t3.2xlarge it's correctly counting logical and physical cores. This could be a quirk of the metal instance but AWS and the machine itself seems to think it has 96 logical cores rather than the reported 48.
As I mentioned in the comments, for MS4U I've added a commit to append to the custom field key rather that overwriting it, feel free to use at your discretion.
I also have a Q about nomenclature, and whether we want to codify cpu_logical_processors as a first-class server attribute rather than a custom field.
| @@ -222,6 +221,9 @@ def v2_runner_on_ok(self, result): | |||
| "storage_allocated_gb": storage_allocated_gb(facts), | |||
| "storage_used_gb": storage_used_gb(facts), | |||
| "cpu_count": cpu_count(facts), | |||
| "custom_fields": { | |||
There was a problem hiding this comment.
I'm not able to get this result in my testing, I believe this custom_fields key/value is being overwritten here.
There was a problem hiding this comment.
The core of the issue was actually here - the update_results function was just replacing the entire custom_fields key in the result hash. Since CPU stats are taken last, they were overwriting the custom field added here. I've taken the liberty of pushing up a commit to this branch which fixes it - feel free to use or revert at your discretion if you don't agree with the approach.
There was a problem hiding this comment.
That's weird; thanks for the deep dive into this.
windows/server_stats.ps1
Outdated
| @@ -167,7 +167,8 @@ $ServerStats = { | |||
| CPU_Description = $cpu.Description | |||
| CPU_Manufacturer = $cpu.Manufacturer | |||
| CPU_L2CacheSize = $cpu.L2CacheSize | |||
| CPU_L3CacheSize = $cpu.L3CacheSize | |||
| CPU_L3CacheSize = $cpu.L3CacheSize | |||
| CPU_LogicalProcessors = $cpu.NumberOfLogicalProcessors | |||
There was a problem hiding this comment.
Nit: the naming convention CPU_LogicalProcessors feels fine here but out of place in the MS4U python and in our app. I think it should be cpu_logical_processors.
There's precedent for this as MS4W already returns the custom fields that we actually use in our rails naming convention:
There was a problem hiding this comment.
As we discussed, I was just following the naming conventions I established, but I agree. We should normalize on snake_case
Note that we still need to test and validate that this will run correctly in vms