-
Notifications
You must be signed in to change notification settings - Fork 324
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
Add Loader Snaps to DevDiagnostics's insights #3815
Conversation
tools/DevDiagnostics/DevHome.DevDiagnostics/Helpers/LoaderSnapAssistantTool.cs
Outdated
Show resolved
Hide resolved
tools/DevDiagnostics/DevHome.DevDiagnostics/Strings/en-us/Resources.resw
Show resolved
Hide resolved
tools/DevDiagnostics/Utilities/EnableLoaderSnaps/EnableLoaderSnaps.cs
Outdated
Show resolved
Hide resolved
...gnostics/DevHome.DevDiagnostics/Controls/InsightForMissingFileProcessTerminationControl.xaml
Show resolved
Hide resolved
tools/DevDiagnostics/DevHome.DevDiagnostics/Pages/InsightsPage.xaml
Outdated
Show resolved
Hide resolved
tools/DevDiagnostics/DevHome.DevDiagnostics/Helpers/LoaderSnapAssistantTool.cs
Outdated
Show resolved
Hide resolved
tools/DevDiagnostics/DevHome.DevDiagnostics/Helpers/LoaderSnapAssistantTool.cs
Outdated
Show resolved
Hide resolved
tools/DevDiagnostics/DevHome.DevDiagnostics/Controls/InsightSimpleTextControl.xaml.cs
Show resolved
Hide resolved
return false; | ||
} | ||
|
||
if ((tracingFlags & 0x4) != 0x4) |
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 opportunity for a
[Flags]
enum TracingFlags : uint
{
Foo = 0x4,
etc = 0x8,
}
Followed by
tracingFlags.HasFlag(Foo) instead of this manual stuff :) #Resolved
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.
Example on how to do this (save you loading a web page if you don't know how)
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.
Got it. I ended up defining the flags twice, as I didn't want to reference a common assembly in my elevated process. But this seems to work well.
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.
What's wrong with using common assemblies in an elevated process?
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.
Just need to worry about statics.... make sure code isn't executing unexpectedly.
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 don't see how that's ever going to work at scale. Adding a bad project reference will get overlooked 99 times out of 100 without automation.
tools/DevDiagnostics/DevHome.DevDiagnostics/Helpers/LoaderSnapAssistantTool.cs
Outdated
Show resolved
Hide resolved
tools/DevDiagnostics/DevHome.DevDiagnostics/Helpers/LoaderSnapAssistantTool.cs
Outdated
Show resolved
Hide resolved
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.
Any telemetry for the feature usage etc?
Ok In reply to: 2339275118 Refers to: tools/DevDiagnostics/TestTools/TestLoadFailure/TestLoadFailure.cpp:27 in 31b782c. [](commit_id = 31b782c, deletion_comment = False) |
Any telemetry for the feature usage etc? #Resolved In reply to: 2290951672 |
@@ -1,11 +1,10 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<Import Project="$(SolutionDir)ToolingVersions.props" /> | |||
<Import Project="$(SolutionDir)Directory.CppBuild.props" /> |
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.
It feels wrong to put a CppBuild into a csproj. What property is this using? Can it go in a general or cs-specific props file? #Resolved
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.
Good callout. I should just be including Directory.Build.props instead.
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.
Actually, that was wrong... sounds like this file is automatically included (I missed the build warning earlier). I'll just remove this line.
|
||
public LoaderSnapAssistantTool() | ||
{ | ||
_dispatcher = Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread(); |
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 know DD does something different than the rest of Dev Home where apparently it's guaranteed to be running on the UI thread more than the rest can guarantee, but are you sure you're always going to be running on the UI thread here? If you're not, this returns null and you get in trouble. #Resolved
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 added an assert here to confirm this is created on the UI thread.
|
||
internal static void Log(string eventName, LogLevel level, Guid? relatedActivityId = null) => Logger.Log(eventName, level, new UsageEventData(), relatedActivityId); | ||
} | ||
// Copyright (c) Microsoft Corporation. |
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.
It may be worth changing the files like this one where the whole file's line endings got corrected back to LF just for diff's sake. We can do a giant sweep to fix line endings separately.
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.
Summary of the pull request
This adds the ability for DevDiagnostics to monitor for processes terminating due to missing dependencies, and let the user know which files were missing.
and after the user has enabled loader snaps
References and relevant issues
Detailed description of the pull request / Additional comments
This utilizes the NT service to monitor for process terminations that exited due to missing files. From that, we rely on the user to launch an elevated exe that writes to the image file execution options (ifeo) to enable loader snaps for a particular image, and then when the process runs again, we parse that log and show the relevant information.
We listen to the kernel ETW provider to learn about process start/stops in the NT service, and we listen to the Windows Loader ETW channel in DevDiagnostics for loader snaps.
As part of this, I revamped the Insights UI to allow insights providers to create a custom control to show their content. This was needed in this scenario to show a button in addition to text, and I would imagine we'll have more scenarios in the future.
This also implements a dummy NT service, so if for whatever reason DevDiagnostics is not able to connect to the real NT service, there is a "stand in" service object instantiated in-proc to ensure DD can still run.
Validation steps performed
Used the TestLoadFailure app to trigger errors.
Testing this both via F5 deploy and package installed.
Ran through the new controls with narrator and confirmed they are accessible.
PR checklist