Skip to content
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

Trimming for .NET 7+ #2732

Merged
merged 117 commits into from
Nov 2, 2023
Merged

Trimming for .NET 7+ #2732

merged 117 commits into from
Nov 2, 2023

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Oct 17, 2023

Summary

One of the pre-requisites for supporting AOT for Sentry to support trimming.

In the Sentry.csproj core package, there are 3 primary challenges with this:

  1. BenDemystifier
  2. WinUIUnhandledExceptionIntegration
  3. System.Text.Json.JsonSerializer

Ben.Demystifier ✅

Ben.Demystifier uses reflection heavily to provide enhanced stack traces.

Approach / Solution

When setting <IsAotCompatible>true</IsAotCompatible> in Sentry.csproj, we get a bunch of errors/warnings about Ben.Demystifier not being compatible with trimming.

To solve this, in our fork of Ben.Demystifier we surface those via RequiresUnreferencedCode attributes on the relavant classes. That way Ben.Demystifier itself compiles when included in a solution with IsAotCompatible set, but only if any code leveraging it handles the warnings (either be not using the Ben.Demystifier functionality in question or by supressing the warnings).

WinUIUnhandledExceptionIntegration ✅

WinUIUnhandledExceptionIntegration uses reflection to set Microsoft.UI.Xaml.Application.Current.UnhandledException to Sentry's WinUIUnhandledExceptionHandler for customers using Microsoft.UI.Xaml (.NET MAUI targeting Windows or WinUI 3 applications). The relection code it users generates the following errors with IsTrimming==true:

Approach / Solution

  • For the time being, if IsTrimmable == true we're not registering our WinUiUnhandledExceptionHandler automatically and we'll need to provide some guidance to users who are both targeting WinUI and wanting to enable AOT compilation.

System.Text.Json.JsonSerializer ✅

When using Trimming, serialization can't be performed using old reflection based technique (which utilise JsonSerializer overloads accepting JsonSerializerOptions parameters).

Approach / Solution

Instead we need to use source generated serializers that can be supplied to overloads JsonSerializer overloads accepting JsonSerializerContext arguments. See System.Text.Json source generators for details.

Bumping the .netcoreapp3 version

In order to use the new source generated JSON serializers, we had to add a PackageReference to System.Text.Json" Version="6.0.8" for version of .NET prior to .net6.0. That resulted in a new error: System.Runtime.CompilerServices.Unsafe doesn't support netcoreapp3.0. Consider updating your TargetFramework to netcoreapp3.1 or later.... As a results, we've had to bump the target framework in our Sentry packages from netcoreapp3.0 to netcoreapp3.1.

Next Steps

Beyond Sentry.csproj many of the other projects can just be added as is to SentryAot.slnf and they compile and tests run without problem. Some have challenges however. One that was obvious during initial exploration is the use of ConfigureFromConfigurationOptions<TOptions> classes to initialize a number of the integrations... which doesn't work with Trimming enabled. This affects:
- Sentry.Extensions.Logging
- Sentry.Maui
- Sentry.Azure.Functions.Worker
- Sentry.AspNetCore

I think we should deal with that in a separate pull request though.

Additional Reference material:

bruno-garcia and others added 30 commits September 15, 2023 17:08
…rs (#2696)

* Replaced multiple TransactionContext constructors to a single constructor having default properties
* Made most of the SpanContext constructor parameters optional
Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments/change suggestions.

I'd still prefer the new solution to be removed (at least until we need it, which we don't in this PR) but that's just me.

I think someone else should review this PR too. @bruno-garcia @bitsandfoxes

src/Sentry/Sentry.csproj Show resolved Hide resolved
test/Directory.Build.props Outdated Show resolved Hide resolved
test/Directory.Build.props Outdated Show resolved Hide resolved
test/Sentry.Tests/Internals/DebugStackTraceTests.verify.cs Outdated Show resolved Hide resolved
test/Sentry.Tests/Internals/DebugStackTraceTests.verify.cs Outdated Show resolved Hide resolved
test/Sentry.Tests/Internals/DebugStackTraceTests.verify.cs Outdated Show resolved Hide resolved
test/Sentry.Tests/Internals/MainExceptionProcessorTests.cs Outdated Show resolved Hide resolved
samples/Directory.Build.props Outdated Show resolved Hide resolved
src/Directory.Build.props Outdated Show resolved Hide resolved
Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor comments/ideas for improvement

@@ -35,6 +35,11 @@
<CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)CodeAnalysis.ruleset</CodeAnalysisRuleSet>
</PropertyGroup>

<!-- Only compile with IsAotCompatible on .NET 6.0 and later. Our Device Tests fail for net7.0-android with this enabled as well -->
<PropertyGroup Condition="'$(TargetFramework)' != 'net7.0-android' AND $([System.Text.RegularExpressions.Regex]::IsMatch('$(TargetFramework)', '^(?!net5\.|net4\d{2}|netcoreapp|netstandard)net\d+'))">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the regexp could do without the negative lookahead

Suggested change
<PropertyGroup Condition="'$(TargetFramework)' != 'net7.0-android' AND $([System.Text.RegularExpressions.Regex]::IsMatch('$(TargetFramework)', '^(?!net5\.|net4\d{2}|netcoreapp|netstandard)net\d+'))">
<PropertyGroup Condition="'$(TargetFramework)' != 'net7.0-android' AND $([System.Text.RegularExpressions.Regex]::IsMatch('$(TargetFramework)', '^net[6-9]'))">

src/Sentry/Sentry.csproj Show resolved Hide resolved
Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, we also need changelog entries for these breaking changes, right?

@jamescrosswell
Copy link
Collaborator Author

Oh wait, we also need changelog entries for these breaking changes, right?

Yeah we do... it's a bit tricky off the basis of this PR only. I suspect we might want to mention something about the symbolication and debug stack traces, and that might require details from some of the PRs you've been working on @vaind.

I've put something basic in there for now. It will need to be adapted once #2778 has been done and we might want to adapt it further and add references to other PRs relevant to AOT.

@jamescrosswell jamescrosswell merged commit 68e143b into feat/4.0.0 Nov 2, 2023
12 checks passed
@jamescrosswell jamescrosswell deleted the net8-trimming branch November 2, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants