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

Give prevalence to process level over primary/module levels #13955

Closed
wants to merge 2 commits into from

Conversation

v0idpwn
Copy link
Contributor

@v0idpwn v0idpwn commented Nov 3, 2024

The docs incorrectly implied that Logger.put_process_level/2 allowed one to decrease the logger level for a process. This is not possible with this function.

@v0idpwn
Copy link
Contributor Author

v0idpwn commented Nov 3, 2024

Reproducing the issue:

Erlang/OTP 27 [erts-15.0] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit:ns]

Interactive Elixir (1.17.1) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> require Logger
Logger
iex(2)> Logger.configure(level: :info)
:ok
iex(3)> Logger.put_process_level(self(), :debug)
:ok
iex(4)> Logger.debug("hello?")
:ok
iex(5)> Logger.info("info")

09:46:25.063 [info] info
:ok
iex(6)> Logger.put_process_level(self(), :error)
:ok
iex(7)> Logger.info("info again")
:ok
iex(8)> Logger.error("error")

09:46:47.350 [error] error
:ok
iex(9)>

@josevalim
Copy link
Member

It seems the put_module_level makes a bypass, so if that's the case, maybe we should try to mirror it here?

@v0idpwn v0idpwn changed the title Clarify Logger.put_process_level/2 precedence in docs Give prevalence to process level over global level Nov 3, 2024
@v0idpwn
Copy link
Contributor Author

v0idpwn commented Nov 3, 2024

Pushed the change to prefer process level over global level. Will write a test.

level
end

process_level ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to check if this level is allowed by either calling :logger_config.allow(level) or by calling :logger_config.allow(level, module) if we want still want to look up the module.

Copy link
Contributor Author

@v0idpwn v0idpwn Nov 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling :logger_config.allow(level, module) there would give precedence for module/primary level. Pushed some fixes and I believe we have the expected behaviour now :)

@@ -760,7 +784,7 @@ defmodule LoggerTest do

test "maps Erlang levels" do
:logger.set_primary_config(:level, :notice)
assert capture_log(fn -> Logger.info("hello") end) =~ "hello"
assert capture_log(fn -> Logger.warning("hello") end) =~ "hello"
Copy link
Contributor Author

@v0idpwn v0idpwn Nov 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what's the behaviour that was being tested here. This started failing when I made capture_log not change log level

@v0idpwn v0idpwn changed the title Give prevalence to process level over global level Give prevalence to process level over primary/module levels Nov 3, 2024
@v0idpwn
Copy link
Contributor Author

v0idpwn commented Nov 3, 2024

There's a big problem with this approach: elixir and erlang logs behave under different rules. I think this is not desirable and probably we can't merge.

@v0idpwn
Copy link
Contributor Author

v0idpwn commented Nov 4, 2024

Closing per the above. Will fix documentation in a separate PR.

@v0idpwn v0idpwn closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants