Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logical processors to custom field #271

Merged
merged 5 commits into from
Jul 10, 2024
Merged

Add logical processors to custom field #271

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6391fb5
Add logical processors to custom field
justinbarclay Jul 4, 2024
474f44a
Updated the update_results function to append any custom fields rathe…
SamDesmondKing Jul 9, 2024
705f696
fix: flake to load all dependencies as outlined in contributing.md
justinbarclay Jul 9, 2024
322797c
fix: bug in cpu_count where it may only retrieve sockets
justinbarclay Jul 9, 2024
2eba583
chore: Rename custom field to be consistent with the api
justinbarclay Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions unix/src/machine_stats/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,12 @@ def storage_used_gb(facts):


def cpu_count(facts):
"""Return the number of CPUs"""
return max(
[
int(facts.get("ansible_processor_count", 0)),
int(facts.get("ansible_processor_vcpus", 0)),
]
)
"""Return the number of CPU cores"""
return int(facts.get("ansible_processor_count", 0))

def cpu_logical_processors(facts):
"""Return the number of CPU logical processors."""
return int(facts.get("ansible_processor_vcpus", 0))


def cpu_name(proc):
Expand Down Expand Up @@ -200,8 +199,16 @@ def update_results(self, host, data: dict):

if host not in self._total_results:
self._total_results[host] = data
else:
return

# Ensure we append any custom fields, rather than overwriting them
if 'custom_fields' in data and 'custom_fields' in self._total_results[host]:
combined_custom_fields = {**self._total_results[host]['custom_fields'], **data['custom_fields']}
data['custom_fields'].update(combined_custom_fields)
self._total_results[host].update(data)
return

self._total_results[host].update(data)

def v2_runner_on_ok(self, result):
self._plugins.ok_callback(self, result)
Expand All @@ -222,6 +229,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": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to get this result in my testing, I believe this custom_fields key/value is being overwritten here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird; thanks for the deep dive into this.

"CPU_LogicalProcessors": cpu_logical_processors(facts)
Copy link
Contributor

@SamDesmondKing SamDesmondKing Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: how would you feel about making this a default server field?

},
"operating_system": facts["ansible_distribution"],
"operating_system_version": facts["ansible_distribution_version"],
"cpu_name": cpu_name(facts["ansible_processor"]),
Expand Down
3 changes: 2 additions & 1 deletion windows/server_stats.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@SamDesmondKing SamDesmondKing Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, I was just following the naming conventions I established, but I agree. We should normalize on snake_case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

CPU_SocketDesignation = $cpu.SocketDesignation
TotalVisible_Memory_GB = $OSTotalVisibleMemory
TotalVirtual_Memory_GB = $OSTotalVirtualMemory
Expand Down
Loading