feat(difs): Unwind info in dif candidates #324
Conversation
Type aliases are just not that good, sorry clippy.
Instructions and example for changelogPlease add an entry to
To the changelog entry, please add a link to this PR (consider a more descriptive message): - Unwind info in dif candidates. ([#324](https://github.com/getsentry/symbolicator/pull/324))If none of the above apply, you can opt out by adding #skip-changelog to the PR description. Instructions for snapshot changesSentry runs a symbolicator integration test suite located at Follow these steps to update snapshots in Sentry:
|
src/actors/cficaches.rs
Outdated
| CacheStatus::Malformed | ||
| } | ||
| }, | ||
| cache_status => cache_status, |
There was a problem hiding this comment.
I preferred the former structure as it was easier to read and had less indentation.
There was a problem hiding this comment.
See the comment above, replacing the old code again.
The extra level of indentation was because the entire thing is wrapped in another async move as the replacement of the normal async fn body. But anyway.
src/actors/cficaches.rs
Outdated
| .await | ||
| { | ||
| Ok(result) => result, | ||
| Err(_) => Err(CfiCacheError::Canceled), |
There was a problem hiding this comment.
nit: Rewrite this match statement as .map_err(|_| CfiCacheError::Canceled).
There was a problem hiding this comment.
.unwrap_or_else() really. The threadpool returns the return of the spawned future wrapped in a result with cancellation in the error case. But this was reverted.
| }; | ||
|
|
||
| Ok(status) |
There was a problem hiding this comment.
This future does not have to return a result, you should be able to return status directly.
There was a problem hiding this comment.
Err, yes this needs to be a result? It's the final result in the return type.
src/actors/cficaches.rs
Outdated
| let result = async move { | ||
| let object_handle = objects_actor | ||
| .fetch(meta_handle.clone()) | ||
| .await |
There was a problem hiding this comment.
this await here is the reason you have two nested futures. Only the inner one runs in the threadpool. Is that intentional? Maybe add a comment explaining the reasoning here.
There was a problem hiding this comment.
So the idea was that the outer async move is essentially the body of the async fn that this actually is, but it isn't because it's a trait method. But this is all muddled because we need to spawn onto a new threadpool which means everything needs to be 'static, plus the future_metrics! macro doesn't yet have an idiomatic replacement.
Given that there are two reviews saying this all got more complicated I'm revering this back to the old code, only replacing future::lazy with async move instead.
Seems like no-one (other than me) found the new code more readable. It's a bit unfortunate that everything needs to be owned because it is all spawned on a new threadpool, thus the new code doesn't become much nicer. Plust the future_metrics! macro didn't have an idiomatic async-await substitute yet which doesn't help as it leaves everything still in a half-way state.
It's part more confusing, part less confusing. Oh well.
jan-auer
changed the title
This adds the unwind info to dif candidates. For this the cficache actor also exposes DIF info and the symbolication actor now correctly merges the various DIF candidates info together.
This adds the unwind info to dif candidates. For this the cficache actor
also exposes DIF info and the symbolication actor now correctly merges
the various DIF candidates info together.
sentry snapshots do not need updating