Add label (repo name) to ownership 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 some error messages. This is helpful
when applications use multiple repos (e.g., read replicas).

Before:

  (DBConnection.OwnershipError) cannot find ownership process for #PID<...>

After:

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

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.

[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?

[nathanl]

I think this is ready for re-review. I'm disappearing until Jan 5, but if anything needs changing I'll take a look then.

[josevalim]

Sorry, this fell through the cracks. I would prefer to rely on the label given to the process name as much as possible, instead of passing the label around. Can we start with that? WDYT?

[mitchellhenke]

Just wanted to add that I'm interested in this and happy to pick it up 🙂

[nathanl]

Sorry for my long silence. Update coming soon.

[nathanl]

Updated to use only a process label. As you said @josevalim, using a process label alone doesn't let us put the repo name in something like ** (DBConnection.ConnectionError) connection is closed because of an error, disconnect or timeout, but it does let us put it in

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

The related ecto_sql PR still needs merging.

[josevalim]

💚 💙 💜 💛 ❤️