search

elixir-ecto / db_connection

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

Add metrics for connection pool usage v3 #307

Closed realglebivanov closed 5 months ago

realglebivanov commented 5 months ago

Hello!

Since these two PRs seem to be abandoned

https://github.com/elixir-ecto/db_connection/pull/306 https://github.com/elixir-ecto/db_connection/pull/292,

I've decided to open another one that will hopefully be merged.

josevalim commented 5 months ago

The pool ID should be the name of the Ecto repo, no? My biggest concern is that those costs will be paid by everyone in every operation, regardless of the sampling or need. So I think we should try polling first and fallback to telemetry only if that doesn’t work. Or put the telemetry in this PR behind some periodic action.

realglebivanov commented 5 months ago

@josevalim Hey, thanks for the review! I fixed the issue you had found but the tests are a little bit ugly. Hopefully, we'll find the correct solution.

josevalim commented 5 months ago

Thank you, this looks great! I think we just need to move it to the DBConnection module now, which is the public API. But perhaps, to do so, we need a callback in the pool and implement it in the ownership pool too? 🤔

realglebivanov commented 5 months ago

Fair enough but this will introduce a breaking change into the API. Every custom pool there is will have to implement the new callback. Also, it would probably be better to move this to DBConnection.Pool as it seems to be a part of the API as well

realglebivanov commented 5 months ago

Actually, it isn't a part of the public API but I guess it should be a part of both DBConnection.Pool and DBConnection.

josevalim commented 5 months ago

Yes, the pool API is private, so we are fine (but it would be fine to add new callbacks to behaviours nonetheless).

realglebivanov commented 5 months ago

@josevalim I've managed to come up with a sketch of what the public API might look like. Please, take a look!

realglebivanov commented 5 months ago

Thanks, José! I'd rather leave it as it is if you don't mind. I just wanted the end user to decide in which way it would be convenient to aggregate the metrics.

josevalim commented 5 months ago

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

realglebivanov commented 5 months ago

:heart: i'm a contributor now :rofl:

josevalim commented 5 months ago

And not an easy feature to add, well done!