-
-
Notifications
You must be signed in to change notification settings - Fork 210
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+ #2699
Trimming for .NET 7+ #2699
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Trimming for .NET 7+ ([#2699](https://github.com/getsentry/sentry-dotnet/pull/2699)) If none of the above apply, you can opt out of this check by adding |
I'm not sure Ben.Demystifier would make much sense in Native AOT because there is not enough information at runtime. Most we can get is a module (ns+class) & method name parsed from frame ToString() output. See dotnet/runtime#92869 |
OK phew! That's one less thing on the backlog then 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've come to look at this PR to see how you're checking whether an app is being published as Native AOT. As far as I can tell, it can only be done at runtime with our current packaging process because we're only shipping a single compiled DLL so we can't just ifdef
.
Alternatively, I think we could ship two versions of the library (for each framework), by following the approach given here: https://learn.microsoft.com/en-us/nuget/create-packages/supporting-multiple-target-frameworks#architecture-specific-folders and have a version for aot
platform identifier and another for any
.
src/Sentry/Sentry.csproj
Outdated
@@ -5,6 +5,8 @@ | |||
<NoWarn Condition="'$(TargetFramework)' == 'netstandard2.0'">$(NoWarn);RS0017</NoWarn> | |||
<CLSCompliant Condition="'$(TargetPlatformIdentifier)' == ''">true</CLSCompliant> | |||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | |||
<IsTrimmable>true</IsTrimmable> | |||
<DefineConstants>$(DefineConstants);TRIMMABLE</DefineConstants> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this add the TRIMMABLE constant unconditionally? That effectively removes enhanced stack frames at all times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I does yeah... I didn't intend for this to be merged into the feat/4.0.0 branch - it was just a quick/easy hack to enable trimming so that I could get a sense for the challenges and start to try to work through those.
I was scratching my head to think how we could do this as well and haven't come up with any good options yet. We might, as you suggested, need separate packages for AOT and Managed. I'm not sure how that is done in MSBuild though. When targeting multiple frameworks, there's a We can always pass this in via an environment variable or the command line, which will work well for our CI workflows on GitHub, but that seems like a clumsy experience for us on our development machines... particularly for testing (where again, multiple frameworks are well supported but multiple platforms conspicuously not mentioned 🤔). |
I've already checked this a couple of weeks ago. There's no support for unit testing at the moment, see dotnet/runtime#79316. Also citing a response from the thread:
On the other hand, I think we need to do integration tests ourselves (for things we can't mock in unit tests) because of the different handling of native-aot vs "standard" builds. |
Enabling trimming will likely be a more constrained/limited experience. We can't serialize anonymous types, enhanced stack traces won't work with AOT and there may be other differences. Users who don't need AOT would probably miss the flexibility/capability they get from reflection so I suspect we'll have to maintain variants of the codebase depending on whether trimming is enabled or not.
Yeah, if we support both AOT and Managed, at the very least we'll want to test each assembly with trimming enabled and disabled. Currently it looks like Sentry will be behave quite different when Trimming/AOT is enabled. |
If you mean the dynamic-value stuff in json extensions - can't we get rid of that completely, regardless of publishing type? There was a breaking change suggested by Matt in the issue about requiring an interface instead? |
Maybe. We'd have to work out what to do with things like Span.Extra and Transaction.Extra, in that case: sentry-dotnet/src/Sentry/Transaction.cs Line 178 in 3c3182d
Currently users can put arbitrary things in the scope with At the moment, the code I have preserves our previous functionality if people don't have trimming enabled... so the scenario above is only a problem when Trimming is turned on. Either way, we have to solve the above for the trimming scenario and maybe then we see whether that behaviour is acceptable in managed/JIT scenarios as well. I'll play around with it over the next few days. |
Closing in favour of #2732 |
Summary
The goal is to support AOT on .NET 7 and 8, per #2247. Matt has left quite a bit of context in the Issue already.
Additional Reference material:
Trimming issues
When enabling trimming for
Sentry.csproj
we have the following main issues.Ben.Demystifier ✅
Ben.Demystifier
uses reflection heavily to provide enhanced stack traces.Approach / Solution
We've disable Ben.Demystifier (so only original stack traces) if
IsTrimmable == true
.WinUIUnhandledExceptionIntegration ✅
WinUIUnhandledExceptionIntegration
uses reflection to setMicrosoft.UI.Xaml.Application.Current.UnhandledException
to Sentry'sWinUIUnhandledExceptionHandler
for customers usingMicrosoft.UI.Xaml
(.NET MAUI targeting Windows or WinUI 3 applications). The relection code it users generates the following errors withIsTrimming==true
:Brainstorming / Options
WinUIUnhandledExceptionIntegration
to the Sentry.Maui package, to a new package, or make a Windows target for the main Sentry package...WinUIUnhandledExceptionIntegration
gets registered as the event handlerIL2075
but I don't think it will work for theIL2026
errorApproach / Solution
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 ☣️
We're still getting the following warnings/errors, which we haven't yet resolved.
Brainstorming / Options
The most obvious solution is to try to use
System.Text.Json
source generators (as the error messages suggest). This should, at the very least, work for known types. It might get a little fiddly for some of the classes we've got withDictionary<string,object?>
properties... need to play around with this to get a feel for it though. As Matt suggested, we could consider an API that would let the end-user provide JsonSerializerContext.Approach / Solution
We have exposed an API for users to supply a custom
JsonSerializerContext
viaSentryOptions.AddJsonSerializerContext
.