zalando / friboo

Utility library for writing microservices in Clojure, with support for Swagger and OAuth
Apache License 2.0
119 stars 16 forks source link

friboo.log limitations. #118

Open PetrGlad opened 7 years ago

PetrGlad commented 7 years ago

They should be documented or resolved.

  1. Log value that has no JSON serializer (I'd prefer Clojure serialization, but that's a matter of preference)
    (org.zalando.stups.friboo.log/info "Message %" (reify Runnable (run [this])))
    ;=> JsonGenerationException Cannot JSON encode object of class: class user$eval37467$reify__37468: user$eval37467$reify__37468@79fe11e0  cheshire.generate/generate (generate.clj:154)
  2. friboo.log assumes that only errors are exceptions. I have some checks that might detect problems. I cannot log those problems at error level, barring following hack that is :)
    (org.zalando.stups.friboo.log/error (RuntimeException. "Error") "This is an error but not an exception.")
PetrGlad commented 7 years ago

Leaving other inconveniences aside, like misleading data structures logging: square brackets produced by formatter are not part of structure: (log/info "The list is %s" [1 2 3]) --> "The list is [[1, 2, 3]]"

sarnowski commented 7 years ago

For the "error and exception" discussion: this depends on your logging strategy. By our definition, an "error" is an unrecoverable error situation. This normally means, you want to leave the normal code flow. So there are these two error cases:

sarnowski commented 7 years ago

The brackets [ ] are part of our logging guidelines to explicitly mark dynamic values - having double brackets for vector values makes sense here.

(rationale: it makes corner cases even more spottable like empty strings, null values or strings that would confuse the reader if not explicitly marked as value)

Otann commented 7 years ago

For the "error and exception" discussion: If you mean that only central error logger exists, why then friboo have error function in the first place?

For instance, I received nil from a channel (where unrecoverable exception happened) and would like to consider this an error and log it with proper formatting and explanation. In your definition — should I throw an exception instead and hope that central logger would print it correctly?

For the brackets: This was quite a surprise for me as well and threw me off for a while. I had to scratch my head intensively to understand why I have double-nested vector.

I also vote that this is something that should be documented and be very clear or maybe different enclosing symbols could be chosen, like '{:foo "bar"}'

PetrGlad commented 7 years ago

Regarding [ ], in this case all values are rendered by friboo.log as JSON so empty or white-space strings are never the case. Therefore these braces are are always redundant.

I feel that writing all values is not much of convenience since Clojure can perfectly print it's values but we already have this and I'd prefer to take it into account.

PetrGlad commented 7 years ago

Regarding exceptions as errors, while yes, in most cases exception is the reasonable way to handle situation, but sometimes the purpose of the code is to detect error which is normal flow in that case :)