Skip to content

Add DiskInfo to HeartBeat #536

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

Merged
jeschu1 merged 1 commit into from
Nov 29, 2018
Merged

Add DiskInfo to HeartBeat #536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion GVFS/GVFS.Common/GVFSPlatform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public static void Register(GVFSPlatform platform)

public abstract InProcEventListener CreateTelemetryListenerIfEnabled(string providerName, string enlistmentId, string mountId);

public abstract Dictionary<string, string> GetPhysicalDiskInfo(string path);
public abstract Dictionary<string, string> GetPhysicalDiskInfo(string path, bool sizeStatsOnly);

public abstract bool IsConsoleOutputRedirectedToFile();
public abstract bool TryGetGVFSEnlistmentRoot(string directory, out string enlistmentRoot, out string errorMessage);
Expand Down
2 changes: 1 addition & 1 deletion GVFS/GVFS.Platform.Mac/MacPlatform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public override string GetOSVersionInformation()
return string.IsNullOrWhiteSpace(result.Output) ? result.Errors : result.Output;
}

public override Dictionary<string, string> GetPhysicalDiskInfo(string path)
public override Dictionary<string, string> GetPhysicalDiskInfo(string path, bool sizeStatsOnly)
{
// TODO(Mac): Collect disk information
Dictionary<string, string> result = new Dictionary<string, string>();
Expand Down
91 changes: 53 additions & 38 deletions GVFS/GVFS.Platform.Windows/WindowsPhysicalDiskInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public class WindowsPhysicalDiskInfo
/// the given pathname. For example, whether the drive is an SSD or HDD.
/// </summary>
/// <returns>A dictionary of platform-specific keywords and values.</returns>
public static Dictionary<string, string> GetPhysicalDiskInfo(string path)
public static Dictionary<string, string> GetPhysicalDiskInfo(string path, bool sizeStatsOnly)
{
// Use the WMI APIs to get details about the physical disk associated with the given path.
// Some of these fields are avilable using normal classes, such as System.IO.DriveInfo:
Expand Down Expand Up @@ -96,46 +96,14 @@ public static Dictionary<string, string> GetPhysicalDiskInfo(string path)
ManagementScope scope = new ManagementScope(@"\\.\root\microsoft\windows\storage");
scope.Connect();

string queryVolumeString = $"SELECT DriveType,FileSystem,FileSystemLabel,Size,SizeRemaining FROM MSFT_Volume WHERE DriveLetter=\"{driveLetter}\"";
ManagementBaseObject mbo = GetFirstRecord(scope, queryVolumeString);
if (mbo != null)
{
result.Add("VolumeDriveType", GetMapValue(MapDriveType, FetchValue(mbo, "DriveType")));
result.Add("VolumeFileSystem", FetchValue(mbo, "FileSystem"));
result.Add("VolumeFileSystemLabel", FetchValue(mbo, "FileSystemLabel"));
result.Add("VolumeSize", FetchValue(mbo, "Size"));
result.Add("VolumeSizeRemaining", FetchValue(mbo, "SizeRemaining"));
}
DiskSizeStatistics(scope, driveLetter, ref result);

string queryPartitionString = $"SELECT DiskNumber FROM MSFT_Partition WHERE DriveLetter=\"{driveLetter}\"";
mbo = GetFirstRecord(scope, queryPartitionString);
if (mbo != null)
if (sizeStatsOnly)
Copy link
Copy Markdown
Contributor

@sanoursa sanoursa Nov 28, 2018

Choose a reason for hiding this comment

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

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.

{
string diskNumber = FetchValue(mbo, "DiskNumber");
result.Add("DiskNumber", diskNumber);

if (diskNumber.Length > 0)
{
string queryDiskString = $"SELECT Model,IsBoot,IsSystem,SerialNumber FROM MSFT_Disk WHERE Number=\"{diskNumber}\"";
mbo = GetFirstRecord(scope, queryDiskString);
if (mbo != null)
{
result.Add("DiskModel", FetchValue(mbo, "Model"));
result.Add("DiskIsSystem", FetchValue(mbo, "IsSystem"));
result.Add("DiskIsBoot", FetchValue(mbo, "IsBoot"));
result.Add("DiskSerialNumber", FetchValue(mbo, "SerialNumber"));
}

string queryPhysicalDiskString = $"SELECT MediaType,BusType,SpindleSpeed FROM MSFT_PhysicalDisk WHERE DeviceId=\"{diskNumber}\"";
mbo = GetFirstRecord(scope, queryPhysicalDiskString);
if (mbo != null)
{
result.Add("PhysicalMediaType", GetMapValue(MapMediaType, FetchValue(mbo, "MediaType")));
result.Add("PhysicalBusType", GetMapValue(MapBusType, FetchValue(mbo, "BusType")));
result.Add("PhysicalSpindleSpeed", FetchValue(mbo, "SpindleSpeed"));
}
}
return result;
}

DiskTypeInfo(scope, driveLetter, ref result);
}
catch (Exception e)
{
Expand All @@ -145,6 +113,53 @@ public static Dictionary<string, string> GetPhysicalDiskInfo(string path)
return result;
}

private static void DiskSizeStatistics(ManagementScope scope, char driveLetter, ref Dictionary<string, string> result)
{
string queryVolumeString = $"SELECT DriveType,FileSystem,FileSystemLabel,Size,SizeRemaining FROM MSFT_Volume WHERE DriveLetter=\"{driveLetter}\"";
ManagementBaseObject mbo = GetFirstRecord(scope, queryVolumeString);
if (mbo != null)
{
result.Add("VolumeDriveType", GetMapValue(MapDriveType, FetchValue(mbo, "DriveType")));
result.Add("VolumeFileSystem", FetchValue(mbo, "FileSystem"));
result.Add("VolumeFileSystemLabel", FetchValue(mbo, "FileSystemLabel"));
result.Add("VolumeSize", FetchValue(mbo, "Size"));
result.Add("VolumeSizeRemaining", FetchValue(mbo, "SizeRemaining"));
}
}

private static void DiskTypeInfo(ManagementScope scope, char driveLetter, ref Dictionary<string, string> result)
{
string queryPartitionString = $"SELECT DiskNumber FROM MSFT_Partition WHERE DriveLetter=\"{driveLetter}\"";
ManagementBaseObject mbo = GetFirstRecord(scope, queryPartitionString);
if (mbo != null)
{
string diskNumber = FetchValue(mbo, "DiskNumber");
result.Add("DiskNumber", diskNumber);

if (diskNumber.Length > 0)
{
string queryDiskString = $"SELECT Model,IsBoot,IsSystem,SerialNumber FROM MSFT_Disk WHERE Number=\"{diskNumber}\"";
mbo = GetFirstRecord(scope, queryDiskString);
if (mbo != null)
{
result.Add("DiskModel", FetchValue(mbo, "Model"));
result.Add("DiskIsSystem", FetchValue(mbo, "IsSystem"));
result.Add("DiskIsBoot", FetchValue(mbo, "IsBoot"));
result.Add("DiskSerialNumber", FetchValue(mbo, "SerialNumber"));
}

string queryPhysicalDiskString = $"SELECT MediaType,BusType,SpindleSpeed FROM MSFT_PhysicalDisk WHERE DeviceId=\"{diskNumber}\"";
mbo = GetFirstRecord(scope, queryPhysicalDiskString);
if (mbo != null)
{
result.Add("PhysicalMediaType", GetMapValue(MapMediaType, FetchValue(mbo, "MediaType")));
result.Add("PhysicalBusType", GetMapValue(MapBusType, FetchValue(mbo, "BusType")));
result.Add("PhysicalSpindleSpeed", FetchValue(mbo, "SpindleSpeed"));
}
}
}
}

private static string FetchValue(ManagementBaseObject mbo, string key)
{
return (mbo[key] != null) ? mbo[key].ToString().Trim() : string.Empty;
Expand Down
2 changes: 1 addition & 1 deletion GVFS/GVFS.Platform.Windows/WindowsPlatform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ public override string GetCurrentUser()
return identity.User.Value;
}

public override Dictionary<string, string> GetPhysicalDiskInfo(string path) => WindowsPhysicalDiskInfo.GetPhysicalDiskInfo(path);
public override Dictionary<string, string> GetPhysicalDiskInfo(string path, bool sizeStatsOnly) => WindowsPhysicalDiskInfo.GetPhysicalDiskInfo(path, sizeStatsOnly);

public override bool IsConsoleOutputRedirectedToFile()
{
Expand Down
4 changes: 2 additions & 2 deletions GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ public override string GetOSVersionInformation()
throw new NotSupportedException();
}

public override Dictionary<string, string> GetPhysicalDiskInfo(string path)
public override Dictionary<string, string> GetPhysicalDiskInfo(string path, bool sizeStatsOnly)
{
throw new NotSupportedException();
return new Dictionary<string, string>();
}

public override void InitializeEnlistmentACLs(string enlistmentPath)
Expand Down
6 changes: 4 additions & 2 deletions GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,13 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelWToInformationalWhenPlaceho
eventLevel.ShouldEqual(EventLevel.Informational);

// "ModifiedPathsCount" should be 1 because ".gitattributes" is always present
metadata.Count.ShouldEqual(5);
metadata.Count.ShouldEqual(6);
metadata.ShouldContain("ProcessName1", "GVFS.UnitTests.exe");
metadata.ShouldContain("ProcessCount1", 1);
metadata.ShouldContain("ModifiedPathsCount", 1);
metadata.ShouldContain("PlaceholderCount", 1);
metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId);
metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue();

