feat: log the number of read/written bytes on IMAP stream read error #6924
Conversation
link2xt
force-pushed
the
link2xt/logging-stream
branch
from
2893f40 to
8efff30
Compare
link2xt
force-pushed
the
link2xt/logging-stream
branch
3 times, most recently
from
7ce86bd to
f6df3ee
Compare
|
First commit currently adds |
link2xt
force-pushed
the
link2xt/logging-stream
branch
2 times, most recently
from
01d9db2 to
5ca9bb1
Compare
|
Logs look like this: Now right before the message about IMAP timeout we get a log message with the peer address and the number of read and written bytes. |
link2xt
changed the title
link2xt
force-pushed
the
link2xt/logging-stream
branch
from
5ca9bb1 to
932a8e1
Compare
link2xt
force-pushed
the
link2xt/logging-stream
branch
from
932a8e1 to
ff537b8
Compare
Hocuri
left a comment
Hocuri
left a comment
There was a problem hiding this comment.
I read through the code and found 3 minor things, otherwise looks good. But I don't know this code, so, I can's say for sure that I didn't overlook anything - please be sure to double-check yourself, because this is an area where it would be bad to have a bug.
src/log/logging_stream.rs
Outdated
| if let Ok(peer_addr) = peer_addr { | ||
| let log_message = format!( |
There was a problem hiding this comment.
Is it on purpose that nothing is logged if the peer_addr is Err?
There was a problem hiding this comment.
This should be impossible, I think to have unbound socket you need to convert it from raw FD. But I'll add a warning in this case and debug_assert.
link2xt
force-pushed
the
link2xt/logging-stream
branch
from
ff537b8 to
7f0841b
Compare
src/log.rs
Outdated
|
|
||
| use crate::context::Context; | ||
|
|
||
| mod logging_stream; |
There was a problem hiding this comment.
I'd rename it to just stream. Though doesn't matter much because LoggingStream is re-exported
src/log/logging_stream.rs
Outdated
| if let Poll::Ready(Err(ref err)) = res { | ||
| debug_assert!( | ||
| peer_addr.is_ok(), | ||
| "Logging stream should be created over bound sockets" |
There was a problem hiding this comment.
| "Logging stream should be created over bound sockets" | |
| "Logging stream should be created over a bound socket" |
src/log/logging_stream.rs
Outdated
| } | ||
|
|
||
| let n = old_remaining - buf.remaining(); | ||
| if n > 0 { |
There was a problem hiding this comment.
What is the purpose if this check, can n be < 0?
src/net/session.rs
Outdated
| let addr = self.as_ref().peer_addr()?; | ||
| Ok(addr) |
There was a problem hiding this comment.
Can be shortened to self.as_ref().peer_addr() probably
LoggingStreamis a wrapper for IMAP/SMTP/HTTP session streams. Originally I started building it with the idea to debug #6477 and similar issues where IMAP loop gets stuck with the hope that right before the loop gets stuck or even continuously there are socket errors like a read timeout that are not treated correctly.This can later be expanded to collect the metrics in the database, display them in the connectivity view, but not in this PR. Same for logging the write or flush errors, I have not added it for now because they are less common.