Allow multiple logger filter #343
Conversation
WalkthroughThe pull request introduces a refined logging filter mechanism for the Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
h3poteto
force-pushed
the
feat/filter-level
branch
2 times, most recently
from
ee22646 to
9f1b02d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/logger_proxy.ex(3 hunks)test/logger_proxy_test.exs(4 hunks)
🔇 Additional comments (6)
lib/logger_proxy.ex (3)
62-64: Looks good.Using a keyword list to retrieve
filtersis clear and concise. This approach allows for easy extension if more LoggerProxy configuration options are added in the future.
74-86: Logic for applying multiple filters is correct.Great approach with
Enum.reduce_while/3to stop evaluation at the first successful filter. This structure is clean and maintainable, ensuring that only one filter ultimately handles the log message.
98-109: Off level handling is cleanly implemented.Allowing a
:offlevel for filters is a neat way to selectively discard messages. Thelogger_with_level/2function is consistent with the rest of the design.test/logger_proxy_test.exs (3)
88-92: Filter that silences debug messages works well.Switching to a list of filters and using
:offensures debug messages are fully suppressed. The test accurately verifies that debug logs are no longer emitted.
Line range hint
105-115: Naming and logic alignment are consistent.Renaming the test to "can_consume error should be warn" clearly reflects the filter’s intended functionality. The updated filter tuple structure matches the new configuration in
LoggerProxy.
126-136: Check for correct log level when can_consume fails.Asserting on
:warnlevel and refuting:errorlogs validates the filter’s effect. This ensures the log message is demoted from:errorto:warnas intended.
h3poteto
force-pushed
the
feat/filter-level
branch
from
9f1b02d to
9d8cb74
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/logger_proxy_test.exs (1)
105-145: Consider extracting a helper function to reduce duplication.While the test case correctly verifies the log level conversion, there's some duplication in the Router.can_consume? calls that could be extracted into a helper function.
Consider applying this refactor:
+ defp try_consume(router) do + Router.can_consume?(router, "d117b485-7490-4146-812f-d3f744f0a8c7", %{ + codecs: [], + headerExtensions: [], + fecMechanisms: [] + }) + Process.sleep(10) + end + test "can_consume error should be warn" do # ... existing setup code ... - assert capture_log([level: :warn], fn -> - Router.can_consume?(router, "d117b485-7490-4146-812f-d3f744f0a8c7", %{ - codecs: [], - headerExtensions: [], - fecMechanisms: [] - }) - Process.sleep(10) - end) =~ "can_consume() | Producer with id " + assert capture_log([level: :warn], fn -> try_consume(router) end) =~ "can_consume() | Producer with id " - refute capture_log([level: :error], fn -> - Router.can_consume?(router, "d117b485-7490-4146-812f-d3f744f0a8c7", %{ - codecs: [], - headerExtensions: [], - fecMechanisms: [] - }) - Process.sleep(10) - end) =~ "can_consume() | Producer with id " + refute capture_log([level: :error], fn -> try_consume(router) end) =~ "can_consume() | Producer with id " end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/logger_proxy.ex(3 hunks)test/logger_proxy_test.exs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and test (stable)
- GitHub Check: Lint Rust (native/mediasoup_elixir/Cargo.toml)
- GitHub Check: lint (stable)
🔇 Additional comments (5)
lib/logger_proxy.ex (4)
49-54: LGTM! Well-structured type definitions.The new type definitions are clear and accurately represent the enhanced filtering capabilities.
Also applies to: 57-57
62-64: LGTM! Proper initialization of filters.The function correctly initializes filters with a default empty list and passes them to the GenServer.
74-86: LGTM! Efficient filter processing implementation.The use of
Enum.reduce_whilewith early termination on the first matching filter is an efficient approach. The implementation preserves the default logging behavior when no filters match.
98-108: LGTM! Clean implementation of level-specific logging.The function properly handles all log levels, including the special
:offcase, while maintaining consistent metadata structure.test/logger_proxy_test.exs (1)
88-103: LGTM! Comprehensive test coverage for debug message filtering.The test case properly verifies both the filtering of debug messages and the preservation of other log levels.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/logger_proxy.ex (1)
74-96: Consider adding error handling for filter execution.While the filter processing logic is well-implemented, it might be good to add error handling for filter execution to prevent crashes if a filter function raises an exception.
Consider wrapping the filter execution in a
try-rescue:def handle_info(%Mediasoup.LoggerProxy.Record{} = msg, %{filters: filters} = state) do with nil <- Enum.reduce_while(filters, nil, fn %{filter: filter, level: level}, _acc -> - case filter.(msg) do + case try do + filter.(msg) + rescue + e -> + Logger.error("Error executing log filter: #{Exception.message(e)}") + false + end do false -> {:cont, nil} true -> logger_with_level(msg, level) {:halt, true} end end) do Logger.log(msg.level, msg.body, %{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/logger_proxy.ex(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint (stable)
- GitHub Check: Lint Rust (native/mediasoup_elixir/Cargo.toml)
- GitHub Check: Build and test (stable)
🔇 Additional comments (3)
lib/logger_proxy.ex (3)
49-57: LGTM! Well-structured type definitions.The new type definitions are clear, comprehensive, and correctly model the enhanced filtering mechanism. The inclusion of
:offin the level type provides good flexibility for selectively disabling logs.
60-67: LGTM! Clean configuration handling.The configuration changes are well-implemented with sensible defaults. The state structure properly accommodates the new filtering mechanism.
98-108: LGTM! Clean implementation of level-based logging.The helper functions are well-structured with proper handling of the
:offlevel and consistent metadata handling.
lib/logger_proxy.ex
Outdated
| @@ -46,16 +46,22 @@ defmodule Mediasoup.LoggerProxy do | |||
| } | |||
| end | |||
|
|
|||
| @type filter_fun :: (Record.t() -> boolean()) | |||
There was a problem hiding this comment.
When multiple filters are specified, it may be difficult to predict the behavior when each one returns true or false. How about modifying the filter function to return three values: :ignore, :log, or :stop?
It may return :log (to log the message), :stop (to not log the message), or :ignore (to ignore the filter).
like this
There was a problem hiding this comment.
I understood :log and :stop, but I can't imagine a scene where it would be necessary to :ignore. What kind of case are you considering?
There was a problem hiding this comment.
How about changing the levels as follows?
@moduledoc """
Proxy rust layer logs to elixir logger.
This module serves as a bridge between Rust's logging system and Elixir's Logger.
Filters
Filters can be used to control log output behavior. Each filter is a function that takes
a Record struct and returns one of these values:
:log- Allow the message to be logged{:log, Record.t()}- Allow the message to be logged with transformed log record:stop- Stop the message from being logged:ignore- Skip this filter and continue to the next one
Example:
filters: [
fn record ->
if String.contains?(record.body, "sensitive") do
:stop
else
:ignore
end
end
]
There was a problem hiding this comment.
I want to customize the log level, but how do you control it?
There was a problem hiding this comment.
I understood :log and :stop, but I can't imagine a scene where it would be necessary to :ignore. What kind of case are you considering?
I think your current implementation would be equivalent to specifying the level as :off return false.
For example, when there are multiple filters that you don’t want to log, like the following:
LoggerProxy.start_link(
max_level: :debug,
filters: [
fn record ->
# stop log sensitive data
if String.contains?(record.body, "sensitive") do
:stop
else
:ignore
end
end,
fn record ->
# stop log can_consume error
if record.level === :error && record.target === "mediasoup::router" &&
Regex.match?(~r/can_consume\(\) \| Producer with id "(?<id>[^"]+)" not found/, record.body) do
:stop
else
:ignore
end
end,
]
)There was a problem hiding this comment.
I want to customize the log level, but how do you control it?
LoggerProxy.start_link(
max_level: :debug,
filters: [
fn record ->
# stop log can_consume error
if record.level === :error && record.target === "mediasoup::router" &&
Regex.match?(~r/can_consume\(\) \| Producer with id "(?<id>[^"]+)" not found/, record.body) do
{:log, Map.put(record, :level, :info)}
else
:ignore
end
end
]
)There was a problem hiding this comment.
You mean, is a log output when the filter returns :ignore?
There was a problem hiding this comment.
If :ignore is returned, the next filter is processed.
If all filters return :ignore, the filter function has no effect, and the log is output.
The function I envision for processing filters is as follows:
def handle_info(%Mediasoup.LoggerProxy.Record{} = msg, %{filters: filters} = state) do
Enum.reduce_while(filters, {:log, msg}, fn filter, acc ->
case filter.(msg) do
:log -> {:halt, acc}
{:log, changed} -> {:halt, {:log, changed}}
:stop -> {:halt, :stop}
_ -> {:cont, acc}
end
end)
|> case do
{:log, msg} ->
Logger.log(msg.level, msg.body, %{
line: msg.line,
file: msg.file,
mfa: msg.target,
module_path: msg.module_path
})
end
{:noreply, state}
endThere was a problem hiding this comment.
Ahhh, I got it.
When we specify multiple filter functions,
:stopStop logging at that point, subsequent filter functions are not called:ignoreDo nothing with the current filter function, and leave it to the next filter function
correct?
There was a problem hiding this comment.
I think your current implementation would be equivalent to specifying the level as :off.
My previous explanation was incorrect.
In the current implementation, false corresponds to :ignore.
h3poteto
force-pushed
the
feat/filter-level
branch
from
ee826a5 to
4a15df0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/logger_proxy.ex (1)
69-78: Consider adding error handling for unexpected filter return values.While the implementation is solid, it might be good to log unexpected filter return values for debugging purposes.
def handle_info(%Mediasoup.LoggerProxy.Record{} = msg, %{filters: filters} = state) do with {:log, msg} <- Enum.reduce_while(filters, {:log, msg}, fn filter, acc -> case filter.(msg) do :log -> {:halt, acc} {:log, changed} -> {:halt, {:log, changed}} :stop -> {:halt, :stop} - _ -> {:cont, acc} + unexpected when unexpected not in [:ignore] -> + Logger.warn("Unexpected filter return value: #{inspect(unexpected)}") + {:cont, acc} + _ -> {:cont, acc} end end) dotest/logger_proxy_test.exs (2)
112-156: Consider extracting test data setup for better readability.While the test is comprehensive, the test data setup could be extracted to improve readability.
+ @test_router_options %{ + codecs: [], + headerExtensions: [], + fecMechanisms: [] + } + test "can_consume error should be warn" do pattern = ~r/can_consume\(\) \| Producer with id "(?<id>[^"]+)" not found/ ... assert capture_log([level: :warn], fn -> - Router.can_consume?(router, "d117b485-7490-4146-812f-d3f744f0a8c7", %{ - codecs: [], - headerExtensions: [], - fecMechanisms: [] - }) + Router.can_consume?(router, "d117b485-7490-4146-812f-d3f744f0a8c7", @test_router_options)
158-190: Consider adding a test case for filter order precedence.The test covers basic multiple filter scenarios well. Consider adding a test case where the order of filters matters to ensure the precedence behavior is documented.
test "multiple filters" do LoggerProxy.start_link( max_level: :debug, filters: [ fn msg -> if msg.level === :debug && Regex.match?(~r/test/, msg.body) do {:log, Map.put(msg, :level, :info)} else :ignore end end, fn msg -> if msg.level === :warn && Regex.match?(~r/test/, msg.body) do :stop else :ignore end end ] ) assert capture_log([level: :info], fn -> Mediasoup.Nif.debug_logger(:debug, "debug test") Mediasoup.Nif.debug_logger(:warn, "warn test") Process.sleep(10) end) =~ "debug test" refute capture_log(fn -> Mediasoup.Nif.debug_logger(:info, "info test") Mediasoup.Nif.debug_logger(:warn, "warn test") Process.sleep(10) end) =~ "warn test" + + # Test filter order precedence + LoggerProxy.start_link( + max_level: :debug, + filters: [ + fn msg -> if msg.level === :warn, do: :stop, else: :ignore end, + fn msg -> if msg.level === :warn, do: {:log, Map.put(msg, :level, :info)}, else: :ignore end + ] + ) + + refute capture_log(fn -> + Mediasoup.Nif.debug_logger(:warn, "warn test") + Process.sleep(10) + end) =~ "warn test" end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/logger_proxy.ex(2 hunks)test/logger_proxy_test.exs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and test (stable)
- GitHub Check: lint (stable)
- GitHub Check: Lint Rust (native/mediasoup_elixir/Cargo.toml)
🔇 Additional comments (3)
lib/logger_proxy.ex (2)
49-52: LGTM! Well-designed type definitions.The new filter function type and config type provide excellent flexibility for log filtering and level overriding.
57-61: LGTM! Clean implementation of the new filter list structure.The changes correctly handle the new filters configuration while maintaining backward compatibility with an empty list default.
test/logger_proxy_test.exs (1)
88-99: LGTM! Comprehensive test coverage for debug message filtering.The test effectively verifies both the stopping of debug messages and the passing through of other messages.
h3poteto
force-pushed
the
feat/filter-level
branch
from
4a15df0 to
e2c622a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/logger_proxy_test.exs (4)
88-110: Consider adding edge cases to strengthen test coverage.The test effectively demonstrates the basic filtering functionality, but consider adding test cases for:
- Messages with undefined or nil levels
- Multiple messages of different levels in rapid succession
- Messages at the boundary of debug/info levels
115-122: Document the filter's transformation behavior.Consider adding a module or function doc explaining:
- The specific conditions that trigger the level transformation
- Why certain errors are downgraded to warnings
- The expected behavior for non-matching messages
158-184: Document filter execution order and precedence.The test demonstrates multiple filters working together but could benefit from:
- Explicit documentation of filter execution order
- Comments explaining the expected behavior of each filter
- Examples of how filters interact when multiple filters match
Add comments to clarify the filter chain:
filters: [ + # Filter 1: Allows messages containing "should be logged" fn msg -> if Regex.match?(~r/should be logged/, msg.body) do :log else :ignore end end, + # Filter 2: Upgrades debug messages containing "test" to info fn msg -> if msg.level === :debug && Regex.match?(~r/test/, msg.body) do {:log, Map.put(msg, :level, :info)} else :ignore end end, + # Filter 3: Blocks warn messages containing "test" fn msg -> if msg.level === :warn && Regex.match?(~r/test/, msg.body) do :stop else :ignore end end ]
186-203: Improve test case organization for clarity.The assertions could be grouped more logically to demonstrate the filter chain's behavior. Consider organizing them by filter functionality:
- Tests for the content-based filter
- Tests for the level transformation
- Tests for the message blocking
- assert capture_log([level: :info], fn -> - Mediasoup.Nif.debug_logger(:debug, "debug test") - Mediasoup.Nif.debug_logger(:warn, "warn test") - Process.sleep(10) - end) =~ "debug test" - - refute capture_log(fn -> - Mediasoup.Nif.debug_logger(:info, "info test") - Mediasoup.Nif.debug_logger(:warn, "warn test") - Process.sleep(10) - end) =~ "warn test" - - assert capture_log(fn -> - Mediasoup.Nif.debug_logger(:warn, "warn test") - Mediasoup.Nif.debug_logger(:info, "should be logged") - Process.sleep(10) - end) =~ "should be logged" + # Test content-based filter + assert capture_log(fn -> + Mediasoup.Nif.debug_logger(:info, "should be logged") + Process.sleep(10) + end) =~ "should be logged" + + # Test level transformation (debug -> info) + assert capture_log([level: :info], fn -> + Mediasoup.Nif.debug_logger(:debug, "debug test") + Process.sleep(10) + end) =~ "debug test" + + # Test message blocking + refute capture_log(fn -> + Mediasoup.Nif.debug_logger(:warn, "warn test") + Process.sleep(10) + end) =~ "warn test"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/logger_proxy.ex(2 hunks)test/logger_proxy_test.exs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/logger_proxy.ex
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint (stable)
- GitHub Check: Lint Rust (native/mediasoup_elixir/Cargo.toml)
- GitHub Check: Build and test (stable)
I want to change the log level that match the filter. Currently, the filter outputs only matching logs.
However, what I desire is to remove specified logs or change the level of the log.
Summary by CodeRabbit
New Features
Improvements
Tests