Consider adding a runtime diagnostic debug feature for trimmed applications

[LakshanF]

Trimming a .NET Core application is expected to give size wins without compromising correctness. However, there are some coding patterns, largely centered around arbitrary use of reflections, that make it challenging for the Trimmer to reason about correctness. Serializers (such as XML and DataContractSerilaizers), feature area like EventSource, and other make heavy use of such patterns. The trimmer does a good job of warning in these cases but for scenarios where these warnings are incorrectly suppressed, trimmed applications can fail in production environment that will be hard to root cause.

We should provide a runtime diagnostic feature that can be leveraged in opt-in scenario testing mode that will warn developers of potential problems in their app. Such a feature should have the following:

• Metadata in trimmed assemblies on trimmed types. This issue tracks the request in the mono repo

• Library support to query the trimmed assemblies to get trimmed metadata

• Showcase this feature using a trim-problematic component

• Developer Experience to leverage this feature including a mechanism for make problematic types trim safe

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[LakshanF]

cc @agocke

[jkotas]

Developer Experience to leverage this feature

I think we should start with defining the developer experience, and how it fits with all other existing developer experiences around trimming.

Do we have real world examples that show where the existing developer experiences around trimming fall short?

[jkotas]

I am worried that investing into diagnostics like this will just encourage bad behavior and distract the focus on providing predictable experience.

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Trimming a .NET Core application is expected to give size wins without compromising correctness. However, there are some coding patterns, largely centered around arbitrary use of reflections, that make it challenging for the Trimmer to reason about correctness. Serializers (such as XML and DataContractSerilaizers), feature area like EventSource, and other make heavy use of such patterns. The trimmer does a good job of warning in these cases but for scenarios where these warnings are incorrectly suppressed, trimmed applications can fail in production environment that will be hard to root cause.

We should provide a runtime diagnostic feature that can be leveraged in opt-in scenario testing mode that will warn developers of potential problems in their app. Such a feature should have the following:

• Metadata in trimmed assemblies on trimmed types. This issue tracks the request in the mono repo

• Library support to query the trimmed assemblies to get trimmed metadata

• Showcase this feature using a trim-problematic component

• Developer Experience to leverage this feature including a mechanism for make problematic types trim safe

Author:	LakshanF
Assignees:	-
Labels:	linkable-framework, untriaged
Milestone:	-

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Trimming a .NET Core application is expected to give size wins without compromising correctness. However, there are some coding patterns, largely centered around arbitrary use of reflections, that make it challenging for the Trimmer to reason about correctness. Serializers (such as XML and DataContractSerilaizers), feature area like EventSource, and other make heavy use of such patterns. The trimmer does a good job of warning in these cases but for scenarios where these warnings are incorrectly suppressed, trimmed applications can fail in production environment that will be hard to root cause.

We should provide a runtime diagnostic feature that can be leveraged in opt-in scenario testing mode that will warn developers of potential problems in their app. Such a feature should have the following:

• Metadata in trimmed assemblies on trimmed types. This issue tracks the request in the mono repo

• Library support to query the trimmed assemblies to get trimmed metadata

• Showcase this feature using a trim-problematic component

• Developer Experience to leverage this feature including a mechanism for make problematic types trim safe

Author:	LakshanF
Assignees:	-
Labels:	area-Host, linkable-framework, untriaged
Milestone:	-

[eerhardt]

One concrete place where this feature would be helpful is in the following DiagnosticSource code:

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

Lines 1148 to 1160 in 5533388

	private static TransformSpec? MakeImplicitTransforms(Type type)
	{
	TransformSpec? newSerializableArgs = null;
	TypeInfo curTypeInfo = type.GetTypeInfo();
	foreach (PropertyInfo property in curTypeInfo.DeclaredProperties)
	{
	// prevent TransformSpec from attempting to implicitly transform index properties
	if (property.GetMethod == null || property.GetMethod!.GetParameters().Length > 0)
	continue;
	newSerializableArgs = new TransformSpec(property.Name, 0, property.Name.Length, newSerializableArgs);
	}
	return Reverse(newSerializableArgs);
	}

What this code does is given a Type (which is either an Activity or some object being written to a DiagnosticSource), it will create a "Transform" that converts all the object's properties to a set of KeyValuePair<string, string>s. A typical pattern is to use an anonymous type, like ASP.NET is doing here:

