search

YoEight / eventstore-rs

Rust GetEventStore TCP Client
MIT License
46 stars 18 forks source link

Implement std::fmt::Debug for eventstore::Connection #36

Open wingertge opened 4 years ago

wingertge commented 4 years ago

I'm trying to use the tracing crate to monitor my functions that access eventstore and detect any potential bottlenecks, but its instrumentation requires all parameters to implement fmt::Debug. Connection doesn't implement that right now, forcing me to wrap and unwrap the connection to enable proper instrumentation. Implementing Debug on the Connection struct would solve this issue.

I'm not making a pull request because I'm not sure about the internals of the crate, so I don't know what should be output in the Debug fmt.

YoEight commented 4 years ago

Hi @wingertge,

Thanks for reporting this issue. I see nothing preventing an std::fmt::Debug implementation. I can cut a new release ASAP if needed.

wingertge commented 4 years ago

The project is still in very early stages and have a workaround for now so no rush. I'm waiting for futures-0.3 update as well (tried updating it myself but I'm not super familiar with Rust yet and got hung up on lifetime stuff). So as long as that change is in the futures-0.3 release I'm good.

YoEight commented 4 years ago

I’m working on the futures 0.3 migration. Still a lot to do but at least, I have no blocker, unlike 2 weeks ago.

wingertge commented 4 years ago

Just a quick update, I'm not sure if this is possible with the way the architecture works, but if the Debug output could log the connection type (single/cluster), as well as the SocketAddr of the current node, that would be ideal. Knowing the exact server can really help for tracing in distributed services.

If I remember the code correctly this might mean implementing Debug for the Driver as well.

YoEight commented 4 years ago

I could easily expose if a connection is in single-node or cluster mode. However, getting the current SocketAddr could be challenging. Will see.

wingertge commented 4 years ago

I can also just do a quick PR once you're done with new-futures. Btw I'm already using that branch (I don't use subscriptions atm) and it's working perfectly in a real world scenario 👍

YoEight commented 4 years ago

@wingertge new-futures got merged. Feel free to propose a patch or give me a list of what you'll like to see in the Connection itself.