Closed MichalStrehovsky closed 3 months ago
Tagging subscribers to this area: @tommcdon See info in area-owners.md if you want to be subscribed.
Author: | MichalStrehovsky |
---|---|
Assignees: | - |
Labels: | `api-suggestion`, `area-System.Diagnostics` |
Milestone: | - |
How would this work with multi-cast delegates (of which events take advantage of)?
using System;
using System.Diagnostics;
Action test = TestA;
Console.WriteLine(test.GetDiagnosticMethodInfo()?.Name); // "TestA"
test += TestB;
Console.WriteLine(test.GetDiagnosticMethodInfo()?.Name); // ???
static void TestA() { }
static void TestB() { }
Should the GetDiagnosticMethodInfo
APIs maybe return arrays instead?
I would expect GetDiagnosticMethodInfo
to behave just like Delegate.Method
behaves today, except that it returns non-reflectable diagnostic-only information.
multi-cast delegates
Existing and future API can be used for decomposing multi-cast delegates.
@mikem8361
I'll try to collect some use cases so that we have a better picture of what information is actually needed:
From this, it appears we'd also want to capture the FullName of the declaring type and FullName of the assembly the method comes from.
@vaind could you point me to what information Sentry needs?
@vaind could you point me to what information Sentry needs?
From the first look at the code, the bare minimum would be:
method.Name;
method.DeclaringType?.FullName;
We're also accessing other things, like method.DeclaringType?.Assembly.FullName
and others for managed frames but I we can live without these.
Also, file name and line number would be nice so that if people don't set up server-side symbolication (i.e. they don't upload debug images), it would still have more info. These, I guess, wouldn't add up to much in terms of binary size.
I'll keep you posted if I have more info.
One more thing related to this: currently, checking whether GetMethod()
returns null is "the best" runtime check whether it's a NativeAOT build and we rely on that in multiple places. See this issue https://github.com/dotnet/runtime/issues/94380
From the first look at the code, the bare minimum would be:
Thanks, that matched what I've seen in the other libraries. I've updated the top-post with this. Also added the assembly FullName.
Also, file name and line number would be nice so that if people don't set up server-side symbolication (i.e. they don't upload debug images), it would still have more info. These, I guess, wouldn't add up to much in terms of binary size.
We track that one in #68714.
One more thing related to this: currently, checking whether
GetMethod()
returns null is "the best" runtime check whether it's a NativeAOT build and we rely on that in multiple places. See this issue #94380
What would you use the API to detect Native AOT for? For stack traces in particular, the expectation is that the new API proposed here would be a replacement for GetMethod()
in diagnostic scenarios. The API would behave the same between AOT/JIT/trimmed/etc. apps. At that point it shouldn't matter if the code is running on native AOT or not.
What would you use the API to detect Native AOT for? For stack traces in particular, the expectation is that the new API proposed here would be a replacement for
GetMethod()
in diagnostic scenarios. The API would behave the same between AOT/JIT/trimmed/etc. apps. At that point it shouldn't matter if the code is running on native AOT or not.
- Decide whether to use Ben.Demystifier or not
Looks like Ben.Demystifier runs into bugs with native AOT. If I enable the trimming/AOT/single file analyzers on the project, it finds multiple issues. Detecting native AOT and turning it off is not the right approach because the issues are not even native AOT specific, they affect all kinds of form factors. The right approach is to just fix the bugs. Someone needs to do the exercise described here to address all the warnings. Otherwise the NuGet will not properly work in PublishSingleFile apps (some F# specific logic will not work), trimmed apps, or Mono AOT apps that were run with ILStrip enabled (none of those are native AOT).
I've done the minimal work to fix things for the C# Sample app in the repo (https://github.com/benaadams/Ben.Demystifier/pull/222). After my fix and with <PropertyGroup><IlcTrimMetadata>false</IlcTrimMetadata></PropertyGroup>
, it produces equivalent results with JIT or native AOT (modulo the line numbers tracked in #68714). IlcTrimMetadata is not documented - what it does is that it keeps metadata about everything within the program. It's an instant 40% size regression, but StackFrame.GetMethod
will work. If you'd like to discuss productizing this switch, please open a new issue. Ben.Demystifier could then just enable it in a .targets file shipped in the NuGet for any project that consumes Demystifier. It's a massive deoptimization for size, but that's the price to pay.
- The whole stack trace resolution logic - there's much more info on managed stack traces and we do steps to collect that at runtime. For NativeAOT, we mostly have to rely on offline (server-side) symbolication which requires uploading debug images in advance. On the other hand, there's also custom logic for to allow symbolicating with portable PDBs
You'll need to be more specific here so that I have better advice. Looks like the thing to detect is whether there are portable PDBs. This also looks not specific to native AOT. Non-portable PDBs happen outside native AOT too.
- Whether to include Sentry's native SDK which handles crashes and resolves native frames
This looks like a build-time decision - can this be done in MSBuild logic based on PublishAot property?
- Also, we have logic to recognize HTTP paths for Azure.Functions.Worker and use that in the performance tracing product
This looks like a general trimming issue, not an issue specific to native AOT. What you probably want is to introduce a feature switch that disables this logic if PublishTrimmed is enabled in the project (PublishTrimmed is also enabled with PublishAot). Please file a new issue if you would like to discuss feature switches.
- Decide whether to use Ben.Demystifier or not
Looks like Ben.Demystifier runs into bugs with native AOT. If I enable the trimming/AOT/single file analyzers on the project, it finds multiple issues. Detecting native AOT and turning it off is not the right approach because the issues are not even native AOT specific, they affect all kinds of form factors. The right approach is to just fix the bugs. Someone needs to do the exercise described here to address all the warnings. Otherwise the NuGet will not properly work in PublishSingleFile apps (some F# specific logic will not work), trimmed apps, or Mono AOT apps that were run with ILStrip enabled (none of those are native AOT).
I've done the minimal work to fix things for the C# Sample app in the repo (benaadams/Ben.Demystifier#222). After my fix and with
<PropertyGroup><IlcTrimMetadata>false</IlcTrimMetadata></PropertyGroup>
, it produces equivalent results with JIT or native AOT (modulo the line numbers tracked in #68714). IlcTrimMetadata is not documented - what it does is that it keeps metadata about everything within the program. It's an instant 40% size regression, butStackFrame.GetMethod
will work. If you'd like to discuss productizing this switch, please open a new issue. Ben.Demystifier could then just enable it in a .targets file shipped in the NuGet for any project that consumes Demystifier. It's a massive deoptimization for size, but that's the price to pay.
I'm not aware we're having any issues in our integration but I'll check with the team.
- The whole stack trace resolution logic - there's much more info on managed stack traces and we do steps to collect that at runtime. For NativeAOT, we mostly have to rely on offline (server-side) symbolication which requires uploading debug images in advance. On the other hand, there's also custom logic for to allow symbolicating with portable PDBs
You'll need to be more specific here so that I have better advice. Looks like the thing to detect is whether there are portable PDBs. This also looks not specific to native AOT. Non-portable PDBs happen outside native AOT too.
I don't remember the details exactly but for PortablePDBs we have some custom logic to collect runtime info that is necessary for offline symbolication. That's certainly not specific to NativeAOT, it's the opposite - we have different code paths for when the frame is recognized as non-trimmed managed frame and when it's a NativeAOT frame. That's all I was saying when you asked what we do based on it being a NativeAOT build. I'm certainly not saying there's no other way around this after the changes you're proposing. It is just what we're currently doing for .NET8
In short, it's like this:
internal SentryStackFrame? CreateFrame(IStackFrame stackFrame)
{
var frame = TryCreateManagedFrame(stackFrame);
#if NET8_0_OR_GREATER
frame ??= TryCreateNativeAOTFrame(stackFrame);
#endif
if (frame is null)
{
return null;
}
....
}
private SentryStackFrame? TryCreateManagedFrame(IStackFrame stackFrame)
{
if (stackFrame.GetMethod() is not { } method)
{
return null;
}
....
}
private SentryStackFrame? TryCreateNativeAOTFrame(IStackFrame stackFrame)
{
if (!stackFrame.HasNativeImage())
{
return null;
}
.....
}
- Whether to include Sentry's native SDK which handles crashes and resolves native frames
This looks like a build-time decision - can this be done in MSBuild logic based on PublishAot property?
We do conditional linking in buildTransitive. However, we also have a runtime check to know whether to run the c# code that uses the library. We cannot check PublishAot or PublishTrimmed because these are specific to the Sentry.dll build configuration, but instead, we need to know whether the project that is consuming the DLL is doing a NativeAOT build. At the time this was implemented I couldn't find a way to ship NativeAOT/trimmed and nontrimmed version of the same library in a single nuget package and let the build system do the right thing. I could imagine there's a way to load load the library manually in a transitive build .targets but I don't even know the scope of what would need to be taken care of so it was a no-go.
- Also, we have logic to recognize HTTP paths for Azure.Functions.Worker and use that in the performance tracing product
This looks like a general trimming issue, not an issue specific to native AOT. What you probably want is to introduce a feature switch that disables this logic if PublishTrimmed is enabled in the project (PublishTrimmed is also enabled with PublishAot). Please file a new issue if you would like to discuss feature switches.
same as the third point - we don't know about the PublishAOT of the client app at the time we're compiling that library.
I'm not aware we're having any issues in our integration but I'll check with the team.
These will be corner case bugs - for example for an F# app with PublishSingleFile, if any of the (multiple) codepaths that do ManifestModule.Name == "FSharp.Core.dll"
is hit. The fix is to go over all the warnings in the library.
I don't remember the details exactly but for PortablePDBs we have some custom logic to collect runtime info that is necessary for offline symbolication
This is another good example of "whenever you feel you need to special case native AOT, you likely need to make a step back":
This is even suppressing the warning from the single file analyzer, incorrectly. That warning is still relevant to single file: Console.WriteLine(typeof(Program).Module.FullyQualifiedName);
with PublishSingleFile=true
will print <unknown>
so this is blindly trying to open a file named <unknown>
. There's a try/catch, so there's at least that. But no native AOT special casing is needed here. What is needed is single file special casing.
same as the third point - we don't know about the PublishAOT of the client app at the time we're compiling that library.
I assume the library is distributed through NuGet - NuGet can have MSBuild logic related to feature switches. Imagine this, but make the ItemGroup
conditional on PublishTrimmed
being true. This way you can set defaults for various features within the library depending on whether the app is trimmed (the .targets file will be consumed by any project consuming the NuGet).
Thank you. I've created a follow ups to have a look into your suggestions
@tommcdon @mikem8361 any objections to marking this API as ready for API review?
I don't have any objections.
namespace System.Diagnostics;
public sealed class DiagnosticMethodInfo
{
// Returns name of the method, same as MethodInfo.Name
public string Name { get; }
// Returns FullName of the declaring type. Same as MethodInfo.DeclaringType.FullName
public string DeclaringTypeName { get; }
// Returns full name of the declaring assembly, same as MethodInfo.Module.Assembly.FullName
public string DeclaringAssemblyName { get; }
// These return a nullable result because if the app is aggressively trimmed (e.g. `StackTraceSupport` https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options?pivots=dotnet-6-0#trimming-framework-library-features feature switch is disabled), we might return null
public static DiagnosticMethodInfo? Create(Delegate delegate);
public static DiagnosticMethodInfo? Create(StackFrame frame);
}
The API now merged with #103220.
I'll keep this open to track a couple leftovers:
<StackTraceSupport>false</>
can disable this on non-NativeAOT runtimes.StackFrame.GetMethod()
to advertise this API as a trim-friendly alternativeDelegate.Method
docsThis is now done.
Background and motivation
There are diagnostic scenarios for which people currently use reflection:
StackFrame.GetMethod
)Delegate.Method
)These APIs return a fully functional
MethodBase
instance.This poses a problem for trimming because the
MethodBase
returned from these APIs needs to be fully functional (up to and including providing the ability to invoke the method). This means theMethodBase
needs to have intact parameter names, custom attributes including all the nullable annotation cruft, etc.Trimming currently approaches this two ways:
StackFrame.GetMethod
is marked asRequiresUnreferencedCode
. Accessing it in a trimmed or AOT'd app produces a build time warning. The property may or may not return a null or an incomplete/damagedMethodInfo
. It will mostly return a damaged/incompleteMethodInfo
in a trimmed app (e.g. missing parameter names), and mostly return null in an AOT'd app.Delegate.Method
is trim safe. Trimming and AOT ensure all delegate targets are considered reflection visible.Solution 1 currently blocks some customer scenarios (#92869) that people are not able to do well in trimmed apps. Solution 2 leaves size savings on the table.
Fundamentally, many of the use cases of
StackFrame.GetMethod
/Delegate.Method
only need names of things, they don't actually need the wholeMethodBase
. For example, the use cases in #92869, or our own use cases within the framework like https://github.com/dotnet/runtime/blob/396edb21dbca7d14e70585551da74869181a2df7/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs#L1687The proposal is to introduce a new API that would be trim safe and produce just the name part, using a more limited portion of the metadata.
API Proposal
API Usage
Alternative Designs
Don't trim any metadata from methods that are statically reachable so that StackFrame.Method always works. This is an instant 20% size regression for AOT (where we don't just trim names of parameters, but also other things), probably a little less for IL level trimming (where we currently only trim parameter names).
Risks
No response