Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for downcasting Instrumentation #4129

Merged
merged 6 commits into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions diesel/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ bitflags = { version = "2.0.0", optional = true }
r2d2 = { version = ">= 0.8.2, < 0.9.0", optional = true }
itoa = { version = "1.0.0", optional = true }
time = { version = "0.3.9", optional = true, features = ["macros"] }
downcast-rs = "1.2.1"

[dependencies.diesel_derives]
version = "~2.2.0"
Expand Down
100 changes: 93 additions & 7 deletions diesel/src/connection/instrumentation.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::fmt::Debug;
use std::fmt::Display;
use downcast_rs::Downcast;
use std::fmt::{Debug, Display};
use std::num::NonZeroU32;
use std::ops::DerefMut;
use std::ops::{Deref, DerefMut};

static GLOBAL_INSTRUMENTATION: std::sync::RwLock<fn() -> Option<Box<dyn Instrumentation>>> =
std::sync::RwLock::new(|| None);
Expand Down Expand Up @@ -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 {
/// The function that is invoced for each event
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@Ten0 Ten0 Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fn on_connection_event(&mut self, event: InstrumentationEvent<'_>);
}
downcast_rs::impl_downcast!(Instrumentation);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use that for BoxableConnection as well if we add this dependency.

(I can address that in a followup PR)

/// Get an instance of the default [`Instrumentation`]
///
Expand All @@ -266,9 +267,11 @@ pub fn get_default_instrumentation() -> Option<Box<dyn Instrumentation>> {
///
/// // a simple logger that prints all events to stdout
/// fn simple_logger() -> Option<Box<dyn Instrumentation>> {
/// // we need the explicit argument type there due
/// // to bugs in rustc
/// Some(Box::new(|event: InstrumentationEvent<'_>| println!("{event:?}")))
/// // we need the explicit argument type there due
/// // to bugs in rustc
/// Some(Box::new(|event: InstrumentationEvent<'_>| {
/// println!("{event:?}")
/// }))
/// }
///
/// set_default_instrumentation(simple_logger);
Expand Down Expand Up @@ -313,3 +316,86 @@ where
}
}
}

#[diesel_derives::__diesel_public_if(
feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
)]
/// An optional dyn instrumentation.
/// For ease of use, this type implements [`Deref`] and [`DerefMut`] to `&dyn Instrumentation`,
/// falling back to a no-op implementation if no instrumentation is set.
pub(crate) struct DynInstrumentation {
/// zst
weiznich marked this conversation as resolved.
Show resolved Hide resolved
no_instrumentation: NoInstrumentation,
inner: Option<Box<dyn Instrumentation>>,
}
impl Deref for DynInstrumentation {
type Target = dyn Instrumentation;
Ten0 marked this conversation as resolved.
Show resolved Hide resolved

fn deref(&self) -> &Self::Target {
self.inner.as_deref().unwrap_or(&self.no_instrumentation)
}
}
impl DerefMut for DynInstrumentation {
fn deref_mut(&mut self) -> &mut Self::Target {
Ten0 marked this conversation as resolved.
Show resolved Hide resolved
self.inner
.as_deref_mut()
.unwrap_or(&mut self.no_instrumentation)
}
}
impl DynInstrumentation {
#[diesel_derives::__diesel_public_if(
Ten0 marked this conversation as resolved.
Show resolved Hide resolved
feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
)]
pub(crate) fn default_instrumentation() -> Self {
Self {
inner: get_default_instrumentation(),
no_instrumentation: NoInstrumentation,
}
}

#[diesel_derives::__diesel_public_if(
feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
)]
pub(crate) fn none() -> Self {
Self {
inner: None,
no_instrumentation: NoInstrumentation,
}
}

#[diesel_derives::__diesel_public_if(
feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
)]
pub(crate) fn on_connection_event(&mut self, event: InstrumentationEvent<'_>) {
if let Some(inner) = self.inner.as_deref_mut() {
inner.on_connection_event(event)
}
}
}
Ten0 marked this conversation as resolved.
Show resolved Hide resolved
impl<I: Instrumentation> From<I> for DynInstrumentation {
fn from(instrumentation: I) -> Self {
Ten0 marked this conversation as resolved.
Show resolved Hide resolved
Self {
inner: Some(unpack_instrumentation(Box::new(instrumentation))),
no_instrumentation: NoInstrumentation,
}
}
}

