elixir-ecto / db_connection

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

feat: add telemetry event for queue timeouts #236

Closed polvalente closed 3 years ago

polvalente commented 3 years ago

As discussed in #235 and #234, this PR adds a telemetry event for DBConnection.ConnectionErrors.

There are some discussion points still:

  1. Which opts should we add to metadata?
  2. Is there any meaningful measurement we could add instead of %{error: 1}?
  3. Although the tests pass as expected, when I used this code in a local project (pointing to the local path for db_connection), the :connection_listeners option is empty. Is this expected? This was the only way I could think of for monitoring a project with different Ecto.Repos. Or maybe this event should be published by either postgrex, or ecto_sql or even ecto?

I chose to keep publishing the event basically in the same place as in #235 because the opts could be useful in handling multiple-Ecto.Repo projects.

josevalim commented 3 years ago

Looks great, I have added some comments and we can ship it!

josevalim commented 3 years ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart:

v0idpwn commented 3 months ago

I'd like to emit telemetry events from disconnects coming from client-side, too, like when the process that checked out a connection exits, causing a disconnect. Currently we are measuring this through logs, but it's not possible to identify which Repo had the issue. Would a PR be welcome? cc @josevalim @greg-rychlewski

I think the optimal place to emit such event (from my perspective) would be postgrex, which has more context and can emit more useful metadata. But since db_connection is already emitting connection_error events, I think maybe it should be emitted from there...

josevalim commented 3 months ago

Would you send a PR for both projects so we can compare them? I think this particular event we have in this PR cannot be emitted by Postgrex, can it?

v0idpwn commented 3 months ago

Actually, I think the same applies here, the telemetry event would need to be emitted from db_connection. db_connection also has all the info Postgrex has, it just can make less assumptions about the data, making metadata look different for different adapters. But I think it's fine.

I wrote https://github.com/elixir-ecto/db_connection/commit/4256384a22f5597a6d3f0dceb34212b51ad88d43 as a PoC (names could be discussed, etc). What strikes me is that it makes the :connection_listeners option look a bit redundant, since it's a more powerful alternative (at least for disconnects, and adding an event for connects would be trivial).

josevalim commented 3 months ago

Couldn’t telemetry be emitted from the listeners then? Maybe we ship a listener that does telemetry?

v0idpwn commented 3 months ago

This would forbid me to add metadata, as listeners only receive :connected/:disconnected + a tag. I was thinking of the reverse path: making a telemetry handler that sends to the listeners.

Note: I figured its possible to use the connection listeners for my use-case due to the newish tags feature, so it might be superfluous to implement the telemetry event. But using telemetry would probably provide a better API 👀