https://github.com/dotnet/aspnetcore/blob/1abdc61c7982ae098d637526cb82f547618cf653/src/Middleware/MiddlewareAnalysis/src/AnalysisMiddleware.cs#L46-L60

        public async Task Invoke(HttpContext httpContext)
        {
            var startTimestamp = Stopwatch.GetTimestamp();
            if (_diagnostics.IsEnabled("Microsoft.AspNetCore.MiddlewareAnalysis.MiddlewareStarting"))
            {
                _diagnostics.Write(
                    "Microsoft.AspNetCore.MiddlewareAnalysis.MiddlewareStarting",
                    new
                    {
                        name = _middlewareName,
                        httpContext = httpContext,
                        instanceId = _instanceId,
                        timestamp = startTimestamp,
                    });
            }

This is problematic because the trimmer won't see anyone using the getters of this type - only the setters are being used. So the DiagnosticSource code above isn't going to log any of the property values, since the getters are trimmed away.

One approach to mitigating this problem is to log a message to the log that says Type {x} has been modified by trimming. Ensure the necessary properties are preserved to get full diagnostic information. The feature being proposed here is what would allow the DiagnosticSource code to check if Type was modified or not.

[jkotas]

Does the linker diagnostic produce warning for this case?

[eerhardt]

Yes, it produces a warning, which is what I'm trying to address now as part of #45623:

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/ILLink/ILLink.Suppressions.xml

Lines 4 to 9 in 31385d4

	<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
	<argument>ILLink</argument>
	<argument>IL2070</argument>
	<property name="Scope">member</property>
	<property name="Target">M:System.Diagnostics.DiagnosticSourceEventSource.FilterAndTransform.MakeImplicitTransforms(System.Type)</property>
	</attribute>

My current thinking is that since there isn't a public API that can be annotated here to preserve the properties, the next best thing would be to give a runtime informational message via the log that tells the user their diagnostic information is incomplete.

[MichalStrehovsky]

This is problematic because the trimmer won't see anyone using the getters of this type - only the setters are being used. So the DiagnosticSource code above isn't going to log any of the property values, since the getters are trimmed away.

Note that we would also like to have the freedom to strip the properties themselves if we see they're not being reflected. We should assume that the code will not see any properties at all (not just properties with missing getters).

[MichalStrehovsky]

One approach to mitigating this problem is to log a message to the log that says Type {x} has been modified by trimming. Ensure the necessary properties are preserved to get full diagnostic information. The feature being proposed here is what would allow the DiagnosticSource code to check if Type was modified or not.

It might be worth pointing out that for the above ASP.NET example, the diagnostic message would be along the lines of:

Type '<>f__AnonymousType0`2[System.String,System.Int32,System.DateTime]' has been modified by trimming. Ensure the necessary properties are preserved to get full diagnostic information.

Not even I would be able to figure out what to do with that.

[eerhardt]

We could put the fully qualified type name, so at least you would know what assembly it is coming from.

Also, it looks like C# embeds the property names into the generic type names, so you would get the names of the properties in the type name as well, which gives you more information about which type it is referring to:

<>f__AnonymousType0<<name>j__TPar, <httpContext>j__TPar, <instanceId>j__TPar, <timestamp>j__TPar>

Offline @jkotas had the suggestion to add a new generic Write<T> API to DiagnosticsSource, which we would annotate the generic argument with [DynamicallyAccessedMembers(PublicProperties)]. I think that would solve this problem with DiagnosticSource since in this case it doesn't walk the full graph - it just loops over the top level properties of the object.

[eerhardt]

Are we still considering this feature? Or should this issue be closed?

[vitek-karas]

There are no plans to add this right now - closing.

[pranavkm]

Opening to reconsider for 7.0.

[MichalStrehovsky]

I don't think we should do this. Over the course of .NET 5 and 6 we put a lot of effort into making sure that the behavior of the app before/after trimming is the same (and one doesn't need to debug their trimmed app to iron out problems caused by trimming).

The only possible use for this diagnostic is to aid in experience that we explicitly don't want to be part of trimming - having to debug the trimmed app because trimming changes the behavior without warnings.

Adding a feature like this will only encourage the ecosystem to build solutions that are fundamentally not compatible with trimming and pat themselves on the back for "doing the work to make it compatible with trimming" (by which they'll mean suppressing trimming warnings, and putting a runtime assert in place).

[agocke]

Is there any new information here that caused us to reconsider? Otherwise I think the previous decision should stand.

[LakshanF]

I agree with @MichalStrehovsky and @jkotas comments above in not pursuing this runtime feature. The position of having the same behavior for trimmed/non-trimmed application allows us to provide a predictable experience for the developer. This feature will fundamentally break that principle.

[pranavkm]

Is there any new information here that caused us to reconsider?

When we looked at ASP.NET Core + STJ, we found it hard to come up with patterns where we could guarantee that a type was not going to use the reflection based code path. The best option we came up with was a way to use the source generated context if it has metadata, and fallback to a reflection based code path if it does not contain that information. The failure behavior for the fallback is really poor for trimmed types and we had hoped having some way to indicate to the user that it failed was important.

[jkotas]

The primary issue is that the default ASP.NET Core + STJ is not really compatible with trimming, and the .NET 6 trimming-compatible model that we were able to come up with is not pleasant to use. In .NET 7, we have to double down on designing a model that is both trimming-compatible and pleasant to use. It will likely require work from accross the team: ASP.NET, libraries - serializers, source generators - Roslyn, runtime, linker.

The failure behavior for the fallback is really poor for trimmed types and we had hoped having some way to indicate to the user that it failed was important.

The experience would be still poor even with this diagnostic feature. The apps are still going to break at will, one just will have more details about the break. It is not a viable option for larger actively developed apps where debugging failures like this is prohibitively expensive.