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: Add scope listener API #469

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

timfish
Copy link
Contributor

@timfish timfish commented May 26, 2022

I've been messing around with the minidumper and crash-handler crates and now I seem to have minidumps being captured in pure Rust and sent via Sentry Rust.

Minidumps are sent from a separate process for reliability but this means for now they are missing all the scope context from the main app process. Adding a "scope updater listener" API would allow subscribing to scope updates and passing them to the crash reporter process to be included with minidump submissions.

I haven't checked all the SDKs, but in the JavaScript SDK there is an scope.addScopeListener() method that lets you subscribe to scope updates. We've made use of this in the Electron SDK to intercept, send and synchronise scope between the multiple processes.

This is in no way a finished PR, more a "is this likely to be accepted" and if so, "am I going in the right direction" PR 😊

@codecov-commenter
Copy link

Codecov Report

Merging #469 (4e9209b) into master (02c6775) will decrease coverage by 0.11%.
The diff coverage is 46.87%.

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
- Coverage   80.53%   80.41%   -0.12%     
==========================================
  Files          73       73              
  Lines        8475     8502      +27     
==========================================
+ Hits         6825     6837      +12     
- Misses       1650     1665      +15     

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.

This is a good start.
What makes this a bit challenging is the fact that the Rust SDK has multiple hubs.

So this change by itself is nice, but the bigger things around it need to be considered if you were to attach the scope to a crash event. Which scope? Essentially the one for the "currently active Hub". But you need that info somehow. If you have that, you might as well access the hub/scope directly? Otherwise you have to stash all kinds of info away "somewhere" and get it using some kind of unique ID based on the currently active Hub.

Also if you work with updates instead of snapshots, it might become a bit complicated when we take Hub cloning / Scope pushing into account.

pub enum ScopeUpdate {
AddBreadcrumb(Breadcrumb),
ClearBreadcrumbs,
User(Option<User>),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
User(Option<User>),
SetUser(Option<User>),

sentry-core/src/scope/real.rs Outdated Show resolved Hide resolved
@timfish
Copy link
Contributor Author

timfish commented May 30, 2022

it might become a bit complicated when we take Hub cloning / Scope pushing into account.

I do agree that this kind of API is at odds with how hubs and the scope stack works. It also completely ignores sessions and transactions!

the bigger things around it need to be considered if you were to attach the scope to a crash event. Which scope?

Yes that is a problem. As with the Electron SDK, minidump submission occurs from another process and the async nature of IPC + minidump generation means that any synced scope is not guaranteed to match up correctly with when the event is captured.

I double checked the JavaScript scope.addScopeListener() usage and it looks like it's only used for Electron but I could have missed something somewhere else. sentry-dotnet has a similar IScopeObserver API which it says is "for the sync. of Scopes across SDKs" and further searching suggests it's used for the Unity SDK.

So it appears this is a common pattern used to sync scope between SKDs or processes but in all cases it appears to ignore the scope stack and anything but the global hub!

If you have that, you might as well access the hub/scope directly?

Making the scope fields public would allow reading the scope directly, but it's still handy to know when the scope has changed and even handier to know exactly what changed.

@Swatinem
Copy link
Member

Making the scope fields public would allow reading the scope directly

I wouldn’t make the fields public. Due to the real/noop scope optimization, that is impossible to begin with ;-)

Rather you could apply the scope to an (empty) event, since thats essentially what you want to achieve.

So yes, all in all this API is perfectly fine, its just hard to see how it can be used in a meaningful way.

@timfish
Copy link
Contributor Author

timfish commented May 31, 2022

I wouldn’t make the fields public. Due to the real/noop scope optimization, that is impossible to begin with ;-)

😬

So yes, all in all this API is perfectly fine, its just hard to see how it can be used in a meaningful way.

Rather than try and describe what my plans were I realised it would be much quicker to try it out!

Since I'm starting the Sentry SDK in both the main process and the crash reporting process, I planned to hook the scope changes in the main process (the client), serialise, send via IPC and then apply them to the global hub scope in the crash reporter process (the server) like this:
https://github.com/timfish/sentry-rust-minidump/pull/1/files

Then when a minidump event is sent from the crash reporter process, it will include that scope that was added via IPC messages.

Do you think the closure with single enum argument is the right approach, or might it be better with a boxed trait with methods matching the scope methods? This would be closer to the .NET implementation.

pub trait ScopeUpdates {
  fn add_breadcrumb(breadcrumb: &Breadcrumb) { } 
  fn clear_breadcrumbs() { } 
  // etc etc
}

@Swatinem
Copy link
Member

Swatinem commented Jun 1, 2022

I think both have pros/cons.

The reporter process looks good, but the problem stands that you only do a single configure_scope, so you only have updates for one of the potentially many scopes in your process.

Let @mitsuhiko weigh in as well. There was talk some time ago about re-thinking the hub/scope model a bit, though that seemed to have stalled. The other thing that might be relevant here is the general concept of "sidecar".

@timfish
Copy link
Contributor Author

timfish commented Jun 28, 2022

the problem stands that you only do a single configure_scope, so you only have updates for one of the potentially many scopes in your process.

I suppose these same limitations already apply to the existing scope.add_event_processor() API.

I can see that it's possible to create isolated hub/scope but with the documented usage of the SDK, under what circumstances would you actually end up with a scope that wasn't cloned from the top of the scope stack? For example, the thread-demo.rs uses Hub::new_from_top so it wouldn't be an issue if you follow that pattern.

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.

3 participants