elixir-grpc / grpc

An Elixir implementation of gRPC
https://hex.pm/packages/grpc
Apache License 2.0
1.36k stars 210 forks source link

Default `accepted_comparators` option of logger interceptors doesn't match Elixir's default behavior #325

Closed guisehn closed 1 year ago

guisehn commented 1 year ago

Describe the bug

elixir-grpc's interceptors GRPC.Server.Interceptors.Logger and GRPC.Client.Interceptors.Logger are doing the opposite check of Elixir's default Logger behavior by default, this seems to be a mistake.

To Reproduce

Set your application level to :debug:

config :logger, level: :debug

Add the GRPC.Server.Interceptors.Logger to your application:

defmodule MyApp.Endpoint do
  use GRPC.Endpoint

  intercept GRPC.Server.Interceptors.Logger # `level` option defaults to `:info`, so elixir-grpc will call Logger.info()

  run MyApp.GRPC.Health.Server
end

Then, do a gRPC call to your server and you'll notice it will not log anything.

Expected behavior

When I set my application log level to :debug, I expect it to print all levels of logs, because :debug is the lowest log priority possible.

This is how Elixir's Logger works by default:

Example:

iex(1)> require Logger
Logger
iex(2)> Logger.configure(level: :debug)
:ok
iex(3)> Logger.info("I am being logged because debug < info")

[info]  I am being logged because debug < info
:ok
iex(4)> Logger.configure(level: :warn) 
:ok
iex(5)> Logger.info("I am not being logged because warn > info")
:ok

Additional context

Both GRPC.Server.Interceptors.Logger and GRPC.Client.Interceptors.Logger compare the level option with Log.level(). I assume this is to prevent unnecessary processing. See code.

However, it seems that the comparison is backwards. To match Elixir's default behavior, we should either 1) reverse the order params are passed to Logger.compare_levels, or 2) set accepted_comparators default to [:gt, :eq].

Doing this change will restore the default behavior before https://github.com/elixir-grpc/grpc/pull/227 was merged.