Add DiagnosticSource.Write<T> API to assist with trimming

[eerhardt]

Background and Motivation

The DiagnosticSource.Write API is not trim-friendly because the type of object being passed in cannot be statically discovered.

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSource.cs

Line 34 in 82ca681

public abstract void Write(string name, object? value);

Internally, we have a DiagnosticSourceEventSource class which listens to events being written to a DiagnosticSource and will forward the event on to an EventSource. This functionality assumes that all the properties on the object, and any properties recursively on the Types of those properties, are available at runtime. It uses Reflection to access the property values based on FilterAndPayloadSpecs strings that are passed to the DiagnosticSourceEventSource.

In order to resolve the ILLink warnings in System.Diagnostics.DiagnosticSource, we are marking DiagnosticSource.Write with RequiresUnreferencedCode. This means any caller of this API will get ILLink warnings themselves, and will need to preserve the necessary properties with [DynamicDependency] attributes, or some other mechanism.

However, a common usage of DiagnosticSource.Write is to pass in an anonymous type, for example this is how ASP.NET uses the API:

https://github.com/dotnet/aspnetcore/blob/85a6cb07ae2e1adf5a03740f8d40333b7b8e360c/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L212-L218

        private void RecordBeginRequestDiagnostics(HttpContext httpContext, long startTimestamp)
        {
            _diagnosticListener.Write(
                DeprecatedDiagnosticsBeginRequestKey,
                new
                {
                    httpContext = httpContext,
                    timestamp = startTimestamp
                });

Since the type is anonymous, there isn't a way to add a [DynamicDependency] pointing to the type.

To resolve this issue, we should add a new overload to the Write method that is generic, so we can annotate the T type with a [DynamicallyAccessedMembers] attribute. Unfortunately, this won't make the API 100% trim compatible because recursive properties aren't supported (see dotnet/linker#1087), thus it will still need to be marked as [RequiresUnreferencedCode]. However, it will be one less set of properties callers will need to manually preserve themselves, and it will allow anonymous types to be used.

See this conversation for more background and reasoning.

Proposed API

namespace System.Diagnostics
{
    public abstract class DiagnosticSource
    {
        [RequiresUnreferencedCode]
        public abstract void Write(string name, object? value);

+       [RequiresUnreferencedCode]
+       public void Write<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(string name, T? value);

        [RequiresUnreferencedCode]
        public Activity StartActivity(Activity activity, object? args);

+       [RequiresUnreferencedCode]
+       public Activity StartActivity<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(Activity activity, T? args);

        [RequiresUnreferencedCode]
        public void StopActivity(Activity activity, object? args);

+       [RequiresUnreferencedCode]
+       public void StopActivity<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(Activity activity, T? args);
     }
}

Usage Examples

The usage is the same as the existing APIs.

            _diagnosticListener.Write(
                DeprecatedDiagnosticsBeginRequestKey,
                new
                {
                    httpContext = httpContext,
                    timestamp = startTimestamp
                });

Alternative Designs

N/A

Risks

Is it possible that caller may still bind to the API accepting object? and they won't get the DynamicallyAccessedMembers attribute applied to their types?

[ghost]

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

Issue Details

---

Background and Motivation

The DiagnosticSource.Write API is not trim-friendly because the type of object being passed in cannot be statically discovered.

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSource.cs

Line 34 in 82ca681

public abstract void Write(string name, object? value);

Internally, we have a DiagnosticSourceEventSource class which listens to events being written to a DiagnosticSource and will forward the event on to an EventSource. This functionality assumes that all the properties on the object, and any properties recursively on the Types of those properties, are available at runtime. It uses Reflection to access the property values based on FilterAndPayloadSpecs strings that are passed to the DiagnosticSourceEventSource.

In order to resolve the ILLink warnings in System.Diagnostics.DiagnosticSource, we are marking DiagnosticSource.Write with RequiresUnreferencedCode. This means any caller of this API will get ILLink warnings themselves, and will need to preserve the necessary properties with [DynamicDependency] attributes, or some other mechanism.

However, a common usage of DiagnosticSource.Write is to pass in an anonymous type, for example this is how ASP.NET uses the API:

https://github.com/dotnet/aspnetcore/blob/85a6cb07ae2e1adf5a03740f8d40333b7b8e360c/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L212-L218

        private void RecordBeginRequestDiagnostics(HttpContext httpContext, long startTimestamp)
        {
            _diagnosticListener.Write(
                DeprecatedDiagnosticsBeginRequestKey,
                new
                {
                    httpContext = httpContext,
                    timestamp = startTimestamp
                });

Since the type is anonymous, there isn't a way to add a [DynamicDependency] pointing to the type.

To resolve this issue, we should add a new overload to the Write method that is generic, so we can annotate the T type with a [DynamicallyAccessedMembers] attribute. Unfortunately, this won't make the API 100% trim compatible because recursive properties aren't supported (see dotnet/linker#1087), thus it will still need to be marked as [RequiresUnreferencedCode]. However, it will be one less set of properties callers will need to manually preserve themselves, and it will allow anonymous types to be used.

See this conversation for more background and reasoning.

Proposed API

namespace System.Diagnostics
{
    public abstract class DiagnosticSource
    {
        [RequiresUnreferencedCode]
        public abstract void Write(string name, object? value);

+       [RequiresUnreferencedCode]
+       public void Write<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(string name, T? value);

        [RequiresUnreferencedCode]
        public Activity StartActivity(Activity activity, object? args);

+       [RequiresUnreferencedCode]
+       public Activity StartActivity<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(Activity activity, T? args);

        [RequiresUnreferencedCode]
        public void StopActivity(Activity activity, object? args);

+       [RequiresUnreferencedCode]
+       public void StopActivity<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(Activity activity, T? args);
     }
}

Usage Examples

The usage is the same as the existing APIs.

            _diagnosticListener.Write(
                DeprecatedDiagnosticsBeginRequestKey,
                new
                {
                    httpContext = httpContext,
                    timestamp = startTimestamp
                });

Alternative Designs

N/A

Risks

Is it possible that caller may still bind to the API accepting object? and they won't get the DynamicallyAccessedMembers attribute applied to their types?

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

[noahfalk]

@eerhardt - I am guessing this work is not planned for 6.0 at this point? I'm changing the milestone but if this was something you plan to do you are welcome to change it further.

[eerhardt]

I am guessing this work is not planned for 6.0 at this point?

Correct. This is a new API and 6.0 has reached feature complete.

[bartonjs]

Video

These new overloads don't quite solve the problem (recursive preservation, polymorphism), but it's an improvement at low cost.

namespace System.Diagnostics
{
    public abstract class DiagnosticSource
    {
        [RequiresUnreferencedCode]
        public abstract void Write(string name, object? value);

+       [RequiresUnreferencedCode]
+       public void Write<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(string name, T? value);

        [RequiresUnreferencedCode]
        public Activity StartActivity(Activity activity, object? args);

+       [RequiresUnreferencedCode]
+       public Activity StartActivity<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(Activity activity, T? args);

        [RequiresUnreferencedCode]
        public void StopActivity(Activity activity, object? args);

+       [RequiresUnreferencedCode]
+       public void StopActivity<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>(Activity activity, T? args);
     }
}