elixir-ecto / db_connection

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

feat: send checkout timeout event to connection listeners #235

Closed polvalente closed 3 years ago

polvalente commented 3 years ago

Adds the :checkout_timeout event as suggested in #234

My main concern here is that I had to use the ConnectionPool as a message relay for the connection_listeners. Is there a way to avoid this? I'm worried that this could cause an inbox overflow to the ConnectionPool.

Another workaround would be to publish this as a Telemetry event, altough in that case I wouldn't know how to fetch the "telemetry event prefix" that is passed to Ecto.

josevalim commented 3 years ago

You are correct. Perhaps it is best to use telemetry for this rather than the connection listener.

polvalente commented 3 years ago

You are correct. Perhaps it is best to use telemetry for this rather than the connection listener.

Any ideas on how to deal with the configurable prefix? I tried to look, but there didn't seem to be an easy way to find it. Anyway, I'll start refactoring this to emit an event instead sending the message

josevalim commented 3 years ago

The configurable prefix has been generally discouraged in telemetry and mostly no other project uses it, besides Ecto for legacy reasons. My suggestion is to use a set prefix and pass any relevant information as metadata. I assume we will at least have access to the repository name from here? 🤔

polvalente commented 3 years ago

@josevalim It seems that either I overlooked something, or some of my changes changed something. I inspected the opts to see if the repo was available, but to my surprise, the connection_listeners were there!

edit: I've pushed the relevant changes to make the checkout_timeout send go directly to the relevant processes :)

josevalim commented 3 years ago

Ok, so let's ship as is and just rename the event. We also need docs and we should be good to go!

polvalente commented 3 years ago

Closed in favor of #236