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

RFC: Logging: enable configuration via ENV #26265

Merged
merged 1 commit into from
Apr 9, 2018
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 28, 2018

This restores the functionality we had previously to set JULIA_DEBUG_LOADING=1 to activate logging;
now the equivalent would be to set JULIA_DEBUG=loading. But with the new infrastructure, this is also
generalized to allow filtering on any key (module, root-module, or filename), or "all".
This sits between the global disable (which still provides a fast-path to skip everything),
and the default min-level for a logger (which will be overridden by the environment variable).

This also tries to move a bit more work into non-inlined helper functions,
rather than inlining that code directly, to slightly reduce the size of
the emitted code.

As a possible future improvement, we could allow specifying specific
levels (such as loading=MaxLevel or all=Debug), to provide even more
fine-grained control.

I know this needs-docs, but Logging doesn't currently appear to have any, so I have nowhere to put them :(

fix #25549

$ JULIA_DEBUG=loading ./usr/bin/julia -e 'using Colors'
┌ Debug: Rejecting cache file /home/vtjnash/.julia/compiled/v0.7/Colors.ji because module InteractiveUtils [b77e0a4c-d291-57a0-90e8-8db25a27a240] is already loaded and incompatible.
└ @ Base loading.jl:1344
┌ Debug: Recompiling stale cache file /home/vtjnash/.julia/compiled/v0.7/Colors.ji for module Colors
└ @ Base loading.jl:1190
┌ Debug: Rejecting cache file /home/vtjnash/.julia/compiled/v0.7/FixedPointNumbers.ji because module InteractiveUtils [b77e0a4c-d291-57a0-90e8-8db25a27a240] is already loaded and incompatible.
└ @ Base loading.jl:1344
┌ Debug: Recompiling stale cache file /home/vtjnash/.julia/compiled/v0.7/FixedPointNumbers.ji for module FixedPointNumbers
└ @ Base loading.jl:1190
┌ Debug: Rejecting cache file /home/vtjnash/.julia/compiled/v0.7/ColorTypes.ji because module InteractiveUtils [b77e0a4c-d291-57a0-90e8-8db25a27a240] is already loaded and incompatible.
└ @ Base loading.jl:1344
┌ Debug: Recompiling stale cache file /home/vtjnash/.julia/compiled/v0.7/ColorTypes.ji for module ColorTypes
└ @ Base loading.jl:1190
┌ Debug: Rejecting cache file /home/vtjnash/.julia/compiled/v0.7/Compat/GSFW.ji because module InteractiveUtils [b77e0a4c-d291-57a0-90e8-8db25a27a240] is already loaded and incompatible.
└ @ Base loading.jl:1344
┌ Debug: Recompiling stale cache file /home/vtjnash/.julia/compiled/v0.7/Compat/GSFW.ji for module Compat
└ @ Base loading.jl:1190
┌ Warning: `names(m, all)` is deprecated, use `names(m, all=all)` instead.
│   caller = top-level scope
└ @ Core :0
┌ Debug: Rejecting cache file /home/vtjnash/.julia/compiled/v0.7/Reexport.ji because module InteractiveUtils [b77e0a4c-d291-57a0-90e8-8db25a27a240] is already loaded and incompatible.
└ @ Base loading.jl:1344
┌ Debug: Recompiling stale cache file /home/vtjnash/.julia/compiled/v0.7/Reexport.ji for module Reexport
└ @ Base loading.jl:1190
WARNING: importing deprecated binding Base.@sprintf into Colors.
WARNING: Base.@sprintf is deprecated: it has been moved to the standard library package `Printf`.
Add `using Printf` to your imports.
  likely near /home/vtjnash/.julia/v0.7/Colors/src/utilities.jl:12
WARNING: Base.@sprintf is deprecated: it has been moved to the standard library package `Printf`.
Add `using Printf` to your imports.
  likely near /home/vtjnash/.julia/v0.7/Colors/src/utilities.jl:24

@vtjnash vtjnash added the logging The logging framework label Feb 28, 2018
@StefanKarpinski
Copy link
Member

Sweet!

@rofinn
Copy link
Contributor

rofinn commented Feb 28, 2018

IMHO, configuring logging based on environment variables might not be the best (or most maintainable) approach.

@StefanKarpinski
Copy link
Member

Since we currently have no obvious way to configure and use logging and little to no documentation on how to do so, this seems like a vast improvement.

@rofinn
Copy link
Contributor

rofinn commented Feb 28, 2018

Seems like we should address the lack of logging configuration options and documentation instead, but I guess if we aren't concerned about needing to maintain this functionality then that's fine for now.

@StefanKarpinski
Copy link
Member

I don't really see what's so bad about having an environment variable in addition to other ways to control logging.

@c42f
Copy link
Member

c42f commented Mar 1, 2018

I like the functionality this brings, but I think the implementation should live in the shouldlog function. In principle the non-swappable parts of CoreLogging shouldn't need to change much at all to support this.

I've got a branch where I've been struggling with logger configuration and more flexible filtering to address more or less the same problems (see also #25404). Let me clean that up and create a PR which we can compare.

Regarding documentation, see #26005 which is basically complete (it's not merged as it got sidetracked into design rather than just documenting what's done).

@vtjnash
Copy link
Member Author

vtjnash commented Mar 1, 2018

I'll admit that I don't see any reason for shouldlog to exist – it just seemed way too complicated to expect each Logging type to reimplement compatible functionality – so I just ignored it in this PR. This also can't be sunk into shouldlog anyways, since it would already have been filtered out by the LogState.

@c42f
Copy link
Member

c42f commented Mar 1, 2018

Yep, expecting each logger to reimplement filtering in a compatible way is a problem, which is why I'm struggling with the logger configuration design. But I don't think that means we need to bake all this into CoreLogging and make it depend on global state. It would be better to provide shared log filtering functionality which Loggers can use so they don't need to reimplement this stuff.

The LogState problem isn't insurmountable, it just means that logger configuration needs a way to re-cache the min level when the logger is modified.

@vtjnash
Copy link
Member Author

vtjnash commented Mar 7, 2018

But I don't think that means we need to bake all this into CoreLogging and make it depend on global state

What? It absolutely should depend on global state. That's the entire point of this PR.

@maleadt
Copy link
Member

maleadt commented Apr 6, 2018

Any movement here? I've been gradually moving my package's logging over to Logging, but it really needs a knob to enable per-module debug messages.

@StefanKarpinski
Copy link
Member

I'm tempted to run this through CI again and just merge it. Other means of tweaking log output can be added in the future, but the lack of any way to do this now is a bit of a problem.

This restores the functionality we had previously to set JULIA_DEBUG_LOADING=1 to activate logging;
now the equivalent would be to set JULIA_DEBUG=loading. But with the new infrastructure, this is also
generalized to allow filtering on any key (module, root-module, or filename), or "all".
This sits between the global disable (which still provides a fast-path to skip everything),
and the default min-level for a logger (which will be overridden by the environment variable).

This also tries to move a bit more work into non-inlined helper functions,
rather than inlining that code directly, to slightly reduce the size of
the emitted code.

As a possible future improvement, we could allow specifying specific
levels (such as `loading=MaxLevel` or `all=Debug`), to provide even more
fine-grained control.

fix #25549
@maleadt
Copy link
Member

maleadt commented Apr 9, 2018

CI looks good, with only circle failing to OOM during build.

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

Successfully merging this pull request may close these issues.

enable debug logging via environment variable
5 participants