elixir-ecto / db_connection

Database connection behaviour
http://hexdocs.pm/db_connection/DBConnection.html
306 stars 113 forks source link

Add meter/log callback module which is configured per call #10

Closed fishcakez closed 8 years ago

fishcakez commented 8 years ago

Proxy modules do not provide a good abstraction for logging because it is devilishly hard to log at the right moments. The two main goals of DBConnection is to separate concerns and make calls to orthogonal parts at the optimum moment. It has failed to do that for logging. It will be much cleaner and more efficient to support logging or metric measuring in DBConnection directly.

@josevalim I agree with your comment on IRC and I think this is the way to go for logging. However if you have other ideas or thoughts please give them.

fishcakez commented 8 years ago

Logging support has been added. The implementation is slightly more efficient than Ecto 1.* because time is read fewer times, 4 for a single query instead of 6.

The DBConnection.LogEntry is not quite the same as Ecto's: https://github.com/fishcakez/db_connection/blob/a2ad7210f6757fc66f838f6590908092b4b9cdb5/lib/db_connection/log_entry.ex#L9-L19.

Ecto can convert these when it builds its own LogEntry.

Note I used :log for the option, with mfargs or arity 1 fun, to match after_connect. Ecto will have to rewrite this option to create its own LogEntry whatever the key so I think it is ok.

josevalim commented 8 years ago

@fishcakez fantastic work as always! Btw, which one do you think would be better:

  1. Have the query function in DBConnection return this struct so we can do whatever we want with it
  2. Or have the :log callback which we would wrap and override?

I'd actually prefer 1 but I am not sure if it is actually possible.

fishcakez commented 8 years ago

1) is very tricky because of consistency as we can't return the struct with transactions. I think we will have to go with 2) unless you have a suggested API.

josevalim commented 8 years ago

Good point about the transactions. I can only think about giving the log entry struct in the transaction callback and returning one in the transaction call. I guess it is clunky though?