-
-
Notifications
You must be signed in to change notification settings - Fork 62
fix: Capture Debug.LogError as message, not exception
#2377
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
base: feat/bump-version6
Are you sure you want to change the base?
Conversation
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.
These are not new tests. The parsing was already tested in the UnityLogExceptionTests and now moved into the helper.
| AddEventProcessor(processor); | ||
| AddTransactionProcessor(processor); | ||
| AddExceptionProcessor(new UnityExceptionProcessor()); | ||
|
|
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.
Bug: UnityLogEventFactory.CreateExceptionEvent() misses Terminal = false for Mechanism, affecting Debug.LogError on WebGL.
Severity: HIGH | Confidence: 0.95
🔍 Detailed Analysis
The UnityLogEventFactory.CreateExceptionEvent() method omits Terminal = false when setting the Mechanism property. This occurs when exceptions are captured from Debug.LogError on WebGL, as CreateExceptionEvent() is still used in this scenario. The absence of Terminal = false means these exceptions will incorrectly mark Sentry sessions as crashed, violating the documented and established behavior for non-terminal log exceptions.
💡 Suggested Fix
Add Terminal = false to the Mechanism object within UnityLogEventFactory.CreateExceptionEvent() to ensure exceptions from Debug.LogError are correctly marked as non-terminal, aligning with established behavior.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/Sentry.Unity/SentryUnityOptions.cs#L346
Potential issue: The `UnityLogEventFactory.CreateExceptionEvent()` method omits
`Terminal = false` when setting the `Mechanism` property. This occurs when exceptions
are captured from `Debug.LogError` on WebGL, as `CreateExceptionEvent()` is still used
in this scenario. The absence of `Terminal = false` means these exceptions will
incorrectly mark Sentry sessions as crashed, violating the documented and established
behavior for non-terminal log exceptions.
Did we get this right? 👍 / 👎 to inform future reviews.
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.
Bug: Bug
The IL2CPP processor now attempts to process synthetic exceptions created by UnityLogEventFactory.CreateExceptionEvent. The incomingException for these is a simple Exception(message) lacking native IL2CPP metadata, causing incorrect or empty stack traces. This also creates an inconsistency between incomingException and the synthetic SentryException (with unity.log mechanism) in sentryEvent.SentryExceptions.
src/Sentry.Unity/Il2CppEventProcessor.cs#L24-L39
sentry-unity/src/Sentry.Unity/Il2CppEventProcessor.cs
Lines 24 to 39 in b608e71
| // We're throwing here but this should never happen. We're validating UnityInfo before adding the processor. | |
| _il2CppMethods = UnityInfo.Il2CppMethods ?? throw new ArgumentNullException(nameof(UnityInfo.Il2CppMethods), | |
| "Unity IL2CPP methods are not available."); | |
| Options.SdkIntegrationNames.Add("IL2CPPLineNumbers"); | |
| } | |
| public void Process(Exception incomingException, SentryEvent sentryEvent) | |
| { | |
| Options.DiagnosticLogger?.LogDebug("Running Unity IL2CPP event exception processor on: Event {0}", sentryEvent.EventId); | |
| var sentryExceptions = sentryEvent.SentryExceptions; | |
| if (sentryExceptions == null) | |
| { | |
| return; | |
| } |
src/Sentry.Unity/Integrations/UnityLogEventFactory.cs#L56-L61
sentry-unity/src/Sentry.Unity/Integrations/UnityLogEventFactory.cs
Lines 56 to 61 in b608e71
| return new SentryEvent(new Exception(message)) | |
| { | |
| SentryExceptions = [sentryException], | |
| Level = SentryLevel.Error | |
| }; |
Bug: Bug
The DebugLogError_InTask_IsCapturedAndIsMainThreadIsFalse test has issues following the change to capture Debug.LogError events as messages. It incorrectly expects the unity.is_main_thread tag and uses the "value" attribute to identify the message instead of "message", unlike the DebugLogError_IsCaptured test.
test/Sentry.Unity.Tests/IntegrationTests.cs#L241-L248
sentry-unity/test/Sentry.Unity.Tests/IntegrationTests.cs
Lines 241 to 248 in b608e71
| [UnityTest] | |
| public IEnumerator DebugLogError_InTask_IsCapturedAndIsMainThreadIsFalse() | |
| { | |
| if (TestEnvironment.IsGitHubActions) | |
| { | |
| Assert.Inconclusive("Flaky"); // Ignoring because of flakiness | |
| } | |
Depends on getsentry/sentry-dotnet#4681
Resolves #2334
Problem
The
UnityApplicationLoggingIntegrationreceives stringified stack traces from Unity for log messages. The SDK automatically capturesDebug.LogErrorevents whenCaptureLogErrorEventsis enabled. Previously, whenAttachStacktracewas enabled, the SDK created a syntheticUnityErrorLogExceptionto attach the stack trace to these events, which would then be processed as a normal exception.This approach caused several issues:
fatalwhen they were just error logsProposal
Debug.LogErrorevents are now captured as message events with stack traces attached via theSentryThreadsproperty, following the same pattern as theMainEventProcessoruses (see here) for regular messages withAttachStacktraceenabled.Result