elixir-ecto / db_connection

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

Telemetry event for all connection errors #239

Closed akash-akya closed 3 years ago

akash-akya commented 3 years ago

Use case

We have an application which connects to multiple MySQL databases. As of now, whenever there is a connection error, all we get is a generic error log, like MyXQL.Connection (#PID<*.*.*>) failed to connect: ** (DBConnection.ConnectionError) connection refused. This makes debugging difficult for us. We like to have error logs with repo information.

Change proposal

I noticed we already have telemetry events. We can use this to log duplicate error message with repo information fetched from telemetry metadata. But looks like currently events are only emitted when there is a checkout attempt. So not all connection errors emit the event (such as connect attempt in background when app starts).

What you think of emitting event for every "connect" failure?

josevalim commented 3 years ago

Please do investigate. However, if you want to have repository information, it is unlikely this would be a DBConnection feature. Perhaps the easiest route to start is by changing MyXQL to include more information in the error messages (at least hostname+port).

akash-akya commented 3 years ago

We are already passing all opts in the telemetry event right? Thought of just logging opts[:repo]. No changes required to db_connection for this, opts is still opaque. Only change is - emitting telemetry event here instead of in the holder.

easiest route to start is by changing MyXQL to include more information in the error messages (at least hostname+port).

Agree. Will look into this first

Thanks

josevalim commented 3 years ago

Ah, if the error is coming from DBConnection then changing MyXQL to include more information won't work. The fix has to be here, although we choose to not allow telemetry events inside the connections themselves. 🤔 We could use the current connection listeners though... maybe include a connection_failure event that includes the exception? And then include more information, if possible, in the exception?

akash-akya commented 3 years ago

if the error is coming from DBConnection then changing MyXQL to include more information won't work

I thought you meant the exception message from myxql should contain more information, so when DBConnection logs it with Exception.format_banner it contains the required information. In fact MongoDB lib already does this for most of the errors.

We could use the current connection listeners

Previously I thought of it. But dropped it thinking that since we dont really need the connection, and thinking it like "telemetry event" made more sense. Anyway I will look into that as well.

akash-akya commented 3 years ago

Opened an issue at MyXQL for changing error message and its already is addressed there.

Closing in favor of that.