-
Notifications
You must be signed in to change notification settings - Fork 113
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
Implement collection rule filters. #882
Merged
jander-msft
merged 5 commits into
dotnet:main
from
jander-msft:dev/jander/collection-rule-filter
Sep 21, 2021
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
cc21807
Implement collection rule filters.
jander-msft 48fce90
PR Feedback
jander-msft d0891b2
Disable CommandLineFilterMatchTest on .NET 5 non-Windows
jander-msft d557f53
Expand exclusion to .NET 5+ on non-Windows
jander-msft 2fc9465
Move IProcessInfo creation to ApplyRules method.
jander-msft File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
131 changes: 131 additions & 0 deletions
131
src/Microsoft.Diagnostics.Monitoring.WebApi/ProcessInfoImpl.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using Microsoft.Diagnostics.Monitoring.EventPipe; | ||
using Microsoft.Diagnostics.NETCore.Client; | ||
using System; | ||
using System.IO; | ||
using System.Runtime.InteropServices; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
namespace Microsoft.Diagnostics.Monitoring.WebApi | ||
{ | ||
/// Named "ProcessInfoImpl" to disambiguate from Microsoft.Diagnostics.NETCore.client ProcessInfo | ||
/// class returned from issuing GetProcessInfo command on diagnostic pipe. | ||
internal sealed class ProcessInfoImpl : IProcessInfo | ||
{ | ||
// The amount of time to wait before cancelling get additional process information (e.g. getting | ||
// the process command line if the IEndpointInfo doesn't provide it). | ||
public static readonly TimeSpan ExtendedProcessInfoTimeout = TimeSpan.FromMilliseconds(1000); | ||
|
||
// String returned for a process field when its value could not be retrieved. This is the same | ||
// value that is returned by the runtime when it could not determine the value for each of those fields. | ||
private static readonly string ProcessFieldUnknownValue = "unknown"; | ||
|
||
// The value of the operating system field of the ProcessInfo result when the target process is running | ||
// on a Windows operating system. | ||
private const string ProcessOperatingSystemWindowsValue = "windows"; | ||
|
||
public ProcessInfoImpl( | ||
IEndpointInfo endpointInfo, | ||
string commandLine, | ||
string processName) | ||
{ | ||
EndpointInfo = endpointInfo; | ||
|
||
// The GetProcessInfo command will return "unknown" for values for which it does | ||
// not know the value, such as operating system and process architecture if the | ||
// process is running on one that is not predefined. Mimic the same behavior here | ||
// when the extra process information was not provided. | ||
CommandLine = commandLine ?? ProcessFieldUnknownValue; | ||
ProcessName = processName ?? ProcessFieldUnknownValue; | ||
} | ||
|
||
public static async Task<IProcessInfo> FromEndpointInfoAsync(IEndpointInfo endpointInfo) | ||
{ | ||
using CancellationTokenSource extendedInfoCancellation = new(ExtendedProcessInfoTimeout); | ||
return await FromEndpointInfoAsync(endpointInfo, extendedInfoCancellation.Token); | ||
} | ||
|
||
// Creates an IProcessInfo object from the IEndpointInfo. Attempts to get the command line using event pipe | ||
// if the endpoint information doesn't provide it. The cancelation token can be used to timebox this fallback | ||
// mechanism. | ||
public static async Task<IProcessInfo> FromEndpointInfoAsync(IEndpointInfo endpointInfo, CancellationToken extendedInfoCancellationToken) | ||
{ | ||
if (null == endpointInfo) | ||
{ | ||
throw new ArgumentNullException(nameof(endpointInfo)); | ||
} | ||
|
||
DiagnosticsClient client = new(endpointInfo.Endpoint); | ||
|
||
string commandLine = endpointInfo.CommandLine; | ||
if (string.IsNullOrEmpty(commandLine)) | ||
{ | ||
try | ||
{ | ||
EventProcessInfoPipelineSettings infoSettings = new() | ||
{ | ||
Duration = Timeout.InfiniteTimeSpan, | ||
}; | ||
|
||
await using EventProcessInfoPipeline pipeline = new(client, infoSettings, | ||
(cmdLine, token) => { commandLine = cmdLine; return Task.CompletedTask; }); | ||
|
||
await pipeline.RunAsync(extendedInfoCancellationToken); | ||
} | ||
catch | ||
{ | ||
} | ||
} | ||
|
||
string processName = null; | ||
if (!string.IsNullOrEmpty(commandLine)) | ||
{ | ||
// Get the process name from the command line | ||
bool isWindowsProcess = false; | ||
if (string.IsNullOrEmpty(endpointInfo.OperatingSystem)) | ||
{ | ||
// If operating system is null, the process is likely .NET Core 3.1 (which doesn't have the GetProcessInfo command). | ||
// Since the underlying diagnostic communication channel used by the .NET runtime requires that the diagnostic process | ||
// must be running on the same type of operating system as the target process (e.g. dotnet-monitor must be running on Windows | ||
// if the target process is running on Windows), then checking the local operating system should be a sufficient heuristic | ||
// to determine the operating system of the target process. | ||
isWindowsProcess = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
} | ||
else | ||
{ | ||
isWindowsProcess = ProcessOperatingSystemWindowsValue.Equals(endpointInfo.OperatingSystem, StringComparison.OrdinalIgnoreCase); | ||
} | ||
|
||
string processPath = CommandLineHelper.ExtractExecutablePath(commandLine, isWindowsProcess); | ||
if (!string.IsNullOrEmpty(processPath)) | ||
{ | ||
processName = Path.GetFileName(processPath); | ||
if (isWindowsProcess) | ||
{ | ||
// Remove the extension on Windows to match the behavior of Process.ProcessName | ||
processName = Path.GetFileNameWithoutExtension(processName); | ||
} | ||
} | ||
} | ||
|
||
return new ProcessInfoImpl( | ||
endpointInfo, | ||
commandLine, | ||
processName); | ||
} | ||
|
||
public IEndpointInfo EndpointInfo { get; } | ||
|
||
public string CommandLine { get; } | ||
|
||
public string OperatingSystem => EndpointInfo.OperatingSystem ?? ProcessFieldUnknownValue; | ||
|
||
public string ProcessArchitecture => EndpointInfo.ProcessArchitecture ?? ProcessFieldUnknownValue; | ||
|
||
public string ProcessName { get; } | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Looks like that if a .NET 5.0 runtime is suspended and a GetProcessInfo command is issued over the diagnostic pipe, then the CommandLine only contains the process name on Unix operating systems, e.g.:
/__w/1/s/.dotnet/dotnet
If the GetProcessInfo command is issued AFTER the runtime is resumed, then it returns the property command line, e.g.:
/__w/1/s/.dotnet/dotnet /__w/1/s/artifacts/bin/Microsoft.Diagnostics.Monitoring.UnitTestApp/Release/net5.0/Microsoft.Diagnostics.Monitoring.UnitTestApp.dll AsyncWait
From what I can tell, this does not repro on .NET 6 when running these newly added tests.
@josalem, are you aware of any issues in regard to this for .NET 5? This is an issue for dotnet-monitor because command line is one of the usable criteria for filtering collection rules.
A potential workaround is to force run the EventProcessInfoPipeline when we know dotnet-monitor hasn't resumed the runtime yet and it is running on .NET 5.
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.
That's odd. No, I'm not aware of an issue like this. This is only happening while the .net5 app is suspended?
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.
Correct. .NET 5, non-Windows, suspended. This build demonstrates the symptoms: https://dev.azure.com/dnceng/public/_build/results?buildId=1372672&view=results
I can produce another build with more verbosing logging that demonstrates the issue precisely. I can also try adjusting the GetProcessInfoTests in the dotnet/diagnostics repo to demonstrate it more simply, but that's running on .NET 6 (which this does not repro), if I change them back to .NET 5.
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.
I'm disabling the affected test for .NET 5 on non-Windows and logged an issue to track and investigate this later: #885
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.
After disabling the test for .NET 5 on non-Windows, it started failing on .NET 6 for non-Windows. Also, I've updated the GetProcessInfoTests (dotnet/diagnostics#2603) in dotnet/diagnostics to check that the values before and after are consistent, which seems to be the case. So this is looking more like a dotnet-monitor bug than a runtime bug. I'm going to keep investigating this and update this thread with new info when its available.
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.
Okay, now I have nearly definitive proof that the runtime (likely in both .NET 5 and .NET 6) only reports the process name for the command line field on the ProcessInfo reponse on non-Windows platforms when the runtime is suspended. See this build, which is the same as the changes in dotnet/diagnostics#2603, except that it writes out the command line at the end of the test and intentionally fails the test. It shows that is has a value of something like
/__w/1/s/.dotnet/dotnet
. This is occurring before and shortly after resuming the runtime. So this is likely another race condition where the command line arguments aren't available until sometime in the future after resuming the runtime. This does not repro on Windows.