struct NoInstrumentation;
impl Instrumentation for NoInstrumentation {
fn on_connection_event(&mut self, _: InstrumentationEvent<'_>) {}
Ten0 marked this conversation as resolved.
Show resolved Hide resolved
}

/// Unwrap unnecessary boxing levels
fn unpack_instrumentation(
mut instrumentation: Box<dyn Instrumentation>,
) -> Box<dyn Instrumentation> {
loop {
match instrumentation.downcast::<Box<dyn Instrumentation>>() {
Ok(extra_boxed_instrumentation) => instrumentation = *extra_boxed_instrumentation,
Err(not_extra_boxed_instrumentation) => {
break not_extra_boxed_instrumentation;
}
}
}
}
5 changes: 2 additions & 3 deletions diesel/src/mysql/connection/bind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ use std::ops::Index;
use std::os::raw as libc;
use std::ptr::NonNull;

use super::stmt::MysqlFieldMetadata;
use super::stmt::StatementUse;
use super::stmt::{MysqlFieldMetadata, StatementUse};
use crate::mysql::connection::stmt::StatementMetadata;
use crate::mysql::types::date_and_time::MysqlTime;
use crate::mysql::{MysqlType, MysqlValue};
Expand Down Expand Up @@ -870,7 +869,7 @@ mod tests {
),
&mut conn.statement_cache,
&mut conn.raw_connection,
&mut conn.instrumentation,
&mut *conn.instrumentation,
).unwrap();

let metadata = stmt.metadata().unwrap();
Expand Down
19 changes: 9 additions & 10 deletions diesel/src/mysql/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ use self::stmt::iterator::StatementIterator;
use self::stmt::Statement;
use self::url::ConnectionOptions;
use super::backend::Mysql;
use crate::connection::instrumentation::DebugQuery;
use crate::connection::instrumentation::StrQueryHelper;
use crate::connection::instrumentation::{DebugQuery, DynInstrumentation, StrQueryHelper};
use crate::connection::statement_cache::{MaybeCached, StatementCache};
use crate::connection::*;
use crate::expression::QueryMetadata;
Expand Down Expand Up @@ -111,7 +110,7 @@ pub struct MysqlConnection {
raw_connection: RawConnection,
transaction_state: AnsiTransactionManager,
statement_cache: StatementCache<Mysql, Statement>,
instrumentation: Option<Box<dyn Instrumentation>>,
instrumentation: DynInstrumentation,
}

