Not dumped values in telemetry events

[fuelen]

Elixir version

1.12.2

Database and Version

PostgreSQL 14.1

Ecto Versions

ecto 3.9.2, ecto_sql 3.9.1

Database Adapter and Versions (postgrex, myxql, etc)

postgrex 0.16.0

Current behavior

After updating ecto_sql from 3.9.0 to 3.9.1 I noticed one error in my test suit. Possibly related to https://github.com/elixir-ecto/ecto_sql/pull/455 As I understand, Ecto sends all arguments to telemetry, which are not dumped to native types.

if we have the following type

  defmodule Money.Ecto.Type do
    use Ecto.ParameterizedType

    def type(_params), do: :money_type

    def init(_opts), do: %{}

    def cast(_data, _params), do: {:ok, nil}

    def load(nil, _loader, _params), do: {:ok, nil}

    def load({currency, value}, _loader, _params),
      do: {:ok, %Money{currency: currency, value: value}}

    def dump(nil, _dumper, _params), do: {:ok, nil}
    def dump(data, _dumper, _params), do: {:ok, {data.currency, data.value}}

    def equal?(a, b, _params) do
      a == b
    end
  end

and try to insert a new record with the type above, then the value comes to telemetry as is - Money struct.

Expected behavior

Seems like current behaviour is a new feature, but for https://github.com/fuelen/ecto_dev_logger I'd like to have some way of having native types in order to print SQL counterpart.

[greg-rychlewski]

Hi @fuelen,

Want to make sure I understand, you'd like to have access to both the cast params and the dumped params in the telemetry events?

[fuelen]

For ecto_dev_logger I'd like to have data which are closer to SQL. And dumped params in this case are better.

When I mentioned you in this comment https://github.com/elixir-ecto/ecto/pull/3869#issuecomment-1304802701 I thought it will touch representing of UUID in dumped params :)

[greg-rychlewski]

Ah ok, so sounds like you need both then because some dumped params are not closer to SQL (i.e. UUID).

[greg-rychlewski]

@josevalim What do you think about keeping the current params as the casted versions and then adding dumped_params to the telemetry event?

[greg-rychlewski]

This makes me think it would be nice to have an attribute on the schema field to keep the casted or dumped version of the param for logging/telemetry. I'm not sure how crazy the change would be though.

[fuelen]

another suggestion: use params for dumped params as it was before and add a new field like param_types which is a list of data types [:integer, :string, :binary_id, ...]. So, I can zip the lists and if the value has type :binary_id, then I would be able to convert uuid back to human-readable format

[josevalim]

We want to avoid double dumping though. And passing the types is not always possible. I think supporting both params (as it was) and passing cast_params too is the way to go!