Skip to content
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

Add Portable PDB support to Raygun4Net and Raygun4NetCore #528

Merged
merged 16 commits into from
May 22, 2024

Conversation

xenolightning
Copy link
Contributor

@xenolightning xenolightning commented May 5, 2024

This captures the required debugging information for reversing the stacktrace line numbers and file from the stack trace when a PDB is not included in the build output

Updated payload:

image

This captures the required debugging information for reversing the stacktrace line numbers and file from the stack trace when a PDB is not included in the build output
This could be an issue in medium trust environments, so safer just to try/catch it, especially in this critical part of code
phillip-haydon
phillip-haydon previously approved these changes May 5, 2024
Copy link
Contributor

@phillip-haydon phillip-haydon left a comment

Choose a reason for hiding this comment

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

:shipit:

1Q: the offset is -1, if the try fails and -1 is set but we have other info what happens to the offset? Do we ignore if the offset is -1?

@xenolightning
Copy link
Contributor Author

xenolightning commented May 5, 2024

:shipit:

1Q: the offset is -1, if the try fails and -1 is set but we have other info what happens to the offset? Do we ignore if the offset is -1?

Yeah I wanted a non-sensical value for determining that it doesn't exist. What I might do is set it to the constant. Which is equal to -1 :D

https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.stackframe.offset_unknown?view=net-8.0

@ProRedCat
Copy link
Contributor

I am happy with this, it works for MAUI (iOS and Android tested) with the traces being symbolicated when using Sean's tool

image

One thing we noticed was that if any program is created without PDB files, the IL offsets will cause the symbolicated trace to be off by one line. This is not an issue as it's undefined behaviour to build an app and then build the PDB separately.

@xenolightning
Copy link
Contributor Author

Holding on this just for now, we may need to add more information to the "frame" that we can use to determine what PDB we should be looking at, maybe that's assembly, or some sort of signature - not sure yet

@xenolightning
Copy link
Contributor Author

There is issues using the PEReader in the context of the Android platform. Xamarin has some code to read assemblies out of the blob

We are going to need to explore this code, and probably port it into Raygun4Net to capture the PDB signatures when targeting android

https://github.com/xamarin/xamarin-android/tree/main/tools/assembly-store-reader

This saves having to go to file/disk/whatever else, when looking for the symbols.

This could be expensive on each crash/frame, so we want to avoid doing too much work
@xenolightning
Copy link
Contributor Author

Raw report inside raygun, after collecting the new fields. The full pipeline supports these fields. They haven't been reversed in the screenshots, but we have the data to do the reversal!

image
image


namespace Mindscape.Raygun4Net
{
public class RaygunErrorMessageBuilder
{
private static readonly ConcurrentDictionary<string, PdbDebugInformation> DebugInformationCache = new();
public static Func<string, PEReader> AssemblyReaderProvider { get; set; } = PortableExecutableReaderExtensions.GetFileSystemPEReader;
Copy link
Contributor Author

@xenolightning xenolightning May 19, 2024

Choose a reason for hiding this comment

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

Intention here is that this is replaced with an android version for MAUI.

This needs to be bundled inside the raygun4maui release though, as our core library doesn't have platform targets

Copy link
Contributor

@phillip-haydon phillip-haydon left a comment

Choose a reason for hiding this comment

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

👍

Debug.WriteLine($"Could not load debug information: {ex}");
}

return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the module is not found and returns null, we could consider adding it to the dictionary too to avoid the lookup attempt every time.

Copy link
Contributor Author

@xenolightning xenolightning May 20, 2024

Choose a reason for hiding this comment

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

Interesting, I originally had this code as that something went wrong and we shouldn't add it, and maybe we could try again.

But when there's no exceptions, maybe we do add a null?

Debug.WriteLine($"Could not load debug information: {ex}");
}

return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as the other implementation.

@xenolightning
Copy link
Contributor Author

Tagged @QuantumNightmare on this one too, as he had suggestions on improving the payload

Copy link
Contributor

@QuantumNightmare QuantumNightmare left a comment

Choose a reason for hiding this comment

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

Last 3 commits look good to me, just commented on what looks like a typo. Would be good to avoid the duplicate code too, but that could be left for another round as I know there are already thoughts on simplifying the repository.

@xenolightning
Copy link
Contributor Author

Last 3 commits look good to me, just commented on what looks like a typo. Would be good to avoid the duplicate code too, but that could be left for another round as I know there are already thoughts on simplifying the repository.

Yeah, we end up having to make changes to both framework and core sadly.

We do have plans to consolidate them into a single netstandard targeting lib at some point soon

Copy link
Contributor

@QuantumNightmare QuantumNightmare left a comment

Choose a reason for hiding this comment

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

Thanks for the typo fixes

@xenolightning xenolightning changed the title Support capturing ILOffset and MetadataToken for the method Add Portable PDB support to Raygun4Net and Raygun4NetCore May 21, 2024
Copy link
Contributor

@phillip-haydon phillip-haydon left a comment

Choose a reason for hiding this comment

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

🚀

@phillip-haydon
Copy link
Contributor

Don't forget to update the Change Log.

@xenolightning
Copy link
Contributor Author

Don't forget to update the Change Log.

I'll do a larger changelog update for 11.0 which will also include the offline storage support for netcore

@xenolightning
Copy link
Contributor Author

Going to merge and release as 11.0-pre1

Then we can base raygun4maui off this package, and bring the rest into line

@xenolightning xenolightning merged commit cd3a86c into master May 22, 2024
1 check passed
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.

None yet

4 participants