Add support for downcasting Instrumentation #4129
Conversation
…l dyn dispatch level we actually returned &mut Option<Box<dyn Instrumentation>> as &mut dyn Instrumentation, so downcasting would have to be done in these two steps, which is counter-intuitive and doesn't seem ideal inside diesel itself either.
Ten0
force-pushed
the
instrumentation_downcast_support
branch
from
5500029
to
f5a316a
Compare
| @@ -242,10 +242,11 @@ impl<'a> InstrumentationEvent<'a> { | |||
| /// More complex usages and integrations with frameworks like | |||
| /// `tracing` and `log` are supposed to be part of their own | |||
| /// crates. | |||
| pub trait Instrumentation: Send + 'static { | |||
| pub trait Instrumentation: Downcast + Send + 'static { | |||
There was a problem hiding this comment.
I'm not sure if that's a breaking change or not.
On the one hand it adds a new super trait without a wild card impl for all types on the other hand Downcast is implemented for all T: Any and any is implemented by the compiler for all types that are 'static (we already have that bound) so it's likely not breaking?
There was a problem hiding this comment.
on the other hand Downcast is implemented for all T: Any and any is implemented by the compiler for all types that are 'static (we already have that bound)
Yes, that's why I thought that it was very unlikely to break anyone 😀
Also I've checked that this dependency is trustworthy and very lightweight.
| /// The function that is invoced for each event | ||
| fn on_connection_event(&mut self, event: InstrumentationEvent<'_>); | ||
| } | ||
| downcast_rs::impl_downcast!(Instrumentation); |
There was a problem hiding this comment.
We should use that for BoxableConnection as well if we add this dependency.
(I can address that in a followup PR)
|
Thanks for the extremely quick review! 😊🙏 |
This is useful to instrument additional events related to the connection.
My use-case is that I want to check that
StartQuerys were consistent withFinishQuerys when releasing the connection to the Pool (which I already have a type wrapper ofr2d2::PooledConnectionwith itsDropimplementation for), and I also want to clean up the instrumentation object at the same time.The
DynInstrumentationtype is useful because without it we actually did tend to return&mut Option<Box<dyn Instrumentation>> as &mut dyn Instrumentationfromconnection.instrumentation(), so downcasting would have to be done in these two steps by the user, which is counter-intuitive and doesn't seem ideal inside diesel itself either (because it induces extra dyn dispatch levels).An alternate implementation would be to just make
()implementInstrumentationand use Box always (Box of ZST doesn't allocate). This means we would always dyn dispatch even if instrumentation is disabled, but maybe the complexity/cost ratio is good, because probably the cost is very low given the events we currently instrument.