[API Proposal]: Diagnostic method information

[MichalStrehovsky]

Background and motivation

There are diagnostic scenarios for which people currently use reflection:

• Obtaining information about a method on a stack frame (StackFrame.GetMethod)
• Obtaining information about a target of a delegate (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 the MethodBase needs to have intact parameter names, custom attributes including all the nullable annotation cruft, etc.

Trimming currently approaches this two ways:

1. StackFrame.GetMethod is marked as RequiresUnreferencedCode. 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/damaged MethodInfo. It will mostly return a damaged/incomplete MethodInfo in a trimmed app (e.g. missing parameter names), and mostly return null in an AOT'd app.
2. 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 whole MethodBase. For example, the use cases in #92869, or our own use cases within the framework like

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs

Line 1687 in 396edb2

TplEventSource.Log.TraceOperationBegin(this.Id, "Task: " + m_action.Method.Name, 0);

The 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

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; }
}

public static void DiagnosticExtensions
{
    // 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? GetDiagnosticMethodInfo(this Delegate del);
    public static DiagnosticMethodInfo? GetDiagnosticMethodInfo(this StackFrame frame);
}

API Usage

Action a = Method;

Console.WriteLine($"Delegate points to {a.GetDiagnosticMethodInfo()?.Name ?? "Unknown method"});

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

[ghost]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

There are diagnostic scenarios for which people currently use reflection:

• Obtaining information about a method on a stack frame (StackFrame.GetMethod)
• Obtaining information about a target of a delegate (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).

Trimming currently approaches this two ways:

1. StackFrame.GetMethod is marked as RequiresUnreferencedCode. Accessing it in a trimmed app produces a build time warning. The property may or may not return a null or an incomplete/damaged MethodInfo at runtime in a trimmed app. It will mostly return null in an AOT'd app.
2. Delegate.Method is trim safe. We 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 whole MethodBase, with custom attributes, parameter names, etc. For example, the use cases in #92869, or our own use cases within the framework like

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs

Line 1687 in 396edb2

TplEventSource.Log.TraceOperationBegin(this.Id, "Task: " + m_action.Method.Name, 0);

The 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

namespace System.Diagnostics;

public sealed class DiagnosticMethodInfo
{
    // Returns name of the method
    public string Name { get; }

    // We can expose things like owning type name, or names of types in the signature,
    // but I'm starting off with the minimal thing.

    // Returns similar string to StackFrame.ToString, but with no parameter names
    public override string ToString();
}

public static void DiagnosticExtensions
{
    // These return a nullable result because if StackTraceSupport is disabled, we would return null
    // https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options?pivots=dotnet-6-0#trimming-framework-library-features
    public static DiagnosticMethodInfo? GetDiagnosticMethodInfo(this Delegate del);
    public static DiagnosticMethodInfo? GetDiagnosticMethodInfo(this StackFrame frame);
}

API Usage

Action a = Method;

Console.WriteLine($"Delegate points to {a.GetDiagnosticMethodInfo()?.Name ?? "Unknown method"});

Alternative Designs

No response

Risks

No response

Author:	MichalStrehovsky
Assignees:	-
Labels:	api-suggestion, area-System.Diagnostics
Milestone:	-

[colejohnson66]

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() { }

[MichalPetryka]

Should the GetDiagnosticMethodInfo APIs maybe return arrays instead?

[jkotas]

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.

[tommcdon]

@mikem8361

[MichalStrehovsky]

I'll try to collect some use cases so that we have a better picture of what information is actually needed:

• https://github.com/microsoft/appcenter-sdk-dotnet/blob/43de9063bee90c9329a8197d586033d08fbad86b/SDK/AppCenterCrashes/Microsoft.AppCenter.Crashes.Apple/Crashes.cs#L184-L193
• https://github.com/Azure/azure-sdk-for-net/blob/d49223f469c664319994ef18354c6e36e4325aca/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/StackFrame.cs#L28-L36

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]

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

[vaind]

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

[MichalStrehovsky]

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.

[vaind]

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.

1. Decide whether to use Ben.Demystifier or not
2. 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
3. Whether to include Sentry's native SDK which handles crashes and resolves native frames
4. Also, we have logic to recognize HTTP paths for Azure.Functions.Worker and use that in the performance tracing product

[MichalStrehovsky]

1. 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, 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.

2. 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.

3. 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?

4. 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.

[vaind]

1. 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, 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.

I'm not aware we're having any issues in our integration but I'll check with the team.

2. 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;
        }
        .....
    }

3. 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.

4. 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.

[MichalStrehovsky]

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":

https://github.com/getsentry/sentry-dotnet/blob/d1e5efcdf3af763ad49f11cd2426cc14a315a901/src/Sentry/Internal/DebugStackTrace.cs#L511-L536

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).

[vaind]

Thank you. I've created a follow ups to have a look into your suggestions

• Check DebugStackTrace conditional execution/suppressions getsentry/sentry-dotnet#3223
• Investigate using custom builds for AOT compiled or trimmed apps getsentry/sentry-dotnet#3222

[MichalStrehovsky]

@tommcdon @mikem8361 any objections to marking this API as ready for API review?

[mikem8361]

I don't have any objections.

[bartonjs]

• We changed the extension methods to static Create methods on the type itself.
• We discussed System.Diagnostics vs System.Reflection'
• We discussed DiagnosticMethodInfo vs MethodDiagnosticInfo and decided DiagnosticMethodInfo was fine
• We asked if DeclaringTypeName should use TypeName vs string, and the answer was "no".

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);
}

[MichalStrehovsky]

The API now merged with #103220.

I'll keep this open to track a couple leftovers:

• SDK repo change so that <StackTraceSupport>false</> can disable this on non-NativeAOT runtimes.
• Docs change to StackFrame.GetMethod() to advertise this API as a trim-friendly alternative
• Advertise this in Delegate.Method docs

[MichalStrehovsky]

This is now done.