Skip to content

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

Merged
jamescrosswell merged 9 commits into
mainfrom
fix/#3680_Nlog
Nov 20, 2024
Merged

fix: Release Builds with Trimming enabled crashing on launch with NLog Integration#3743
jamescrosswell merged 9 commits into
mainfrom
fix/#3680_Nlog

Conversation

@bricefriha

@bricefriha bricefriha commented Nov 11, 2024

Copy link
Copy Markdown
Contributor

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: #3698

Comment thread src/Sentry.NLog/Sentry.NLog.csproj Outdated
@bricefriha

Copy link
Copy Markdown
Contributor Author

@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?

Comment thread src/Sentry.NLog/Sentry.NLog.csproj
@jamescrosswell

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Collaborator

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?

@bricefriha bricefriha marked this pull request as ready for review November 12, 2024 05:06
@jamescrosswell

Copy link
Copy Markdown
Collaborator

#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

Copy link
Copy Markdown
Contributor Author

so best to keep separate from this PR.

Sounds good. Well, there you have it! 😁

@jamescrosswell jamescrosswell changed the title fix: prevent Release Builds with Trimming ON Crashing on Launch when using NLog Integration and FailedRequestStatusCodes Options fix: Release Builds with Trimming enabled crashing on launch with NLog Integration Nov 16, 2024
Comment thread CHANGELOG.md

@jamescrosswell jamescrosswell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great. Thanks @bricefriha !

@jamescrosswell jamescrosswell self-requested a review November 20, 2024 08:09
@jamescrosswell jamescrosswell merged commit d93f96b into main Nov 20, 2024
@jamescrosswell jamescrosswell deleted the fix/#3680_Nlog branch November 20, 2024 09:26
Comment thread CHANGELOG.md

- Fixed ArgumentNullException in FormRequestPayloadExtractor when handling invalid form data on ASP.NET ([#3734](https://github.com/getsentry/sentry-dotnet/pull/3734))
- ArgumentNullException in FormRequestPayloadExtractor when handling invalid form data on ASP.NET ([#3734](https://github.com/getsentry/sentry-dotnet/pull/3734))
- Crash when using NLog with FailedRequestStatusCodes options in a Maui app with Trimming enabled ([#3743](https://github.com/getsentry/sentry-dotnet/pull/3743))

@bruno-garcia bruno-garcia Nov 22, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MAUI is all caps. Maui is a city in Hawaii

@github-actions

Copy link
Copy Markdown
Contributor
Fails
🚫 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 🚫 dangerJS against 84d00f7

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.

Release Builds Crashing on Launch when using NLog Integration and FailedRequestStatusCodes Options

3 participants