Conversation
| Probe::QadIpv4 | Probe::QadIpv6 => unreachable!("must not be used"), | ||
| } | ||
| }; | ||
| debug!(?report, "probe finished"); |
There was a problem hiding this comment.
we're logging "starting probe" above, so I think a finished log at same level makes sense, otherwise the span is kinda pointless.
There was a problem hiding this comment.
I'm dubious on this one. Isn't the probe report already logged or something?
There was a problem hiding this comment.
We print the final report from all probes, but not the results of individual probes, this we so far only do for QAD probes if I'm not missing something. Changed it so that the "probe started" are trace logs and the "probe finished" ones remain debug.
| { | ||
| Poll::Ready(Ok(())) => Poll::Ready(Ok(())), | ||
| Poll::Ready(Ok(())) => { | ||
| trace!( |
There was a problem hiding this comment.
we log received datagrams at trace level, but didn't log sent transmits at all. this adds a trace log to mirror the receive side.
iroh/src/socket/transports.rs
Outdated
| } | ||
|
|
||
| impl<'a> Transmit<'a> { | ||
| pub(crate) fn datagram_count(&self) -> usize { |
There was a problem hiding this comment.
used in the new trace log below
There was a problem hiding this comment.
Doesn't need to be pub(crate) though, does it?
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3988/docs/iroh/ Last updated: 2026-03-03T09:22:52Z |
|
|
||
| impl Socket { | ||
| /// Creates a [`Socket`] listening. | ||
| pub(crate) async fn spawn(opts: Options) -> Result<EndpointInner, BindError> { |
There was a problem hiding this comment.
not strictly related to this PR, but Socket::spawn -> EndpointInner is weird, isn't it. moved it to impl EndpointInner
There was a problem hiding this comment.
Makes sense, I assume this code structure made more sense many refactors ago :)
1d72884 to
08ceaac
Compare
c6eadfe to
d0e10be
Compare
flub
left a comment
There was a problem hiding this comment.
Nice, thanks for picking this up!
| .instrument(Span::current()) | ||
| .await?; | ||
| debug!( | ||
| id = %inner.static_config.tls_config.secret_key.public(), |
There was a problem hiding this comment.
How come id needs to be added here? Is this a different one than the one from the endpoint span you just added?
There was a problem hiding this comment.
Here it's printed in full, not just the first 8 chars from fmt_short. I thought it would be good to somewhere log the full endpoint id, and this seemed like a fitting place.
| Probe::QadIpv4 | Probe::QadIpv6 => unreachable!("must not be used"), | ||
| } | ||
| }; | ||
| debug!(?report, "probe finished"); |
There was a problem hiding this comment.
I'm dubious on this one. Isn't the probe report already logged or something?
| /// | ||
| /// [`Poll::Pending`]: std::task::Poll::Pending | ||
| #[instrument(skip_all)] | ||
| #[instrument(skip_all, parent = self.sock.span.clone())] |
There was a problem hiding this comment.
Oh, i wonder if this should be on more of the Endpoint public API. Anyway, that'd be fore another PR.
There was a problem hiding this comment.
Yeah that's the one that stood out when looking at the logs. We can add more once we spot them.
|
|
||
| impl Socket { | ||
| /// Creates a [`Socket`] listening. | ||
| pub(crate) async fn spawn(opts: Options) -> Result<EndpointInner, BindError> { |
There was a problem hiding this comment.
Makes sense, I assume this code structure made more sense many refactors ago :)
iroh/src/socket/transports.rs
Outdated
| } | ||
|
|
||
| impl<'a> Transmit<'a> { | ||
| pub(crate) fn datagram_count(&self) -> usize { |
There was a problem hiding this comment.
Doesn't need to be pub(crate) though, does it?
Description
endpoint{id=id_short}info-level span when binding the endpointBreaking Changes
Notes & open questions
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme