Hey!

Unfortunately changing the contract of log_level function from (env) -> atom to ({:ok, env} | {:error, any}) -> atom would be a breaking change, so we can't do that.

In general, I think it makes sense to have log level function accept both ok & error cases.

One way to solve this, would be to add a new option, i.e. level: atom | fun and deprecate the current log_level option with some logic like this:

cond do
  log_level option given and level option given -> 
    error (can't provide both)
  log_level option given ->
    show deprecation warning
    use this function: fn {:ok, env} -> log_level.(env); _ -> :error end
  level option given ->
    use level
  else ->
    use default level function
end

Hey @teamon , I read your comment then drafted a rough commit but got a few questions. (Actually, there’s more to consider than I thought )

Therefore I have put it all together as below:

Context

First of all, this is the original feedback.

If the request returns {:error, whatever}, the log level is always set to :error, the custom log level function is never called. The custom log level is only used when an {:ok, ...} is returned.

And it depends on these logic.

  defp log_level({:error, _}, _), do: :error

  defp log_level({:ok, env}, config) do
    case Keyword.get(config, :log_level) do
      nil ->
        default_log_level(env)

      fun when is_function(fun) ->
        case fun.(env) do
          :default -> default_log_level(env)
          level -> level
        end

      atom when is_atom(atom) ->
        atom
    end
  end

  @spec default_log_level(Tesla.Env.t()) :: log_level
  def default_log_level(env) do
    cond do
      env.status >= 400 -> :error
      env.status >= 300 -> :warn
      true -> :info
    end
  end

Current logic only allow log_level option apply to {:ok, env} and always return error when {:error, _}, and current code expect log_level will accept Tesla.Env.t() if it is a function.

Let's say it makes sense to have log level function accept both ok & error cases.

QUESTION 1:

My understanding is that it essentially needs to handle level first and then parse response not matter {:ok, env} or {:error, any}, does it make sense ？

https://github.com/Yarnus/tesla/blob/logger_draft/lib/tesla/middleware/logger.ex#L198-L208

QUESTION 2:

For new option log_level, if it define as a function, it may accept different response cases. i.e. {:ok, Tesla.Env.t()} or {:error, some_error} instead of Tesla.Env.t()

  defmodule MyClient do
    use Tesla

    plug Tesla.Middleware.Logger, log_level: &my_log_level/1
    
    @spec my_log_level({:ok, %Tesla.Env.t() | {:error, any()}) :: atom
    def my_log_level(response) do
       ...
    end
  end

And I also extended default_log_level/1 but will not break the log_level/1 contract.

https://github.com/Yarnus/tesla/blob/logger_draft/lib/tesla/middleware/logger.ex#L246-L261

Does that make sense to you ? I’m not sure if my understanding is what you’re expecting

QUESTION 3

For show deprecation warning, I have no idea to compatible with compile time and runtime middleware 🤔

Anyway, thanks for your reading and leading

What should we do about this PR and #379 issue?

Ideally I'd go with #504 (comment) (new option, deprecate old one) and update docs accordingly.