Repository: diesel-rs/diesel (language: rust)

Issue:
Missing transaction instrumentation in `AnsiTransactionManager`
<!--
If you want to report a bug, we added some points below which help us track down the problem faster.
-->

## Setup

### Versions

- **Rust:** 1.83.0
- **Diesel:** 2.2.6
- **Database:** SQLite
- **Operating System** Linux

### Feature Flags

- **diesel:** `r2d2`, `returning_clauses_for_sqlite_3_35`, `sqlite`, `uuid`

## Problem Description

The following problem happens with `SqliteConnection`'s methods `immediate_transaction()` and `exclusive_transaction()`. Under the hood, these use `AnsiTransactionManager::begin_transaction_sql()` instead of the usual `begin_transaction()` (implemented for trait `TransactionManager`).

The latter method's implementation generates instrumentation events of type `InstrumentationEvent::BeginTransaction`:

https://github.com/diesel-rs/diesel/blob/b705023d85e6ef76292a427aa70f21c28a5902ff/diesel/src/connection/transaction_manager.rs#L337-L359

The former, however, is not generating these events:

https://github.com/diesel-rs/diesel/blob/b705023d85e6ef76292a427aa70f21c28a5902ff/diesel/src/connection/transaction_manager.rs#L314-L329

This causes custom instrumentation to no longer uphold the invariant that every `BeginTransaction` is followed by exactly one of either `RollbackTransaction` or `CommitTransaction`.

### What are you trying to accomplish?

Trace ongoing transactions using custom instrumentation, including `IMMEDIATE` and `EXCLUSIVE` transactions.

### What is the expected output?

Using `SqliteConnection::immediate_transaction()` or `exclusive_transaction()` over the regular `transaction()` should generate the same instrumentation events. In particular, starting transactions with these two methods should generate a `BeginTransaction` event.

### What is the actual output?

Transactions started with `SqliteConnection::immediate_transaction()` or `exclusive_transaction()` do not generate the expected instrumentation event `BeginTransaction`.

### Are you seeing any additional errors?

No.

### Steps to reproduce

See above. The problem is independent of any actual database structure or queries inside the transcation.

<!--
Please include as much of your codebase as needed to reproduce the error.  If the relevant files are large, please consider linking to a public repository or a [Gist](https://gist.github.com/). This includes normally the following parts:

* The exact code where your hit the problem
* Relevant parts your schema, so any `table!` macro calls required for
* Any other type definitions involved in the code, which produces your problem
-->

## Checklist

- [x] I have already looked over the [issue tracker](https://github.com/diesel-rs/diesel/issues) and the [discussion forum](https://github.com/diesel-rs/diesel/discussions) for similar possible closed issues.
<!--
If you are unsure if your issue is a duplicate of an existing issue please link the issue in question here
--> 
- [x] This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)
- [x] This issue can be reproduced without requiring a third party crate

<!--
Thank you for your submission!  You're helping make Diesel more robust 🎉

We'll try to respond as quickly as possible.
-->


Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b006453871c5..693fd1fa693d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,6 +23,7 @@ Increasing the minimal supported Rust version will always be coupled at least wi
 in a way that makes the pools suitable for use in parallel tests.
 * Added `Json` and `Jsonb` support for the SQLite backend.
 * Fixed diesel thinking `a.eq_any(b)` was non-nullable even if `a` and `b` were nullable.
+* Generate `InstrumentationEvent::BeginTransaction` for immediate and exclusive transactions in SQLite
 
 ## [2.2.2] 2024-07-19
 
diff --git a/diesel/src/connection/transaction_manager.rs b/diesel/src/connection/transaction_manager.rs
index e361346d8612..b741e7a478c0 100644
--- a/diesel/src/connection/transaction_manager.rs
+++ b/diesel/src/connection/transaction_manager.rs
@@ -316,15 +316,22 @@ impl AnsiTransactionManager {
         Conn: Connection<TransactionManager = Self>,
     {
         let state = Self::get_transaction_state(conn)?;
-        match state.transaction_depth() {
-            None => {
-                conn.batch_execute(sql)?;
-                Self::get_transaction_state(conn)?
-                    .change_transaction_depth(TransactionDepthChange::IncreaseDepth)?;
-                Ok(())
-            }
-            Some(_depth) => Err(Error::AlreadyInTransaction),
+        if let Some(_depth) = state.transaction_depth() {
+            return Err(Error::AlreadyInTransaction);
         }
+        let instrumentation_depth = NonZeroU32::new(1);
+        // Keep remainder of this method in sync with `begin_transaction()`.
+
+        conn.instrumentation().on_connection_event(
+            super::instrumentation::InstrumentationEvent::BeginTransaction {
+                depth: instrumentation_depth.expect("We know that 1 is not zero"),
+            },
+        );
+        conn.batch_execute(sql)?;
+        Self::get_transaction_state(conn)?
+            .change_transaction_depth(TransactionDepthChange::IncreaseDepth)?;
+
+        Ok(())
     }
 }
 
@@ -343,15 +350,17 @@ where
                 Cow::from(format!("SAVEPOINT diesel_savepoint_{transaction_depth}"))
             }
         };
+        let instrumentation_depth =
+            NonZeroU32::new(transaction_depth.map_or(0, NonZeroU32::get).wrapping_add(1));
+        let sql = &start_transaction_sql;
+        // Keep remainder of this method in sync with `begin_transaction_sql()`.
+
         conn.instrumentation().on_connection_event(
             super::instrumentation::InstrumentationEvent::BeginTransaction {
-                depth: NonZeroU32::new(
-                    transaction_depth.map_or(0, NonZeroU32::get).wrapping_add(1),
-                )
-                .expect("Transaction depth is too large"),
+                depth: instrumentation_depth.expect("Transaction depth is too large"),
             },
         );
-        conn.batch_execute(&start_transaction_sql)?;
+        conn.batch_execute(sql)?;
         Self::get_transaction_state(conn)?
             .change_transaction_depth(TransactionDepthChange::IncreaseDepth)?;
 


Test patch: