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

How to log SQL queries, or log slow queries? #32

Closed
adamw opened this issue Jul 26, 2024 · 8 comments · Fixed by #44
Closed

How to log SQL queries, or log slow queries? #32

adamw opened this issue Jul 26, 2024 · 8 comments · Fixed by #44

Comments

@adamw
Copy link
Contributor

adamw commented Jul 26, 2024

Is there a way to (configurably) log the generated SQL queries, or log queries which are slow to execute?

There are some logSql functions in util, but they seem unused.

@AugustNagro
Copy link
Owner

AugustNagro commented Jul 26, 2024 via email

@adamw
Copy link
Contributor Author

adamw commented Jul 31, 2024

Ah! Finally got it to work. Turns out that private val Log = System.getLogger(getClass.getName) will create a logger for scala.Predef, instead of com.augustnagro.magnum - what I was expecting in my logback config file. I suppose this should rather be the magnum's package?

Also, it would be great to have some kind of "interceptor" - logSql is fine for basic logging, but for more advanced use cases (like logging slow queries), or plugging in some metrics - an interceptor would be needed I think.

@AugustNagro
Copy link
Owner

AugustNagro commented Aug 1, 2024 via email

@adamw
Copy link
Contributor Author

adamw commented Aug 1, 2024

I was using Doobie's LogHandler before and it worked well: https://github.com/tpolecat/doobie/blob/d1f84e64b026c0989818546c935c879e5dcc3598/modules/free/src/main/scala/doobie/util/log.scala#L122

So maybe something similar, passed as parameter to transact - just as connectionConfig is passed now?

@AugustNagro
Copy link
Owner

AugustNagro commented Oct 3, 2024

I've merged an MR to fix the logger name, thanks again for reporting. I'm aiming to make a new release next week.

For the 'interceptor' feature, I still like the idea. However I'm worried that If we add a new parameter to transact, it would require too much discipline to remember at every transact call site.

In doobie you can put the LogHandler in your Transactor and then it's applied to every query with the Transactor. I wonder if we could have something similar, like

case class MagDataSource(
  dataSource: DataSource,
  sqlLogger: SqlLogger = SqlLogger.NoOp,
  connectionConfig: Connection => Unit = con => ()
)

// new overloads
def connect[T](magDataSource: MagDataSource)(f: DbCon ?=> T): T = ???
def transact[T](magDataSource: MagDataSource)(f: DbTx ?=> T): T = ???

// updated classes
class DbCon private[magnum] (val connection: Connection, val sqlLogger: SqlLogger)
class DbTx private[magnum] (connection: Connection, sqlLogger: SqlLogger) extends DbCon(connection, sqlLogger)

@adamw
Copy link
Contributor Author

adamw commented Oct 3, 2024

Putting it in the data source would work as well, though this would make logging DS-global, while I think in practice you might have different warning thresholds for different transactions (or even queries).

Definitely adding a new mandatory parameter to transact is a no-go, though maybe an overload, or using configuration from the implicit scope? Where the default configuration would be not to log, or to log using TRACE/DEBUG?

@AugustNagro
Copy link
Owner

@adamw please review when you get a chance. Here's an example:

val transactor = Transactor(
  dataSource = ???,
  sqlLogger = SqlLogger.logSlowQueries(500.milliseconds),
  connectionConfig = con =>
    con.setTransactionIsolation(Connection.TRANSACTION_REPEATABLE_READ)
)

transact(transactor):
  sql"SELECT id from myUser".query[Long].run()

@adamw
Copy link
Contributor Author

adamw commented Oct 7, 2024

Looks great! And since Transactor is a case class, it will be trivial to update the config for a specific query/code path. Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants