search

getsentry / sentry-dotnet

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

fix: `Release` Builds with Trimming enabled crashing on launch with NLog Integration #3743

Closed bricefriha closed 1 week ago

bricefriha commented 2 weeks ago

This PR aims to fix #3680, an issue that causes a crash when using NLog with FailedRequestStatusCodes options in a MAUI app with Trimming enabled in Release.

Note

we needed to update Nlog to 5.2 to be able to use RegisterType for this fix

worth mentioning: https://github.com/getsentry/sentry-dotnet/issues/3698

bricefriha commented 2 weeks ago

@jamescrosswell About #3698, do you think my PR would be problematic for other platforms? I mean, in a way that would make it safer to simplify SentryNLogOptions's complexity first.

Or can we go ahead with this change for now?

jamescrosswell commented 2 weeks ago

@jamescrosswell About #3698, do you think my PR would be problematic for other platforms? I mean, in a way that would make it safer to simplify SentryNLogOptions's complexity first.

Or can we go ahead with this change for now?

Not sure I follow. This PR is to address #3698 right? I can't see anything in the PR (yet) that is platform specific.

Have you had a chance to test this out yet, to make sure it resolves the issue? It's another one that's hard to test in unit tests due to the need to enable trimming.

bricefriha commented 2 weeks ago

@jamescrosswell Sorry, about that.

The PR is for #3680, but you mentioned in #3698 that we needed to bump the NLog and I did this here as part of the fix.

3698 is about changing the complexity of the options, so I was just wondering if you think it would be better to wait for this to be trackle before merging the PR?

Have you had a chance to test this out yet, to make sure it resolves the issue?

Yes it's all good, I'm just waiting for your input to mark this PR as reviewable

jamescrosswell commented 2 weeks ago

Yes it's all good, I'm just waiting for your input to mark this PR as reviewable

If you're waiting for input on this PR, you should mark it ready for review... or is there another discussion I'm neglecting somewhere?

jamescrosswell commented 2 weeks ago

3698 is about changing the complexity of the options, so I was just wondering if you think it would be better to wait for this to be trackle before merging the PR?

Ah I see, no I don't think we need to change the way the options are structured if this PR resolves the original issue. Changing the options would be quite complex - definitely couldn't be done in a point release (would have to happen in a major version bump) so best to keep separate from this PR.

bricefriha commented 2 weeks ago

so best to keep separate from this PR.

Sounds good. Well, there you have it! 😁

github-actions[bot] commented 5 days ago
Fails
:no_entry_sign: Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- `Release` Builds with Trimming enabled crashing on launch with NLog Integration ([#3743](https://github.com/getsentry/sentry-dotnet/pull/3743))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by :no_entry_sign: dangerJS against 84d00f742a381a7b335fe746bcf1127d800917af