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

Hard coded min level for TransformerLogger #93

Closed
theogf opened this issue Aug 15, 2024 · 9 comments
Closed

Hard coded min level for TransformerLogger #93

theogf opened this issue Aug 15, 2024 · 9 comments

Comments

@theogf
Copy link

theogf commented Aug 15, 2024

I was surprised to see that

min_enabled_level(transformer::TransformerLogger) = BelowMinLevel

I don't understand why it is not

min_enabled_level(transformer::TransformerLogger) = min_enabled_level(transformer.logger)
@oxinabox
Copy link
Member

Because each component in the compositional logging system serves one purpose.
The TransformerLogger transforms.
If you want to filter based on level, wrap it in a MinLevelLogger

@theogf
Copy link
Author

theogf commented Aug 16, 2024

That's fair, but following this logic I would expect TransformerLogger to leave min level of the logger it wraps untouched?

It feels to me like setting a hardcoded min level is an additional action from TransformerLogger?

@fredrikekre
Copy link
Member

It feels to me like setting a hardcoded min level is an additional action from TransformerLogger?

Perhaps think of it like this: The default setting for a logger is BelowMinLevel, i.e. no filtering based on level so this isn't an extra action. If the wrapped logger filters based on another level it should be that logger who actually gets to do the filtering. With your suggestion the filtering would move up the chain.

@oxinabox
Copy link
Member

That's fair, but following this logic I would expect TransformerLogger to leave min level of the logger it wraps untouched?

It does, doesn't it?

We check that here:

if comp_handle_message_check(transformer.logger, args...; kwargs...)

using:
(min_enabled_level(logger) <= level && shouldlog(logger, level, _module, group, id)) ||

and respect it.

I guess as an implementation detail we could have put that info into our min_enabled_level overload.
But it doesn't change behavour.

@theogf
Copy link
Author

theogf commented Aug 19, 2024

I am very confused now. I opened this issue cause I was surprise by the behavior of

with_logger(TransformLogger(identity, ConsoleLogger(Info))) do
  @debug "A"
  @info "B"
end

which returned both the @debug and @info.
But now that I tried again it behaves as I expected, returning the @info only...

@oxinabox
Copy link
Member

Spooky.
I am going to close this for now.
Reopen, or open a new one if it reoccurs.

@theogf
Copy link
Author

theogf commented Aug 19, 2024

If I can ask a design question related to this.

When composing loggers (ignoring the Tee one), one can view it as a pipeline.

So we can have something like MinLevelLogger(TransformerLogger(identity, ConsoleLogger()), Debug).
However unlike function composition, the log argument is seen first by the outermost logger MinLevelLogger here.

What if in my code I would like to keep transform my logs based on the code path?

I wanted to do the following but it obviously does not work due to my issue above.

logger = MinLevelLogger(TransformerLogger(identity, ConsoleLogger()), Warn)

a = rand(1:2)
with_logger(TransformerLogger(log -> log._line = a, logger) do
  # .... more stuff
end

This will work, however all @debug statements will now be executed first and filtered later...
Is there another way to do this while keeping the early filtering happening?

@theogf
Copy link
Author

theogf commented Aug 19, 2024

I am very confused now. I opened this issue cause I was surprise by the behavior of

with_logger(TransformLogger(identity, ConsoleLogger(Info))) do
  @debug "A"
  @info "B"
end

which returned both the @debug and @info. But now that I tried again it behaves as I expected, returning the @info only...

Regarding this, I think I misremembered the behavior. What I had was

with_logger(TransformLogger(identity, ConsoleLogger(Info))) do
  @debug error("Oh no!")
  @info "B"
end

(where the error came from an external package)

So no 👻 yet :)

@oxinabox
Copy link
Member

This will work, however all @debug statements will now be executed first and filtered later...
Is there another way to do this while keeping the early filtering happening?

Not currently.

I could imagine we could introduce an optimize_log_pipeline that reordered compositional loggers to move MinLevelLoggers and EarlyFilteredLoggers to be before ActiveFilteredLoggers and TransformerLoggers.
Which is semantically identical, but avoids executing the expressions in logs that might later be filtered.

Broadly speaking this is how our LevelOverrideLogger works.
It doesn't re-arrange but it does insert into non-top positioned in the pipeline.

# Propagating the constructor down to places it is needed

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

3 participants