viesti / timbre-json-appender

JSON appender for Timbre
MIT License
44 stars 11 forks source link

Support maps as args #12

Closed Limess closed 3 years ago

Limess commented 3 years ago

This allows data to be provided directly to the log function, rather than requiring flattening key-values to keywords.

I found the current behaviour slightly confusing as the timbre documentation doesn't seem to prescribe a particular format, and using data rather than keywords seems to be more idiomatic Clojure.

I also find the message being an arbitrary type confusing, it may be simpler if it's assumed this will always be a string, and if it is then checked as such.

As this would be a breaking change - it may be viable to instead put this behind a flag?

Example

To achieve the same output you can use:

(timbre/info "Doing Operation" {:duration 5 :operation "123"})
(timbre/info {:duration 5 :operation "123"})

instead of

(timbre/info "Doing Operation" :duration 5 :operation "123")
(timbre/info :duration 5 :operation "123")
viesti commented 3 years ago

Passing a map to log invocation seems really natural, why didn't I think of this before :)

It's funny how the structured logging invocation seems to have a kind of tension on how one builds the data to be logged. Might be that there's some unlearning for me personally :) This being Clojure, passing a data map into the logging call seems natural. And still keeping a more keyword argument style way of adding select data into the logging call is also nice.

I think you have a very good changeset here :) I took only a brief look now, but is there actually a breaking change here? To me, it seems pretty good as it is even :)

viesti commented 3 years ago

I also accepted the version bump change (#13), the conflict on "src/timbre_json_appender/core.clj" is probably because of that. For some reason, kaocha 1.0.830 wasn't on clojars yet, so I backed to 1.0.829 for now.

Limess commented 3 years ago

I guess the breaking change is that the log output will change if users were calling with the string/map args but it's pretty niche and the API suggested using a string + keywords or purely keywords.

Thanks for fixing the Kaocha version, I had just picked the latest release from the github when bumping the other packages.

I'll rebase this now.

Limess commented 3 years ago

Done, also fixed a couple of warnings I saw when compiling/running tests (Kaocha wants a config file now, and there were a small handful of reflection warns).

viesti commented 3 years ago

Thank you! Actually, when :inline-args? was introduced, it became the default, which also changed the default shown in the first example in the readme 😅. Seems I don't fare well with major.minor.patch versioning 😓

I think I'll merge these changes. The first example in the readme can be fixed separately to match the current default and a 0.2.0 entry in the changelog can also be started, just to note that things have progressed :).

Limess commented 3 years ago

Hey @viesti, do you have plans to release this? I'd like to put it to use.

Thanks!

viesti commented 3 years ago

Ah yeah, forgot about this. I'll see if I can do a Friday release :)

viesti commented 3 years ago

Lucky I found a release script :D 0.2.0 should be now in clojars!