Add label (repo name) to ownership and connection errors

[nathanl]

A label like MyApp.Repo can be passed via the :label option. It will be
used in the process label for the DBConnection.Ownership.Manager process
and will appear in error messages from both
DBConnection.Ownership.Manager and DBConnection.Holder. This is helpful
when applications use multiple repos (e.g., read replicas).

Before:

  (DBConnection.OwnershipError) cannot find ownership process for #PID<...>
  (DBConnection.ConnectionError) connection is closed because of an error...

After:

  (DBConnection.OwnershipError) cannot find ownership process for #PID<...>
  (MyApp.Repo) using mode :manual.

  (DBConnection.ConnectionError) MyApp.Repo connection is closed because of
  an error, disconnect or timeout

Tests verify labels appear correctly in both error types, and that errors
work correctly if no label is given.

In elixir-ecto/ecto_sql#698 the adapter is
updated to pass the repo name as a label.

[nathanl]

on line 381

[josevalim]

Hi @nathanl. I would prefer to not depend on repo, because it is really not a db_connection configuration or concern. Can you find anything on Process.info(self()) or Process.info(self(), :registered_name) you could use?

[nathanl]

@josevalim good point, but won't 99% of users pass a repo? In maybe_prefix_repo it's optional - would you be satisfied if it were optional everywhere in these messages?

[realglebivanov]

@nathanl would you consider adding some libary-agnostic metadata(contained e.g. in a kw-list) to the exception a viable option?

[nathanl]

@realglebivanov Can you elaborate a bit on what that might look like?

[realglebivanov]

@realglebivanov Can you elaborate a bit on what that might look like?

So, right now DBConnection.ConnectionError contains only message, reason and severity.
Instead of modifying the existing message text, we could add an additional field to the DBConnection.ConnectionError that could contain useful metadata.
Also, in this case we wouldn't have to rely on specific option types, it could be just a keyword list.
Of course, that could cost us an overhead of storing and copying around the list.
Hopefully, I made my point clear.

[nathanl]

Updated so that callers can optionally pass a label. If it's the repo name, like in elixir-ecto/ecto_sql#698, it shows up like this:

11:21:51.330 [error] Process #PID<0.1502.0> raised an exception
** (DBConnection.OwnershipError) cannot find ownership process for #PID<0.1502.0> (:erlang)
(GridPoint.Repo) using mode :manual.
(Note that a connection's mode reverts to :manual if its owner
terminates.)

[nathanl]

@josevalim what do you think of this approach?

[josevalim]

Since we are passing the label, wouldn't make more sense to use it for Process.set_label? And then we don't. need to pass it around? It just becomes part of our Util.inspect_pid ?

[nathanl]

I'm not sure the repo name would be a good label for the ownership manager - it isn't the repo per se, and probably ecto_sql and other callers shouldn't tell it how to label itself. I'm just trying to have this info available for the not_found error for this connection.

[josevalim]

You could compose it {__MODULE__, label}

[nathanl]

@josevalim I updated to set the process label. However, to get the repo name in the errors in DBConnection.Holder we have to pass them along, and I thought fetching the label back out of the process label was awkward. So I'm also putting it in to process state. Thoughts?