Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Credo.Check.Warning.MissedMetadataKeyInLoggerConfig reporting false errors since 1.7.2 upgrade #1101

Closed
Billzabob opened this issue Jan 2, 2024 · 8 comments

Comments

@Billzabob
Copy link

Precheck

Environment

  • Credo version (mix credo -v):
    1.7.2-ref.main.ef07378c+uncommittedchanges
  • Erlang/Elixir version (elixir -v):
    Erlang/OTP 26 [erts-14.1] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit]
    Elixir 1.15.6 (compiled with Erlang/OTP 24)
  • Operating system:
    MacOS Sonoma 14.2.1 (23C71)

What were you trying to do?

Run mix credo --strict

Expected outcome

No Credo errors

Actual outcome

Since going from 1.7.1 to 1.7.2 we are now getting a ton of errors from Credo.Check.Warning.MissedMetadataKeyInLoggerConfig that look like this:

┃ [W] ↗ Logger metadata key aid not found in Logger config
┃       lib/libs/quiltt.ex:85 #(Libs.Quiltt.upsert_account)

We have all these specified in our config/config.exs like so:

config :logger, :console,
  format: "$time $metadata[$level] $message\n",
  metadata: [
    :account,
    :admin,
    :aid,
    ...
  ]
@tcitworld
Copy link

Same issue here.

:logger |> Application.get_env(:default_formatter) |> Keyword.get(:metadata) returns the correct metadata list in the same environment.

@rrrene
Copy link
Owner

rrrene commented Jan 4, 2024

@Billzabob Ah, we changed the key that is read to :default_formatter for newer versions of Elixir. Does it work when you change your logger config to the new key?

@tcitworld When you are writing "in that environment", is that the same env Credo is run in?

tcitworld added a commit to framasoft/mobilizon that referenced this issue Jan 4, 2024
…ggerConfig check

It's buggy rrrene/credo#1101

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@ViseLuca
Copy link

ViseLuca commented Jan 4, 2024

@rrrene yeah, I tried on my project to add the metadata in every environment file but nothing is changing. Same error of @Billzabob

@rhammer1
Copy link

rhammer1 commented Jan 4, 2024

Changing :console to :default_formatter worked for me on a clean phoenix install

ie.
`$ mix credo -v
1.7.2

$ elixir -v
Erlang/OTP 26 [erts-14.2.1] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit:ns]

Elixir 1.16.0 (compiled with Erlang/OTP 24)

$ uname -a
Linux russ-76 6.6.8-200.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Dec 21 04:01:49 UTC 2023 x86_64 GNU/Linux

$ mkdir /tmp/temp-phoenix
$ cd /tmp/temp-phoenix
$ mix archive.install hex phx_new
$ mix phx.new hello

finish install

$ sed -i -e '/:dns_cluster/a \ {:credo, "~> 1.7.2", only: [:dev, :test], runtime: false},' mix.exs
$ mix deps.get

finish install

$ mix credo

all good

$ echo 'defmodule Hello.Foo do
@moduledoc false

require Logger

defp foo do
Logger.metadata(request_id: request_id)
end
end' >lib/hello/foo.ex

$ mix credo
Checking 22 source files ...

Warnings - please take a look

┃ [W] ↗ Logger metadata key request_id not found in Logger config
┃ lib/hello/foo.ex:7 #(Hello.Foo.foo)

Please report incorrect results: https://github.com/rrrene/credo/issues

Analysis took 0.2 seconds (0.04s to load, 0.2s running 55 checks on 22 files)
66 mods/funs, found 1 warning.

Showing priority issues: ↑ ↗ → (use mix credo explain to explain issues, mix credo --help for options).

$ sed -i -e 's/:console/:default_formatter/' config/{config,dev}.exs

$ mix credo

all good

`

Question? is this the correct way to clear the 'issue'? I don't know the impact to the rest of the application when changing :console to :default_formatter

@rrrene
Copy link
Owner

rrrene commented Jan 5, 2024

@Billzabob @tcitworld @ViseLuca Thanks for reporting this 😀 It should now be fixed on master.

You can try this by setting the Credo dep to

{:credo, github: "rrrene/credo"}

Please report back if your issue is solved! 👍

@rhammer1 No, we don't want people to change their config.exs for Credo to work. We f'ed up on this one 🤔

@ViseLuca
Copy link

ViseLuca commented Jan 5, 2024

{:credo, github: "rrrene/credo"}

Yeah issue is solved! I tried right now, thank you very much! 😄

@rrrene
Copy link
Owner

rrrene commented Jan 5, 2024

@Billzabob @tcitworld @ViseLuca @rhammer1 Fix is live as part of 1.7.3. Sorry for the inconvenience. ✌️

Please feel free to re-open if there are any problems 👍

@rrrene rrrene closed this as completed Jan 5, 2024
@rhammer1
Copy link

rhammer1 commented Jan 5, 2024

Credo 1.7.3 fixes the problem, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants