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

Add leveled logging #16

Closed
wants to merge 3 commits into from
Closed

Conversation

arnaudbos
Copy link
Contributor

@arnaudbos arnaudbos commented Apr 4, 2020

I've tried to provide a mechanism for leveled logging inside μ/log which leverages local contexts and transformation functions for filtering.

There are three standalone commits:

  1. Fix typos and μ/log style
    No code changes, only docs and docstrings.
  2. Add support for leveled logging
    • Leveraging local contexts I've created a bunch of macros related to some usual log levels (verbose, debug, info, warning, error, fatal) stored in a default hierarchy.
    • Then I've added an optional level configuration in the built-in (console, simple file, elasticsearh & kafka) publishers, to compose a filter (based on the provided log level configuration) with user-provided custom transformation functions.
    • I've also documented the feature and provided examples for using the log levels as well as how to use the global context to setup a default level.
    • And finally I've added a section in "How to write custom publishers" on how to leverage the built-in leveled logging mechanism itself, or the ->filter helper alone, in order to implement custom leveled logging.
    • All tests pass and the example project logs as expected.
  3. Deprecated :transform in favor of transducers via :transduce
    • I've had some fun turning custom transformation functions and log level filter into transducers in order to compose more easily and avoid intermediate collections on arbitrary user logic.
    • The transformation functions are still supported for backward compatibility, but they are wrapped inside a stateful transducer.
      The logic is: :transduce, if provided, takes precedence over :transform which, if provided is wrapped in a stateful transducer and takes precedence over the default (map identity).
    • Although I don't think it's a big gain in terms of speed/memory efficiency because users probably won't do a lot of transformation (although we can't know) and the buffer size is not supposed to be *"too big"*™️. But I like how they compose.
    • I've isolated this change in its own commit so that you could reject it more easily should the change not suit you.
    • I've updated the documentation to reflect the change and explicitly declare the :transform configuration of the built-in publishers as deprecated in favor of :transduce.
    • All tests pass and the example project logs as expected.

I hope this is interesting to you and look forward to your feedback 🙂

Copy link
Owner

@BrunoBonacci BrunoBonacci left a comment

Choose a reason for hiding this comment

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

As mentioned in #17 I don't think I should add the concept of levels into the core.

Could you please separate the type+style fixes into a separate PR which I will merge immediately.

For the level PR, I strongly suggest you have this in your own namespace.
As far as I know, you should be able to make it work like shown here without requiring changes from the core.
You should be able to add the levels using the macros like in your PR and have the publisher showing only a specific set of level by filtering them with the transform

{:type :console
 :transform (partial filter (where :mulog/level :in? [:level/info :level/warn :level/error]))
 }

this example uses where

Finally regarding the tranform -> transduce I totally agree with you that transducers are a good fit here.
I was battled whether require the use to provide only an element transformation or a generic transform, or to force them to use transducers but at the end I decided that this shouldn't be my choice.
So tranform is just f -> events -> events and I don't care whether is implemented with transducers or not.

I leave you with a quote from Prof John Ousterhout

[...] the keys to good software design is knowing when you should not use information that you have in your mind!
Prof John Ousterhout

mulog-core/src/com/brunobonacci/mulog.clj Show resolved Hide resolved
(log ~event-name ~@pairs)))
(defmacro fatal [event-name & pairs]
`(with-context {:mulog/level ::lvl/fatal}
(log ~event-name ~@pairs)))
Copy link
Owner

Choose a reason for hiding this comment

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

if the {:mulog/level ::lvl/fatal} only applies to that one event then it is better to just add it as a pairs

(defmacro fatal [event-name & pairs]
  `(log ~event-name :mulog/level ::lvl/fatal ~@pairs))

the context is something you want to apply to all events within the scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True that! Thanks.

@arnaudbos
Copy link
Contributor Author

Thank you for taking the time to review the PR even though you're not sold on the idea.


For the level PR, I strongly suggest you have this in your own namespace.
As far as I know, you should be able to make it work like shown here without requiring changes from the core.

Yes! I've actually worked on the PR and submitted it because I thought I needed it and that it could be useful for someone else instead of re-implementing an ad-hoc layer in each project.

However, your comment on the issue made me think a lot. I haven't responded yet because I'm trying to think this through and form an articulate answer.


So tranform is just f -> events -> events and I don't care whether is implemented with transducers or not.

Sure, this makes sense. I proposed it because after adding level filters into the mix I thought that transducers would compose better, as you've acknowledged.

Anyways thank you for the quote. This is actually the reason why I spend an extra hour splitting my commit in two to isolate the changes related to transducers. But I still need to practice, obviously 🙂

I had a lot of fun playing re-reading a few posts and playing with transducers, so I consider this time well invested.

@BrunoBonacci
Copy link
Owner

Thanks again @arnaudbos for the other PR, and thanks also for this PR.
I appreciate you bouncing ideas off and I know it isn't a good feeling to have a PR, on which you invested a good amount of time, to be pushed back.

One colleague of mine warned me on the risk of calling this library *µ/log and pushing the analogy with a logging framework. His concern was that most of the ppl would discard the library as just another logging framework rather than an events management and observability solution.

@arnaudbos
Copy link
Contributor Author

No problem. As I said, I've had fun, and I don't mind the time as long as I learn something.
I'm closing the PR but leaving a last question if you don't mind: how would you have designed it should you have decided to add log levels? What did I do right/wrong?


One colleague of mine warned me on the risk of calling this library µ/log and pushing the analogy with a logging framework. His concern was that most of the ppl would discard the library as just another logging framework rather than an events management and observability solution.

Yes, this!

After you stated the reasons why you're not going to add level logging here, I was trying to formulate an answer, mostly questions about observability. I'm not done clearing my mind but, if anything, I think maybe adding a link to that comment at the end of the Motivation section may prove useful. It was insightful to me, at least.

As per the name, I don't think it's that much of an issue, but adding the "observability" buzzword in the tagline probably wouldn't hurt either?

μ/log is an observability focused micro-logging library that logs events and data, not words!

My 2 cents.

@arnaudbos arnaudbos closed this Apr 6, 2020
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.

None yet

2 participants