// Create more placeholders
fileSystemCallbacks.OnPlaceholderFileCreated("test.txt", "2222233333444445555566666777778888899999", "GVFS.UnitTests.exe2");
Expand All @@ -156,14 +157,15 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelWToInformationalWhenPlaceho
metadata = fileSystemCallbacks.GetMetadataForHeartBeat(ref eventLevel);
eventLevel.ShouldEqual(EventLevel.Informational);

metadata.Count.ShouldEqual(5);
metadata.Count.ShouldEqual(6);

// Only processes that have created placeholders since the last heartbeat should be named
metadata.ShouldContain("ProcessName1", "GVFS.UnitTests.exe2");
metadata.ShouldContain("ProcessCount1", 2);
metadata.ShouldContain("ModifiedPathsCount", 1);
metadata.ShouldContain("PlaceholderCount", 3);
metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId);
metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue();
}
}

Expand Down
5 changes: 5 additions & 0 deletions GVFS/GVFS.Virtualization/FileSystemCallbacks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,11 @@ public EventMetadata GetMetadataForHeartBeat(ref EventLevel eventLevel)
}

metadata.Add(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId);
metadata.Add(
"PhysicalDiskInfo",
GVFSPlatform.Instance.GetPhysicalDiskInfo(
this.context.Enlistment.WorkingDirectoryRoot,
sizeStatsOnly: true));
Copy link
Copy Markdown
Member

