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 Device/Unity context to native events #650

Closed
bruno-garcia opened this issue Mar 18, 2022 · 5 comments · Fixed by #747
Closed

Add Device/Unity context to native events #650

bruno-garcia opened this issue Mar 18, 2022 · 5 comments · Fixed by #747
Assignees
Labels
Feature New feature or request Native

Comments

@bruno-garcia
Copy link
Member

Data that's applied to the event through the Unity layer (UnityEventProcessor) should also be available in crashes from native layers.

This is gap is more visible in Windows (minidump) crashes which don't have the rich device info that comes on iOS and Android crashes.

@bitsandfoxes
Copy link
Contributor

Possible solution: Capturing "what we can" and things that don't change (i.e. not memory usage) and set it to the Scope as context and pass it down to the Native Layer.

@vaind
Copy link
Collaborator

vaind commented May 12, 2022

RFC: I'm not 100 % sure about the approach for this... What I've started in #747 is make a ContextWriter that can take any of the App, GPU, etc. context objects and forward it to a native implementation (these are not yet implemented in the PR, but I'd create one at least for sentry-native, and likely also for java & cocoa). I want to call this during SentryUnity.Init() but that requires contexts to be already populated. I was thinking of splitting up the UnityEventProcessor functionality, with the "static" info being configured at init() using ConfigureScope and only the few remaining dynamic properties would be added to events on demand (i.e. EventProcessor). What do you think @bitsandfoxes and @bruno-garcia ?

@bitsandfoxes
Copy link
Contributor

bitsandfoxes commented May 13, 2022

I was thinking of splitting up the UnityEventProcessor functionality, with the "static" info being configured at init() using ConfigureScope and only the few remaining dynamic properties would be added to events on demand (i.e. EventProcessor)

That means everything "static" from #744 makes it to the native layer?
I'm just wary about us setting them once in combination with scope.Contexts.Clear();

@vaind
Copy link
Collaborator

vaind commented May 13, 2022

I'm just wary about us setting them once in combination with scope.Contexts.Clear();

I don't think there's anything we can do at the time of an event. Even though there are callback hooks we could assign, I don't feel confident doing c# interop calls from native code at the time of a crash - I wouldn't be confident they're signal-safe, which is a requirement because they run in a signal handler at the time of a crash.

Therefore, the only option I see is setting the contexts on the scope. Also, if we want to, we could make notifications on some context changes, like adding new ones, clearing, etc. But changing properties on existing contexts would still go unnoticed...

Overall, this all feels like expanding the scope of the issue, which is about setting "Device/Unity" context to native events - those that don't change.

@vaind
Copy link
Collaborator

vaind commented May 13, 2022

To get around the cost of early init & native sync, even though it's likely negligible, we could have it run in a coroutine. For native error context sync, that should be just fine even with a slight delay (unlike for dotnet errors where it was more noticeable, #744)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request Native
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants