Conversation
This comment has been minimized.
This comment has been minimized.
| 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) }; |
There was a problem hiding this comment.
It is possible to avoid this direct usage of unsafe (and the one in SentryFuture) by adding a dependency to pin-project-lite.
|
@lcian what is the status with this PR? Is it abandoned or are you still planning to finish it and get it ready for review? |
|
@szokeasaurusrex it's ready for review now, please take a look whenever you have time. I don't need this at the moment but it came up a couple times, so I think it would be a nice utility to have. |
| /// rather use the [`StreamExt::bind_hub`] method instead. | ||
| /// | ||
| /// [`StreamExt::bind_hub`]: trait.StreamExt.html#method.bind_hub |
There was a problem hiding this comment.
SentryStream docs point at non-existent StreamExt
Rustdoc names and links StreamExt::bind_hub, but the public trait is SentryStreamExt; update the prose and intra-doc link to match.
Evidence
SentryStreamdocs say users should call[StreamExt::bind_hub]and linktrait.StreamExt.html#method.bind_hub.- The exported extension trait is
pub trait SentryStreamExtwithfn bind_hub, re-exported fromsentry-core/src/lib.rsasSentryStreamExt. - No public
StreamExtexists in this crate, so the rendered docs name and link are wrong.
Also found at 1 additional location
sentry-core/src/lib.rs:137
Identified by Warden · docs-review · KZT-QMP
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Hey @lcian, apologies that it took me so long to get around to this.
I am requesting changes because this adds a dependency on futures-core to sentry-core, and we should try to avoid that 🙏 more details on inline comment
| metrics = [] | ||
|
|
||
| [dependencies] | ||
| futures-core = { workspace = true } |
There was a problem hiding this comment.
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-core to depend on futures-core.
I see a few alternatives that would avoid this unconditional dependency:
- It seems we could probably provide the extension trait in a separate crate; perhaps we could name it
sentry-futures, kinda like we do for integrations. The crate itself could perhaps later be evolved into a full-fledged integration. - Alternatively, we can put the trait behind a feature-flag. Then,
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.
| pub struct SentryStream<S> { | ||
| hub: Arc<Hub>, | ||
| stream: S, | ||
| } |
There was a problem hiding this comment.
m: If possible, I would make this type private, and adjust SentryStreamExt::bind_hub to return impl Stream<Self::Item>.
| pub struct SentryStream<S> { | |
| hub: Arc<Hub>, | |
| stream: S, | |
| } | |
| struct SentryStream<S> { | |
| hub: Arc<Hub>, | |
| stream: S, | |
| } |
| /// 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(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
You need to adjust this trait a bit in order to be able to return impl Stream
| /// 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(), | |
| } | |
| } | |
| } | |
| /// Stream extensions for Sentry. | |
| pub trait SentryStreamExt: Sized + Stream { | |
| /// Binds a hub to this stream. | |
| /// | |
| /// This ensures that the stream is polled within the given hub. | |
| fn bind_hub<H>(self, hub: H) -> impl Stream<Item = Self::Item> | |
| where | |
| H: Into<Arc<Hub>>, | |
| { | |
| SentryStream { | |
| stream: self, | |
| hub: hub.into(), | |
| } | |
| } | |
| } |
Description
This adds an extension trait for
futures_core::Streamthat can be used to.bind_hub(...)on a stream, much like the existing machinery we have for futures.This came up a few times already, and it happens to be one of the ways one could work around #1208 (given that the problematic response body is a stream, one can use such a stream wrapper to bind the current hub before converting that stream to a response -- I still think a proper separate fix should be introduced in the
sentry-towerinstrumentation though, as the user should not have to remember to do this manually. This is a more general construct that can be used in other scenarios too.