Skip to content

Conversation

oxinabox
Copy link
Member

This fixes the broken test for using with_level on top of a compositional logger.

I had hoped there was something we were doing wrong in the compositional components,
the fixing of which would make this all work elegantly.
But there is not.1

So instead this takes the brute force approach of recursively reconstructing the logging stack. (well tree)
Luckily there is only a very finite number of logging types we will ever have so we can just handle each case.

TODO:

  • More tests
  • Think about if we can ever stop propagating early based on min_enabled_level (I don't think we can)

1 I have noted before this compositional loggers feels like how logging in julia should work, but it does not get a clean implementation, for julia 2.0 we should change Logging some how, including incorperating compositional logging into the stdlib, and removing inpure sinks.)

@oxinabox oxinabox requested a review from quinnj May 27, 2022 23:34
@quinnj
Copy link
Member

quinnj commented May 28, 2022

  • Think about if we can ever stop propagating early based on min_enabled_level (I don't think we can)

I agree that I don't think this is possible w/ current Base.CoreLogging logic. The problem is the call to get_current_env_logger does the min_enabled_level check before we even get to see what the current logger is, so there's no chance to overload, it's just a static check on the min_enabled_level when the logger was set.

We should probably start a document around proposals for 2.0 CoreLogging improvements. I've got a few thoughts as I've been digging around a bunch. It'd be nice to get it all cleaned up.

@oxinabox
Copy link
Member Author

oxinabox commented Aug 1, 2022

shall i rebase and merge this, or should i add more tests?

@quinnj
Copy link
Member

quinnj commented Aug 10, 2022

Sorry, I've been on vacation the last 2 weeks. If it's alright, I'll try to circle back to this in the next few days, if we're ok waiting. I need to re-immerse myself with some of the logging stuff I was doing to get back in context.

@quinnj quinnj force-pushed the ox/fix_order_composition branch from f37cc4a to 6ea8fc5 Compare October 21, 2022 21:25
@quinnj quinnj merged commit 1172d4f into master Oct 22, 2022
@quinnj quinnj deleted the ox/fix_order_composition branch October 22, 2022 00:44
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

Successfully merging this pull request may close these issues.

2 participants