// mysql connection can be shared between threads according to libmysqlclients documentation
Expand Down Expand Up @@ -156,7 +155,7 @@ impl Connection for MysqlConnection {
/// * `ssl_mode` expects a value defined for MySQL client command option `--ssl-mode`
/// See <https://dev.mysql.com/doc/refman/5.7/en/connection-options.html#option_general_ssl-mode>
fn establish(database_url: &str) -> ConnectionResult<Self> {
let mut instrumentation = crate::connection::instrumentation::get_default_instrumentation();
let mut instrumentation = DynInstrumentation::default_instrumentation();
instrumentation.on_connection_event(InstrumentationEvent::StartEstablishConnection {
url: database_url,
});
Expand All @@ -181,7 +180,7 @@ impl Connection for MysqlConnection {
&source,
&mut self.statement_cache,
&mut self.raw_connection,
&mut self.instrumentation,
&mut *self.instrumentation,
)
.and_then(|stmt| {
// we have not called result yet, so calling `execute` is
Expand All @@ -200,19 +199,19 @@ impl Connection for MysqlConnection {
}

fn instrumentation(&mut self) -> &mut dyn Instrumentation {
&mut self.instrumentation
&mut *self.instrumentation
}

fn set_instrumentation(&mut self, instrumentation: impl Instrumentation) {
self.instrumentation = Some(Box::new(instrumentation));
self.instrumentation = instrumentation.into();
}
}

#[inline(always)]
fn update_transaction_manager_status<T>(
query_result: QueryResult<T>,
transaction_manager: &mut AnsiTransactionManager,
instrumentation: &mut Option<Box<dyn Instrumentation>>,
instrumentation: &mut DynInstrumentation,
query: &dyn DebugQuery,
) -> QueryResult<T> {
if let Err(Error::DatabaseError(DatabaseErrorKind::SerializationFailure, _)) = query_result {
Expand Down Expand Up @@ -244,7 +243,7 @@ impl LoadConnection<DefaultLoadingMode> for MysqlConnection {
&source,
&mut self.statement_cache,
&mut self.raw_connection,
&mut self.instrumentation,
&mut *self.instrumentation,
)
.and_then(|stmt| {
let mut metadata = Vec::new();
Expand Down Expand Up @@ -329,7 +328,7 @@ impl MysqlConnection {
raw_connection,
transaction_state: AnsiTransactionManager::default(),
statement_cache: StatementCache::new(),
instrumentation: None,
instrumentation: DynInstrumentation::none(),
};
conn.set_config_options()
.map_err(CouldntSetupConfiguration)?;
Expand Down
1 change: 0 additions & 1 deletion diesel/src/pg/connection/cursor.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use super::raw::RawConnection;
use super::result::PgResult;
use super::row::PgRow;
use crate::connection::Instrumentation;
use crate::pg::Pg;
use crate::query_builder::QueryFragment;
use std::rc::Rc;
Expand Down
49 changes: 19 additions & 30 deletions diesel/src/pg/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@ mod result;
mod row;
mod stmt;

use self::copy::CopyFromSink;
use self::copy::CopyToBuffer;
use self::copy::{CopyFromSink, CopyToBuffer};
use self::cursor::*;
use self::private::ConnectionAndTransactionManager;
use self::raw::{PgTransactionStatus, RawConnection};
use self::stmt::Statement;
use crate::connection::instrumentation::DebugQuery;
use crate::connection::instrumentation::Instrumentation;
use crate::connection::instrumentation::StrQueryHelper;
use crate::connection::instrumentation::{
DebugQuery, DynInstrumentation, Instrumentation, StrQueryHelper,
};
use crate::connection::statement_cache::{MaybeCached, StatementCache};
use crate::connection::*;
use crate::expression::QueryMetadata;
Expand All @@ -29,9 +28,7 @@ use std::ffi::CString;
use std::fmt::Debug;
use std::os::raw as libc;

use super::query_builder::copy::CopyFromExpression;
use super::query_builder::copy::CopyTarget;
use super::query_builder::copy::CopyToCommand;
use super::query_builder::copy::{CopyFromExpression, CopyTarget, CopyToCommand};

pub(super) use self::result::PgResult;

Expand Down Expand Up @@ -178,7 +175,7 @@ impl Connection for PgConnection {
type TransactionManager = AnsiTransactionManager;

fn establish(database_url: &str) -> ConnectionResult<PgConnection> {
let mut instrumentation = crate::connection::instrumentation::get_default_instrumentation();
let mut instrumentation = DynInstrumentation::default_instrumentation();
instrumentation.on_connection_event(InstrumentationEvent::StartEstablishConnection {
url: database_url,
});
Expand All @@ -187,7 +184,7 @@ impl Connection for PgConnection {
connection_and_transaction_manager: ConnectionAndTransactionManager {
raw_connection: raw_conn,
transaction_state: AnsiTransactionManager::default(),
instrumentation: None,
instrumentation: DynInstrumentation::none(),
},
statement_cache: StatementCache::new(),
metadata_cache: PgMetadataCache::new(),
Expand Down Expand Up @@ -233,11 +230,11 @@ impl Connection for PgConnection {
}

fn instrumentation(&mut self) -> &mut dyn Instrumentation {
&mut self.connection_and_transaction_manager.instrumentation
&mut *self.connection_and_transaction_manager.instrumentation
}

fn set_instrumentation(&mut self, instrumentation: impl Instrumentation) {
self.connection_and_transaction_manager.instrumentation = Some(Box::new(instrumentation));
self.connection_and_transaction_manager.instrumentation = instrumentation.into();
}
}

Expand Down Expand Up @@ -517,7 +514,7 @@ impl PgConnection {
};
Statement::prepare(conn, sql, query_name.as_deref(), &metadata)
},
&mut self.connection_and_transaction_manager.instrumentation,
&mut *self.connection_and_transaction_manager.instrumentation,
);
if !execute_returning_count {
if let Err(ref e) = query {
Expand Down Expand Up @@ -557,7 +554,7 @@ mod private {
pub struct ConnectionAndTransactionManager {
pub(super) raw_connection: RawConnection,
pub(super) transaction_state: AnsiTransactionManager,
pub(super) instrumentation: Option<Box<dyn Instrumentation>>,
pub(super) instrumentation: DynInstrumentation,
}

pub trait PgLoadingMode<B> {
Expand Down Expand Up @@ -800,8 +797,7 @@ mod tests {

#[test]
fn transaction_manager_returns_an_error_when_attempting_to_commit_outside_of_a_transaction() {
use crate::connection::AnsiTransactionManager;
use crate::connection::TransactionManager;
use crate::connection::{AnsiTransactionManager, TransactionManager};
use crate::result::Error;
use crate::PgConnection;

Expand All @@ -818,8 +814,7 @@ mod tests {

#[test]
fn transaction_manager_returns_an_error_when_attempting_to_rollback_outside_of_a_transaction() {
use crate::connection::AnsiTransactionManager;
use crate::connection::TransactionManager;
use crate::connection::{AnsiTransactionManager, TransactionManager};
use crate::result::Error;
use crate::PgConnection;

Expand All @@ -838,8 +833,7 @@ mod tests {
fn postgres_transaction_is_rolled_back_upon_syntax_error() {
use std::num::NonZeroU32;

use crate::connection::AnsiTransactionManager;
use crate::connection::TransactionManager;
use crate::connection::{AnsiTransactionManager, TransactionManager};
use crate::pg::connection::raw::PgTransactionStatus;
use crate::*;
let conn = &mut crate::test_helpers::pg_connection_no_transaction();
Expand Down Expand Up @@ -883,8 +877,7 @@ mod tests {
fn nested_postgres_transaction_is_rolled_back_upon_syntax_error() {
use std::num::NonZeroU32;

use crate::connection::AnsiTransactionManager;
use crate::connection::TransactionManager;
use crate::connection::{AnsiTransactionManager, TransactionManager};
use crate::pg::connection::raw::PgTransactionStatus;
use crate::*;
let conn = &mut crate::test_helpers::pg_connection_no_transaction();
Expand Down Expand Up @@ -1165,8 +1158,7 @@ mod tests {

#[test]
fn postgres_transaction_is_rolled_back_upon_deferred_constraint_failure() {
use crate::connection::AnsiTransactionManager;
use crate::connection::TransactionManager;
use crate::connection::{AnsiTransactionManager, TransactionManager};
use crate::pg::connection::raw::PgTransactionStatus;
use crate::result::Error;
use crate::*;
Expand Down Expand Up @@ -1215,8 +1207,7 @@ mod tests {

#[test]
fn postgres_transaction_is_rolled_back_upon_deferred_trigger_failure() {
use crate::connection::AnsiTransactionManager;
use crate::connection::TransactionManager;
use crate::connection::{AnsiTransactionManager, TransactionManager};
use crate::pg::connection::raw::PgTransactionStatus;
use crate::result::Error;
use crate::*;
Expand Down Expand Up @@ -1291,8 +1282,7 @@ mod tests {

#[test]
fn nested_postgres_transaction_is_rolled_back_upon_deferred_trigger_failure() {
use crate::connection::AnsiTransactionManager;
use crate::connection::TransactionManager;
use crate::connection::{AnsiTransactionManager, TransactionManager};
use crate::pg::connection::raw::PgTransactionStatus;
use crate::result::Error;
use crate::*;
Expand Down Expand Up @@ -1374,8 +1364,7 @@ mod tests {

#[test]
fn nested_postgres_transaction_is_rolled_back_upon_deferred_constraint_failure() {
use crate::connection::AnsiTransactionManager;
use crate::connection::TransactionManager;
use crate::connection::{AnsiTransactionManager, TransactionManager};
use crate::pg::connection::raw::PgTransactionStatus;
use crate::result::Error;
use crate::*;
Expand Down
Loading
Loading