Add DiskInfo to HeartBeat#536
Conversation
|
Two questions:
Keep in mind that "queryVolumeString" is totally Windows-specific. What you might consider doing is add a flag to |
|
@sanoursa that makes sense To answer both questions. I'll update GetPhysicalDiskInfo to have a sizeStatsOnly flag. That should solve both problems nicely. Thanks! |
jeschu1
force-pushed
the
enlistmentinfo
branch
from
5b90a08 to
ed4c646
Compare
|
@sanoursa any chance you may be able to give this a final review. If your too busy let me know and I'll ask someone else. Thank You! |
sanoursa
left a comment
sanoursa
left a comment
There was a problem hiding this comment.
Just a minor code cleanup comment, this looks good otherwise
| result.Add("VolumeSizeRemaining", FetchValue(mbo, "SizeRemaining")); | ||
| } | ||
|
|
||
| if (sizeStatsOnly) |
There was a problem hiding this comment.
Not all your doing, but this method is quite long, does two separate things, and now with an exit in the middle, can be pretty hard to follow. I would split it into two new methods, so that this top level method can call the first, then early exit if needed, and then call the second.
| "PhysicalDiskInfo", | ||
| GVFSPlatform.Instance.GetPhysicalDiskInfo( | ||
| this.context.Enlistment.WorkingDirectoryRoot, | ||
| sizeStatsOnly: true)); |
There was a problem hiding this comment.
This might be overkill for this PR, but should we set eventLevel to Informational if the free space is below some threshold (so that the heartbeat will be written to the logfile)?
If we can reliably find a user's telemetry data this would be less important.
There was a problem hiding this comment.
(I don't think you need to block the PR on this, as the current changes are an improvement)
There was a problem hiding this comment.
@wilbaker it gets a little tricky only because we're cross plat and there's no contract on whats coming back in the map exactly.
Definitely something good to think about for the future!
jeschu1
force-pushed
the
enlistmentinfo
branch
from
ed4c646 to
77c27aa
Compare
jrbriggs
added
the
affects: live-site
4 participants
This logs disk info on heat beat:
"PhysicalDiskInfo":{"DriveLetter":"C","VolumeDriveType":"Fixed","VolumeFileSystem":"NTFS","VolumeFileSystemLabel":"OSDisk","VolumeSize":"510928089088","VolumeSizeRemaining":"103830548480","DiskNumber":"0","DiskModel":"MTFDDAK512MBF1A","DiskIsSystem":"True","DiskIsBoot":"True","DiskSerialNumber":"XXXXXX","PhysicalMediaType":"SSD","PhysicalBusType":"SATA","PhysicalSpindleSpeed":"0"}
This should come in very handy when investigating why users run out of disk space.
@sanoursa I'm using the existing GetPhysicalDiskInfo method, which brings back more info that we need. Let me know if you think it's worth seperating queryVolumeString into it's own method and just calling that here. I'll update the failing unit test once we make a decision there.