search

craftytrickster / stubborn-io

io traits/structs for tokio that automatically recover from potential disconnections/interruptions
https://docs.rs/stubborn-io
MIT License
68 stars 17 forks source link

Error/Info messages don't show the peer #23

Open fredclausen opened 2 years ago

fredclausen commented 2 years ago

It would be useful to have the error/info messages show the peer's information in the message to help diagnostics.

Something like:

[INFO] - Will re-perform initial connect attempt #1 to 127.0.0.1:5555 in 5s.

Thank you for this excellent crate!

craftytrickster commented 2 years ago

Thank you for the kind words, it's always a nice surprise to find out someone other than me actually uses this!

As for your request, it is a very specific message tied to a TcpStream connection. As you can see in: https://github.com/craftytrickster/stubborn-io/blob/ddb3babc195638c7b06d4dd1fda99d34594b98bb/src/tokio/io.rs#L153-L156 the actual logic that handles these messages is generic, and not tied to TcpStreams in particular. Therefore, I would not want to add IP Address specific logic there.

However, one way to help your needs would be to expose the actual Error on the error callbacks https://github.com/craftytrickster/stubborn-io/blob/1c8eeec1a3321b8ab50678e804c24850f4e9a3d1/src/config.rs#L22-L25 , which would then allow the user (you) to log whatever message they want.

This would be a breaking change, since right now the callbacks take no args. Also, it is unclear to me if the attempt number should be another callback arg, ex: (e: Box<dyn Error>, attempt_count: u32), I'd have to think about it.

Even in the present state, you could set it to mention the IP address yourself in the error callbacks from the config object if that is valuable. What do you think?

fredclausen commented 2 years ago

Thank you for the kind words, it's always a nice surprise to find out someone other than me actually uses this!

I'm very new to rust and your crate happened to nicely fill a need I had for my little application. Seriously made my life much easier.

the actual logic that handles these messages is generic, and not tied to TcpStreams in particular. Therefore, I would not want to add IP Address specific logic there.

I hadn't considered that, and it seems so obvious. Specifically addressing IP logic there would obviously not work.

However, one way to help your needs would be to expose the actual Error on the error callbacks , which would then allow the user (you) to log whatever message they want.

This seems like a nice way of handling it, but I understand the need to be circumspect about breaking changes. Also, unless the "generic" messages are removed or optionally muted, you'd still end up with them in the logs.

Even in the present state, you could set it to mention the IP address yourself in the error callbacks from the config object if that is valuable. What do you think?

That would be the best option in the present state, although as I mentioned above the logs would still have the "generic" messages in them.

In any case, I appreciate you taking the time to consider this request.

craftytrickster commented 2 years ago

There could be some value to making the "generic" messages that I log myself as optional (via a config boolean). However, I personally think they occur infrequently enough that the fear of "spamming" a logfile seems fairly low risk.

Having said that, at the very least, next time I make a breaking change, I will add that error info to the callbacks as I suggested.