Use a different package to extract system information during host discovery #428
Conversation
nelsonkopliku
requested review from
fabriziosestito,
arbulu89,
stefanotorresi,
dottorblaster and
rtorrero
| infoStat, err := host.Info() | ||
| if err != nil { | ||
| log.Errorf("Error while getting host info: %s", err) | ||
| } | ||
| return infoStat.PlatformVersion |
There was a problem hiding this comment.
What if we get an error from host.Info()? We'll still have PlatformVersion inside infoStat?
There was a problem hiding this comment.
It will get its null value, in this case an empty string. I guess 😅
| v, err := mem.VirtualMemory() | ||
| if err != nil { | ||
| log.Errorf("Error while getting memory info: %s", err) | ||
| } | ||
| return int(v.Total) / 1024 / 1024 |
There was a problem hiding this comment.
Same here, we got an empty struct if err != nil? I really don't know what we get
There was a problem hiding this comment.
It will get the int zero value 0.
There was a problem hiding this comment.
I think that's risky, because we would be reporting 0 as metric, which is false.
We should handle the error cases and set the this discovery with a failed state, so the server knows that something wrong happened.
| return infoStat.PlatformVersion | ||
| } | ||
|
|
||
| func getTotalMemoryMB() int { |
There was a problem hiding this comment.
Is there any reason why this function is private? (starts with small letter)
There was a problem hiding this comment.
No particular reason. No one is using it, except for this discovery.
When neede It could very well be public, or even an Interface to a service.
I kept it really simple.
Is that good enough for now?
There was a problem hiding this comment.
I was just surprised because the other Get... function are using capital letter. I just noticed the difference and lack of consistency. Nothing else hehe
There was a problem hiding this comment.
Ah, my bad, now I spotted.
They were supposed to be all private actually. Now I see I have all public except for one.
A leftover. Gonna make all private.
Thanks 🙏
| v, err := mem.VirtualMemory() | ||
| if err != nil { | ||
| log.Errorf("Error while getting memory info: %s", err) | ||
| } | ||
| return int(v.Total) / 1024 / 1024 |
There was a problem hiding this comment.
I think that's risky, because we would be reporting 0 as metric, which is false.
We should handle the error cases and set the this discovery with a failed state, so the server knows that something wrong happened.
nelsonkopliku
force-pushed
the
fix_host_discovery
branch
from
6671222
to
5083508
Compare
Thanks @arbulu89. I'd agree with you. Yet I am not confident on the way to go in that regard. |
|
After a bit of thought, I think this was the right decision. Tracking core counts of VMs also makes sense, and in the future we could add checks to report if the agent is running behind a hypervisor. Good job! (just left a comment at some small detail I think we should change before merging) |
nelsonkopliku
force-pushed
the
fix_host_discovery
branch
from
74599b8
to
98acf0d
Compare
|
It looks good to me now, thanks :-)! |
On demo we are projecting the
HostTelemetry, but the current package is not able to properly extract CPU count.This PR replaces it with https://github.com/shirou/gopsutil.
I have doubts on the physical/logical cpu stuff. Please let know if that would work