getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
584 stars 206 forks source link

Incorrect release version detection for MacOS applications #3639

Open TimBurik opened 3 weeks ago

TimBurik commented 3 weeks ago

Package

Sentry

.NET Flavor

.NET

.NET Version

8.0.303

OS

macOS

SDK Version

4.10.2

Self-Hosted Sentry Version

No response

Steps to Reproduce

  1. Create a sample project Sentry.Test which targets .net8-macos
  2. Specify application identifier and version in the Info.plist:
    <key>CFBundleIdentifier</key>
    <string>com.Test.Sentry-Mac</string>
    <key>CFBundleShortVersionString</key>
    <string>5.55.0</string>
    <key>CFBundleVersion</key>
    <string>555</string>
  3. Add Sentry nuget package, initialize it from the NSApplicationDelegate.DidFinishLaunching() callback
  4. Capture and send any exception using SentrySdk.CaptureException()

Expected Result

Captured exception is reported with a release version in the format "bundle_identifier>@<bundle_short_version+", in this case it should be "com.Test.Sentry-Mac@5.55.0+555"

Actual Result

Captured exception is reported with a release version of "Sentry.Test@1.0.0+3698dd19ef2b4114dd77aed39cc71fd09a9c6586", which seems to be an assembly name and version + git commit hash

It seems that if application is targeting .net8-macos - Sentry Cocoa SDK is not initialized at all, even though it feels like it should. https://github.com/getsentry/sentry-dotnet/blob/33be706eda6b4281e97de82e3f7b025e1e764e30/src/Sentry/SentrySdk.cs#L67-L69

Sevenish commented 3 weeks ago

Looking at the nuget entries for Sentry and Sentry.Bindings.Cocoa is seems the cocoa binding are not installed or supported for net8-macos but only for net8.0-maccatalyst.

Sevenish commented 3 weeks ago

As a workaround you can set the release manually when initialising sentry:

o.Release = NSBundle.MainBundle.ObjectForInfoDictionary("CFBundleVersion").ToString();

TimBurik commented 3 weeks ago

@Sevenish I suppose a workaround could be used, but I am wondering if the current behavior is valid and expected. Because according to the documentation, it seems that native Sentry Cocoa SDK does support MacOS applications: https://docs.sentry.io/platforms/apple/guides/macos/usage/#capturing-uncaught-exceptions

Sevenish commented 3 weeks ago

@TimBurik I know. This is very frustrating. Working with net8-macos you will encounter this patern often. The source library supports native non-catalyst appkit. But the binding libraries for dotnet do not. Because they focus on dotnet MAUI which only supports catalyst. There is so much focus on MAUI from Microsoft. Many don't even realise that normal macos appkit is still fully supported in dotnet

jamescrosswell commented 2 weeks ago

Yeah the Sentry SDK doesn't currently target .net8-macos or provide any specific support for it. MAUI support was added before I started working on the SDK so I'm not sure if the omission of .net8-macos was intentional or not. @bitsandfoxes do you have any context for this?

We could investigate adding this but it might be a while until we get around to it as there's quite a lot on at the moment. It would help to know what impact this might have as well. How much easier would life be for SDK users if we spent a few weeks ensuring the SDK worked with .net8-macos?

bruno-garcia commented 2 weeks ago

I'm not sure if the omission of .net8-macos was intentional or not

There was no specific API from macOS that we needed, so no need to add a specific platform target like that.

We added the -maccatalyst becase we have the sentry-cocoa library, bindings and a series of native support. But we never did add sentry-cocoa solely for an app running on macOS.

So the versioning format that you're getting on a Mac, is how the SDK versions all apps on all platforms, except Android and iOS (and Mac Catalyst which is basically iOS). If you build and run on Windows, macOS or Linux, that is. The version will be as you have it now.

So someone developing an aspnetcore app on Mac, they'll see the version the same way that it would be on their Linux or Windows server.

As a workaround you can set the release manually when initialising sentry: o.Release = NSBundle.MainBundle.ObjectForInfoDictionary("CFBundleVersion").ToString();

I would suggest you take this approach. For us to change the version now for all apps running on macOS, it would be a rather large breaking change.

