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

A more pure design #9

Merged
merged 20 commits into from
Apr 21, 2019
Merged

A more pure design #9

merged 20 commits into from
Apr 21, 2019

Conversation

oxinabox
Copy link
Member

This PR solves all the philosphiocal issues I had with the earlier version.
Other than the basically indicidental improvement to UX from the switch to giving the filter named tuples, it doesn;t change much in the capacity of LoggingExtras.

FileLogger is now a pure sink, so accepts 100% of everything given to it. (#8)

to allow it to be not a problem we introduced more levels of filter,
MinLevelLogger, EarlyFilteredLogger (#6) and ActiveFilteredLogger.

The ActiveFilteredLogger is a revised version of the previous FilteredLogger.
In the new version (and in the EarlyFilteredLogger) rather than passing to the user specified functions like 7 different arguments, we pass them a named tuple. (solving #7).

One thing to note is that compositional loggers,
like the filtered loggers and the DemuxLogger,
to have to make sure to check the shouldlog and min_enabled_level of there children.
Which is a bit ugly.

But these are almost all the composable loggers that I can imagine.
Except for the TransformingLogger (#5).
All filtering you might ever want to do can be configured using the 3 filtering loggers
and a filtering function or closure.
As such no-one should ever have to write a composable logger again,
except for writing pure sinks (like the FileLogger),
and writing pure sinks doesn't run into that uglyness of having to check your children.
So I think that that little big of uglyness is fine.

@codecov-io
Copy link

codecov-io commented Apr 13, 2019

Codecov Report

Merging #9 into master will decrease coverage by 19.67%.
The diff coverage is 80.7%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #9       +/-   ##
===========================================
- Coverage     100%   80.32%   -19.68%     
===========================================
  Files           3        7        +4     
  Lines          19       61       +42     
===========================================
+ Hits           19       49       +30     
- Misses          0       12       +12
Impacted Files Coverage Δ
src/filelogger.jl 87.5% <100%> (-12.5%) ⬇️
src/LoggingExtras.jl 100% <100%> (ø)
src/earlyfiltered.jl 55.55% <55.55%> (ø)
src/minlevelfiltered.jl 57.14% <57.14%> (ø)
src/activefiltered.jl 81.81% <81.81%> (ø)
src/transformer.jl 90% <90%> (ø)
src/demux.jl 90.9% <90.9%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5114301...1c26386. Read the comment docs.

@oxinabox
Copy link
Member Author

Added Transforming logger,
closing #5.

And a discussion of what compositional logging is.

@oxinabox
Copy link
Member Author

@c42f this is the outcome of that discussion we had a few (6?) months ago.
Thoughts?

README.md Outdated Show resolved Hide resolved
src/activefiltered.jl Outdated Show resolved Hide resolved
src/minlevelfiltered.jl Outdated Show resolved Hide resolved
Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

This is an interesting approach. I wasn't quite sure that filtering and transformation are orthogonal, but the benefit of deciding that they are is that it's simple: they can be defined with a single function each.

For now I've just added some high level comments without looking very carefully at the implementation.

I really should finish my (mostly equivalent-in-spirit) version of the same thing so we can compare notes better. My prototype had a few differences, perhaps the most obvious one being that filters / transformers were not AbstractLoggers, but rather a separate filter type. This meant filters could be composed together and instantiated without reference to the underlying sink. This is helpful because it allows for things like a function filterlogs(SomeFilter(args...)) do ... rather than the pattern with_logger(SomeWrapper(current_logger(), args...)) do ... which kind of gets annoying.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@c42f
Copy link
Member

c42f commented Apr 18, 2019

By the way, I'm reviewing this with the thought that some successor or closely related sibling of this code will end up in Logging which is why I mention various annoying design issues which aren't strictly related to this PR. I think what you've got here is already a nice improvement.

I think we can (carefully) change core logging to make it easier to write backends. The only thing which is really set in stone at this point is the syntax of the logging macros themselves. Changes to those would be julia-2.0 material.

@oxinabox oxinabox merged commit 0ee09fc into master Apr 21, 2019
@oxinabox oxinabox deleted the ox/philo branch April 21, 2019 17:45
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.

3 participants