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.
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
pull request on adding missing statistic under solaris/illumos #1381
Conversation
1. the disk io statistic data as: nread, nwritten, reads, writes, rtime, wtime; 2. the free memory under global zone; 3. the net io statistic data as: rbytes64, ipackets64, idrops64, ierrors, obytes64, opackets64, odrops64, oerrors. The new feature branch adds the above missing statistic data based on the psutil project (https://psutil.readthedocs.io/), it has been tested under solaris ( Oracle Solaris 11.4 X86) and illumos (OmniOS v11 r151044).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! Unfortunately, I can not confirm it works on solaris/illumos, but it looks good for me, except one comment.
mem/mem_solaris.go
Outdated
@@ -34,6 +35,13 @@ func VirtualMemoryWithContext(ctx context.Context) (*VirtualMemoryStat, error) { | |||
return nil, err | |||
} | |||
result.Total = cap | |||
freemem, err := globalZoneFreeMemory() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about to pass ctx
instead of creating a new context in this function?
freemem, err := globalZoneFreeMemory() | |
freemem, err := globalZoneFreeMemory(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Shirou,
Thank for your review and comment; I have made the suggested change and submitted the commit.
For the tested OS environments, below is what I did for the Solaris and illumos environments:
- For the Solaris OS test, I have created the Solaris instance from the Oracle Solaris 11.4 image provided by the Oracle Cloud marketplace at https://cloudmarketplace.oracle.com/marketplace/en_US/listing/61750333 (OCI allows to create free Solaris instances with the public image);
- For the illumos OS test, I used the omniOS-r151044 (recommended by the illumos.org) from https://omnios.org/download.html to create the virtual machine and do the test.
For both environments, I tested with the go1.15 illumos/amd64 (and go1.13 illumos/amd64) and tests result look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks a lot!!
Thank, Shirou! Will contribute the other missing features under Solaris/Illumos if needed. |
The current codes miss below statistic data under solaris/illumos:
The new feature branch adds the above missing statistic data based on the psutil project (https://psutil.readthedocs.io/), it has been tested under solaris ( Oracle Solaris 11.4 X86) and illumos (OmniOS v11 r151044).