Skip to content

PLAT-9092 missing stacktraces in android notify events #700

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

Merged
merged 5 commits into from
Mar 7, 2023

Conversation

richardelms
Copy link
Contributor

@richardelms richardelms commented Feb 27, 2023

Goal

If a C# exception is not thrown then it contains no stacktrace.

When Bugsnag.Notify(System.Exception) is called with an unthrown exception, we generate a backup stacktrace using System.Diagnostics.StackTrace().GetFrames();

Previously, when generating this trace we stripped the top 3 frames so that internal frames from the notifier would not be reported.

The pipeline now automatically marks any frame from our notifier as out of project, and in some cases we were stripping frames that should not be stripped due to compiler optimisations.

In this PR we remove the stripping and allow the pipeline to decide what is and isn't in project.

Changes to grouping

I was initially concerned that this change would break grouping for existing events but i have tested and this change will only break grouping for events where there are no InProject frames and Notify is called with an unthrown exception which is acceptable.

Changeset

  • Removed hardcoded frame stripping.
  • Removed unecessary Notify overloads in Client.cs and Bugsnag.cs

Testing

  • Re-enabled existing Android tests that were disabled due to this issue.
  • Existing tests should cover this change
  • Added overload check to BugsnagUnity.Tests

@richardelms richardelms self-assigned this Feb 27, 2023
@richardelms richardelms marked this pull request as ready for review February 27, 2023 21:27
@@ -4,6 +4,7 @@
using System.Collections;
using System.IO;
using System.Runtime.InteropServices;
using BugsnagUnity;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unused import now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed 👍

@richardelms richardelms merged commit 904f151 into next Mar 7, 2023
@richardelms richardelms deleted the 9092-emptyTraceOnAndroidNotify branch March 7, 2023 13:36
@richardelms richardelms mentioned this pull request Mar 8, 2023
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.

2 participants