-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Get Debug ID for Full PDB format on Windows #2222
Conversation
DebugFile: mscorlib.pdb, | ||
CodeId: ______________, | ||
CodeFile: .../mscorlib.dll | ||
}, |
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.
Hey look - now that are collecting it, we also get a debug id for mscorlib on .NET Framework. (This should light up some framework symbolication.)
8c8e345
to
a2f6d05
Compare
return idx; | ||
} | ||
|
||
internal static DebugImage? GetDebugImage(Module module, SentryOptions options) |
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 this is also just refactoring. The AddDebugImage
method now is entirely responsible for managing the cache, while GetDebugImage
can focus on the hard parts. (We can later add some tests on GetDebugImage
.)
a2f6d05
to
69539dc
Compare
else | ||
{ | ||
// Full PDB Format (Windows only) | ||
// Version Major=0, Minor=0 | ||
debugId = $"{codeView.Guid}-{codeView.Age}"; | ||
} |
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.
This is the format we did not previously have. Everything else in the PR is refactoring and optimization.
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.
What if we write something like [assembly: SentrySourceContext]
if <UploadSources>
or <EmbedSources>
is on. In other words figure out at runtime that we in fact uploaded sources during build time?
And only then add debug_meta?
Sounds great though it would break uses using sentry-cli manually. |
Another problem is, we'd have to write the attribute to the assembly during compilation before the upload was attempted. So if the upload fails, it's to late to change the attribute. I'll keep thinking about it though. It might be ok. Either way, it's a separate concern from this PR. This one is specifically about getting the |
I would be surprised if people are using sentry cli manually. Is there a reason we need to support that? I feel the trade off here (vast majority will not opt in to this feature) indicates it's best to avoid the overhead of adding all this debug id's and extra debug_meta to events unless we know we need server side symbolication or source context. We could also get source context client side if sources are embedded in the pdb and released with the app (server use cases) avoiding the need to upload anything. |
Let's keep this issue focused on the full-pdb debug id. Moving discussion about whether debug images should be sent or not to #2227. Thanks. |
Fixes #2221.
Also applied some refactoring and cleanup.