-
-
Notifications
You must be signed in to change notification settings - Fork 54
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: Synchronize contexts from .NET to native #747
Conversation
|
c3c8e8f
to
74db263
Compare
update: I've worked around this by moving the reusable native context-writer static functions to Sentry.Unity.NativeUtils @bitsandfoxes I've tried implementing the scope sync for Android native crashes (which internally use sentry-native) by reusing the code in the Sentry.Unity.Native project. However, I'm getting some unity errors which I can't figure out - you'll likely know more: changes: 8e1d28c errors in the Unity console:
|
I've seen this before and I think I know what this is. Let me take a look. |
03fcf2f
to
7913a02
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! Maybe @bruno-garcia wants to take a look too?
} | ||
|
||
void SentryNativeBridgeWriteScope( // clang-format off | ||
// // const char *AppStartTime, |
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.
// // const char *AppStartTime, | |
// const char *AppStartTime, |
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 know it looks weird but there's a hidden logic for these... the single-comment fields should actually be added, but we cannot do that because there's no way to merge existing contexts for iOS. So if you imagine commenting out the whole block, you'll end up with the fields that are missing and should be added, but depend on something currently not possible with sentry-cocoa. The double-commented lines (which would be just commented-out after un-commenting the whole block), are already present.
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.
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.
Thanks for clarifying!
// // const char *DeviceModel, | ||
// // long DeviceMemorySize, |
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.
// // const char *DeviceModel, | |
// // long DeviceMemorySize, | |
// const char *DeviceModel, | |
// long DeviceMemorySize, |
void SentryNativeBridgeWriteScope( // clang-format off | ||
// // const char *AppStartTime, | ||
// const char *AppBuildType, | ||
// // const char *OperatingSystemRawDescription, |
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.
// // const char *OperatingSystemRawDescription, | |
// const char *OperatingSystemRawDescription, |
const char *UnityRenderingThreadingMode | ||
) // clang-format on | ||
{ | ||
// Note: we're using a NSMutableDictionary because it will skip fields with nil values. |
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 does skip
mean? It will create no entry if the value is nil
?
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.
yes
// // DeviceModel, | ||
// // DeviceMemorySize, |
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.
// // DeviceModel, | |
// // DeviceMemorySize, | |
// DeviceModel, | |
// DeviceMemorySize, |
// Additionally, there's currently no way to update existing contexts, so no more Device info for now... | ||
[DllImport("__Internal")] | ||
private static extern void SentryNativeBridgeWriteScope( | ||
// // string? AppStartTime, |
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.
// // string? AppStartTime, | |
// string? AppStartTime, |
private static extern void SentryNativeBridgeWriteScope( | ||
// // string? AppStartTime, | ||
// string? AppBuildType, | ||
// // string? OperatingSystemRawDescription, |
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.
// // string? OperatingSystemRawDescription, | |
// string? OperatingSystemRawDescription, |
// // string? DeviceModel, | ||
// // long? DeviceMemorySize, |
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.
// // string? DeviceModel, | |
// // long? DeviceMemorySize, | |
// string? DeviceModel, | |
// long? DeviceMemorySize, |
Co-authored-by: Stefan Jandl <stefan.jandl@sentry.io>
{ | ||
if (Attach()) | ||
{ | ||
using var gpu = new AndroidJavaObject("io.sentry.protocol.Gpu"); |
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.
If the type isnt' found, is this going to blow up? (proguard)
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.
yes, see #797
closes #650