That said, we should at least document this, so others looking at adding the Release in the format more friendly for Mac apps, can do so. Perhaps if this is more of a common scenario than not, and we can find a way to change this without breaking the world on non Mac app developers workflow (e.g: aspnet), we should do that on a major version too.

Sevenish commented 2 weeks ago

@bruno-garcia I feel like there is some confusion on what the -macos target is or does.

It is not needed to run dotnet on macos. The basic net6, net7 and net8 run fine on all supported platforms. asp.net is included in the basic target. An asp.net project would not normally target net8-macos but just net8. Just start a new asp net project on macos and look at the target framework. Just "net-8.0". Same goes for a console app.

see https://learn.microsoft.com/en-us/dotnet/standard/frameworks qoute from that page: "net6.0-macos - xamarin.mac (plus everything else inherited from net6.0)"

What the -macos target does is add specific bindings for desktop apps like AppKit, StoreKit... This is what xamarin.mac was for before net6. It is meant for native desktop cocoa apps written in dotnet.

The -macos target is similar to the -ios and -maccatalyyst targets. But used for classic AppKit/Cocao apps.

Your choice for not using the sentry-cocoa library means all the great extra features and intergrations are dropped: https://docs.sentry.io/platforms/apple/guides/macos/features/

We lose all that just because it is programmed in dotnet/c# instead of objc or swift.

It would be great if there was at least an option of enabling sentry-cocoa

TimBurik commented 2 weeks ago

Also it seems that there's a behavior difference for .net-macos apps (.net-maccatalyst might also be affected) - AppKit seems to swallow all unhandled exceptions on the main thread: https://github.com/xamarin/xamarin-macios/issues/15037 We didn't manage to re-route unhandled exceptions into AppDomain.CurrentDomain.UnhandledException event and ended up using ObjCRuntime.Runtime.MarshalManagedException event to capture unhandled exceptions manually

bruno-garcia commented 5 days ago

It is not needed to run dotnet on macos. The basic net6, net7 and net8 run fine on all supported platforms. asp.net is included in the basic target. An asp.net project would not normally target net8-macos but just net8. Just start a new asp net project on macos and look at the target framework. Just "net-8.0". Same goes for a console app.

No confusing there for me, I understand.

Your choice for not using the sentry-cocoa library means all the great extra features and intergrations are dropped: https://docs.sentry.io/platforms/apple/guides/macos/features/

It's not only a choice, there's a prioritization consideration, and added complexity of the SDK which are costs.

That said, what I am not clear about, is if the bindings we use (Sentry.Bindings.Cocoa) could be used on a netX.0-macos. My understanding is that the original Xamarin Mac/iOS bindings didn't work with the "new", .NET Core -macos targets.

But if I'm outdated/wrong here, and it's an easier light to add Sentry cocoa to -macos, we could consider adding it to the Sentry package.

Also it seems that there's a behavior difference for .net-macos apps (.net-maccatalyst might also be affected) - AppKit seems to swallow all unhandled exceptions on the main thread: https://github.com/xamarin/xamarin-macios/issues/15037 We didn't manage to re-route unhandled exceptions into AppDomain.CurrentDomain.UnhandledException event and ended up using ObjCRuntime.Runtime.MarshalManagedException event to capture unhandled exceptions manually

Is there a work around we can apply here? Since @rolfbjarne raised that ticket, he might have some tips.

rolfbjarne commented 2 days ago

Also it seems that there's a behavior difference for .net-macos apps (.net-maccatalyst might also be affected) - AppKit seems to swallow all unhandled exceptions on the main thread: xamarin/xamarin-macios#15037 We didn't manage to re-route unhandled exceptions into AppDomain.CurrentDomain.UnhandledException event and ended up using ObjCRuntime.Runtime.MarshalManagedException event to capture unhandled exceptions manually

Is there a work around we can apply here? Since @rolfbjarne raised that ticket, he might have some tips.

This might work: https://github.com/dotnet/maui/issues/7176#issuecomment-1766065230

bruno-garcia commented 23 hours ago

Thanks @rolfbjarne.

For reference, the tip is:

var defs = new NSDictionary ((NSString) "NSApplicationCrashOnExceptions", NSValue.FromBoolean (true)); NSUserDefaults.StandardUserDefaults.RegisterDefaults (defs);