elixir-ecto / db_connection

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

Proposal: add encode_time to LogEntry #284

Closed ruslandoga closed 1 year ago

ruslandoga commented 1 year ago

Right now LogEntry includes decode_time but no encode_time. I wonder why and if it's OK to add encode_time to the logs.

I think meter = event(meter, :encode) can be added to encode and maybe_encode similar to https://github.com/elixir-ecto/db_connection/blob/285650ee8bbcea119ac74327b81f2571a42eedc0/lib/db_connection.ex#L1350 and then calculated similar to https://github.com/elixir-ecto/db_connection/blob/285650ee8bbcea119ac74327b81f2571a42eedc0/lib/db_connection/log_entry.ex#L62-L64

This metric would be useful in ClickHouse-like adapters that occasionally perform large INSERTs. Separating the encoding time from network latency there allows for better insight on where time is spent and what should be optimised.

josevalim commented 1 year ago

Encoding happens outside of db_connection, which is why it isn't measured here.

ruslandoga commented 1 year ago

Sorry for a dumb question, but what does "outside" mean in this context?

josevalim commented 1 year ago

No dumb question at all. It means db_connection is not the one doing the encoding. It already receives encoded data!

josevalim commented 1 year ago

The important bit is that encoding is done outside of the connection process (for performance) and decoding is partially done inside the connection process.

ruslandoga commented 1 year ago

Ah, I think I understand. DBConnection.Query.encode and decode are called outside of checkout. I think what confuses me is that handle_execute (which is wrapped in checkout) does not seem to be called in the connection process either -- it's called in the caller process similar to encode and decode. But even then, what stops us from recording encode times? It doesn't seem like process dict or anything tied to a process is used for storing metrics, and when encode happens, there's already access to meter.

josevalim commented 1 year ago

Sorry, the word process was misleading there. Very little happens in the connection process. It is mostly there to hold the connection when no-one is using, handle idle checks, and timeouts. A better way to say is that it happens while we own the connection.