-
-
Notifications
You must be signed in to change notification settings - Fork 190
feat(core): Stream extension trait #1214
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
base: master
Are you sure you want to change the base?
Changes from all commits
abe6c9c
8af241c
6c52b02
dc753c9
52a238e
0f6454c
ea7d7cf
5f51ca7
fc1b9d1
2eb75b7
fd7303a
9f0b083
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,131 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::pin::Pin; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::sync::Arc; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::task::{Context, Poll}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use futures_core::Stream; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::Hub; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// A stream that binds a `Hub` to its polling. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// This activates the given hub for the duration of the inner stream's `poll_next` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// method. Users usually do not need to construct this type manually, but | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// rather use the [`StreamExt::bind_hub`] method instead. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// [`StreamExt::bind_hub`]: trait.StreamExt.html#method.bind_hub | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 15 in sentry-core/src/stream.rs
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+13
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SentryStream docs point at non-existent StreamExt Rustdoc names and links Evidence
Also found at 1 additional location
Identified by Warden · docs-review · KZT-QMP |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[derive(Debug)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub struct SentryStream<S> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hub: Arc<Hub>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stream: S, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+17
to
+20
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: If possible, I would make this type private, and adjust
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl<S> SentryStream<S> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Creates a new bound stream with a `Hub`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn new(hub: Arc<Hub>, stream: S) -> Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Self { hub, stream } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl<S> Stream for SentryStream<S> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| where | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| S: Stream, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type Item = S::Item; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let hub = self.hub.clone(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // https://doc.rust-lang.org/std/pin/index.html#pinning-is-structural-for-field | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let stream = unsafe { self.map_unchecked_mut(|s| &mut s.stream) }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is possible to avoid this direct usage of |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(feature = "client")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let _guard = crate::hub_impl::SwitchGuard::new(hub); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stream.poll_next(cx) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(not(feature = "client"))] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let _ = hub; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stream.poll_next(cx) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Stream extensions for Sentry. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub trait SentryStreamExt: Sized { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Binds a hub to this stream. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// This ensures that the stream is polled within the given hub. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn bind_hub<H>(self, hub: H) -> SentryStream<Self> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| where | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| H: Into<Arc<Hub>>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SentryStream { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stream: self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hub: hub.into(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+52
to
+66
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to adjust this trait a bit in order to be able to return
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl<S> SentryStreamExt for S where S: Stream {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(all(test, feature = "test"))] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mod tests { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::test::with_captured_events; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::{capture_error, capture_message, configure_scope, Hub, Level, SentryStreamExt}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use futures::StreamExt; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use tokio::runtime::Runtime; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[derive(Debug)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| struct TestError(&'static str); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl std::fmt::Display for TestError { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| write!(f, "{}", self.0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl std::error::Error for TestError {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn test_streams() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut events = with_captured_events(|| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let runtime = Runtime::new().unwrap(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Two real streams, each bound to its own hub. The work inside each | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // stream runs during `poll_next`, so the captured errors must end up | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // tagged with the scope of the hub the stream was bound to. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| runtime.block_on(async { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let stream1 = futures::stream::once(async { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| configure_scope(|scope| scope.set_transaction(Some("transaction1"))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| capture_error(&TestError("oh no from 1")); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .bind_hub(Hub::new_from_top(Hub::current())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let stream2 = futures::stream::once(async { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| configure_scope(|scope| scope.set_transaction(Some("transaction2"))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| capture_error(&TestError("oh no from 2")); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .bind_hub(Hub::new_from_top(Hub::current())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stream1.collect::<Vec<_>>().await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stream2.collect::<Vec<_>>().await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| capture_message("oh hai from outside", Level::Info); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| events.sort_by(|a, b| a.transaction.cmp(&b.transaction)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(events.len(), 3); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The message captured outside any bound stream has no transaction and no | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // exception, and sorts first. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(events[0].transaction, None); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert!(events[0].exception.is_empty()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The errors captured inside `poll_next` carry the scope of their bound | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // hub and the expected exception payload. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(events[1].transaction, Some("transaction1".into())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(events[1].exception[0].value, Some("oh no from 1".into())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(events[2].transaction, Some("transaction2".into())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(events[2].exception[0].value, Some("oh no from 2".into())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
h: We should avoid adding dependencies when there is not a strong reason why we need the dependency.
In this case, I am not seeing a strong reason for
sentry-coreto depend onfutures-core.I see a few alternatives that would avoid this unconditional dependency:
sentry-futures, kinda like we do for integrations. The crate itself could perhaps later be evolved into a full-fledged integration.futures-corewould be an optional dependency activated by that flag. If we go with this path, I would probably expose the trait insentry, notsentry-core, unless there's a reason why it needs to be insentry-core.