-
-
Notifications
You must be signed in to change notification settings - Fork 44
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: Add support for Portable PDB debug files / caches and .NET symboliction #883
Conversation
Codecov ReportBase: 70.28% // Head: 70.72% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## release/upd-deps #883 +/- ##
====================================================
+ Coverage 70.28% 70.72% +0.43%
====================================================
Files 58 59 +1
Lines 11311 11621 +310
====================================================
+ Hits 7950 8219 +269
- Misses 3361 3402 +41
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
only a partial review so far
crates/symbolicator/src/services/symbolication/symbolication.rs
Outdated
Show resolved
Hide resolved
This does a `cargo update`, pulling in a new symbolic and reqwest. The updated symbolic contains a new symcache version that now uses a LEB128-prefixed string table. The patched reqwest was also updated to the latest upstream code.
eb31e84
to
76c3afc
Compare
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.
lgtm
image_addr: "0x0" | ||
candidates: | ||
- source: local | ||
location: "http://localhost:<port>/download/integration.pdb/0C1033F78632492E91C6C314B72E1920e60b819d/integration.pdb" |
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.
NOTE to self: not sure if we should FF out the age right away, or leave it for later.
index: usize, | ||
) -> Result<Vec<SymbolicatedFrame>, FrameStatus> { | ||
tracing::trace!("Loading symcache"); | ||
let symcache = match symcache.parse() { |
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.
I’m just now realizing that we are loading these cache files over and over again for each frame. Not a problem for now, but an opportunity for improvement later on.
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.
It also happens in several different places.
crates/symbolicator/src/services/symbolication/symbolication.rs
Outdated
Show resolved
Hide resolved
crates/symbolicator/src/services/symbolication/symbolication.rs
Outdated
Show resolved
Hide resolved
The RFC regarding extension of the protocol is here: getsentry/rfcs#13 |
This implements support for Portable PDB files, namely fetching them from sentry and external symbol sources as well converting those to appropriate cache files.
This also implements .NET event symbolication based on the new Event properties specified in RFC 0013.
Depends on getsentry/symbolic#696.