@wilbaker wilbaker Nov 28, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@wilbaker wilbaker Nov 28, 2018

Choose a reason for hiding this comment

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

(I don't think you need to block the PR on this, as the current changes are an improvement)

Copy link
Copy Markdown
Member Author

@jeschu1 jeschu1 Nov 28, 2018

Choose a reason for hiding this comment

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

@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!


return metadata;
}
Expand Down
2 changes: 1 addition & 1 deletion GVFS/GVFS/CommandLine/GVFSVerb.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ protected void LogEnlistmentInfoAndSetConfigValues(ITracer tracer, GitProcess gi
metadata.Add(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId);
metadata.Add(nameof(mountId), mountId);
metadata.Add("Enlistment", enlistment);
metadata.Add("PhysicalDiskInfo", GVFSPlatform.Instance.GetPhysicalDiskInfo(enlistment.WorkingDirectoryRoot));
metadata.Add("PhysicalDiskInfo", GVFSPlatform.Instance.GetPhysicalDiskInfo(enlistment.WorkingDirectoryRoot, sizeStatsOnly: false));
tracer.RelatedEvent(EventLevel.Informational, "EnlistmentInfo", metadata, Keywords.Telemetry);

GitProcess.Result configResult = git.SetInLocalConfig(GVFSConstants.GitConfig.EnlistmentId, RepoMetadata.Instance.EnlistmentId, replaceAll: true);
Expand Down