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

feat: debug images for Portable PDB #2050

Merged
merged 51 commits into from
Dec 20, 2022
Merged

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Nov 15, 2022

This adds the new event properties defined in RFC 0013, namely InstructionAddress / FunctionId + Relative Indexing, and a list of pe_dotnet DebugImages.

TODOs & followups

  • make sure we know how this affects existing issues grouping - seems to be creating new issues if users upload symbols meaning all ignored issues would be resurrected too.
  • add changelog, depending on the previous item
  • docs, if needed
  • Android doesn't contain assemblies as DLLs
  • generate PDBs for Sentry libraries (see the asp.net (or Android) event below) (see sentry-cli uploading PDBs fails when symbols embedded #2073)
  • unit test MergeDebugImagesInto()
  • unit test for stack traces to contain (correct) debug images
  • maybe device tests that debug images are actually sent? Locally, this seems covered because we're testing it through the verifier.

Event examples

Notice the presence of the "Images loaded" section and the line numbers in (full) stack traces. All apps were built with the following commands before executing:

cd samples/<sample-name>
git clean -ffxd . # after a branch switch
dotnet build --configuration Release 
sentry-cli upload-dif --project=sentry-dotnet --org=sentry-sdks --wait bin/Release/
rm -vf ./**/*.pdb # Remove-Item ./* -Recurse -Include "*.pdb"

@github-actions
Copy link
Contributor

github-actions bot commented Nov 15, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 2c532be

@Swatinem
Copy link
Member

Swatinem commented Nov 16, 2022

Thanks @vaind for picking this up ❤️

Serverside everything should be ready, you should be able to upload portable pdb files via sentry-cli, and serverside symbolication should pick up the new event properties. Feel free to reach out to me anytime if you need help.

@vaind vaind force-pushed the feat/debug-image-info branch 3 times, most recently from 24f1816 to 8567b0f Compare November 21, 2022 21:03
@vaind vaind force-pushed the feat/debug-image-info branch 3 times, most recently from a23941d to a664893 Compare November 25, 2022 11:54
@vaind vaind marked this pull request as ready for review November 25, 2022 20:04
@vaind vaind mentioned this pull request Nov 30, 2022
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

First pass. I'll come back to review those AssemblyReaders and other bits

scripts/accept-verifier-changes.ps1 Show resolved Hide resolved
scripts/update-test-apks.ps1 Outdated Show resolved Hide resolved
src/Sentry/Extensibility/SentryStackTraceFactory.cs Outdated Show resolved Hide resolved
src/Sentry/Sentry.csproj Outdated Show resolved Hide resolved
src/Sentry/Internal/DebugStackTrace.cs Outdated Show resolved Hide resolved
src/Sentry/Internal/DebugStackTrace.cs Outdated Show resolved Hide resolved
src/Sentry/Internal/DebugStackTrace.cs Outdated Show resolved Hide resolved
src/Sentry/Internal/DebugStackTrace.cs Outdated Show resolved Hide resolved
src/Sentry/Internal/DebugStackTrace.cs Show resolved Hide resolved
@bruno-garcia
Copy link
Member

Forgot to thank you for doing this. This is a huge step for Sentry's .NET support. 🙏

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I haven’t reviewed the AssemblyReader in too much detail.
lgtm, though I would rather defer proper review to @bruno-garcia

src/Sentry/Internal/AndroidAssemblyReader.cs Outdated Show resolved Hide resolved
@vaind vaind force-pushed the feat/debug-image-info branch 2 times, most recently from ceca6c0 to aac53d8 Compare December 6, 2022 15:41
Swatinem and others added 8 commits December 7, 2022 14:43
This adds new protocol fields to StackFrame and DebugImage that are needed for server side Portable PDB symbolication.

The StackTraceFactory is also extended in such a way that it collects referenced DebugImages which are then later added to the event.
@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Dec 19, 2022

Almost done here. Some failures to investigate. Will finish and merge soon. Overall looking pretty good. 🙂

@mattjohnsonpint
Copy link
Contributor

There's an issue with Verify getting in the way here. Addressing that in #2092.

@mattjohnsonpint mattjohnsonpint merged commit f98922d into main Dec 20, 2022
@mattjohnsonpint mattjohnsonpint deleted the feat/debug-image-info branch December 20, 2022 00:16
@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Dec 20, 2022

@vaind - I pushed some changes to address the items I saw, and merged this. Got caught on the unrelated Verify issue along the way, but that's fixed now too. The main changes were:

  • Moved the Android assembly reader classes to their own folder/namespace under src/Sentry/Platforms/Android/AssemblyReader. That will keep them only being distributed with the Android target.
  • Made a separate test project Sentry.Android.Tests that pulls in those files manually for the non-Android testing.
  • Updated the LZ4 dependency also, so only the Android target depends on it at runtime.
  • Made a separate app in test/AndroidTestApp that is used for the apk tests. That prevents some circular build issues (since it doesn't depend on Sentry) and it also builds faster anyway.

Everything else was minor refactoring and a few script changes. Overall things looked pretty good to me. Thanks for your hard work on this one!

I did see a few unused items in AndroidAssemblyStoreReader. I left them there, since the comment said the code was adapted from Xamarin sources. But if you think we don't need them then let me know (or send a new PR). They are:

  • AssemblyStoreExplorer.Assemblies
  • AssemblyStoreManifestReader.Entries
  • AssemblyStoreManifestEntry.StoreId
  • AssemblyStoreManifestEntry.IndexInStore
  • AssemblyStoreHashEntry.Is32Bit
  • AssemblyStoreAssembly.GetDebugData
  • AssemblyStoreAssembly.GetAssemblyConfig

Thanks.

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.

Line number for .NET stack traces with server-side PDB support
4 participants