cheshire icon indicating copy to clipboard operation
cheshire copied to clipboard

feature: context specific encoders

Open noisesmith opened this issue 11 years ago • 18 comments

I would like to make a structured logger with JSON output, but one catch would be that you might want a different output for a given object when logging as compared to other usage of JSON in the same application.

Is there an obvious way to implement this with the existing cheshire design?

Is this a feature that would be welcome for cheshire if I implemented it?

noisesmith avatar Apr 04 '15 18:04 noisesmith

Is there an obvious way to implement this with the existing cheshire design?

Not that I can immediately think of, I will have to experiment to see if there already is.

Is this a feature that would be welcome for cheshire if I implemented it?

Yes, definitely would be nice as long as it does not negatively affect performance.

dakrone avatar Apr 06 '15 17:04 dakrone

This has been a big pain point for me as well.

One option I just thought of is to have another option that can be passed of a :fallback-encoder -- an encoder function that gets used whenever the JsonGenerationException would otherwise be thrown. Presumably that wouldn't be a big perf problem (at least for people not using it) since it's only a code change in an otherwise-exceptional codepath.

For my use cases I think the fallback encoder would pretty much always just use str, so we could also consider adding a feature that just does that, or at least an easier way to opt into that than an actual encoder fn.

gfredericks avatar Jul 09 '15 18:07 gfredericks

Though now that I look through the implementation I remember that options are passed around as first-class args, and so having an extra option means expanding the signatures everywhere :/

gfredericks avatar Jul 09 '15 18:07 gfredericks

If the signature thing is a big perf deal, maybe an internal dynamic var that gets set once at the top level would be an alternative.

gfredericks avatar Jul 09 '15 18:07 gfredericks

I remember that options are passed around as first-class args, and so having an extra option means expanding the signatures everywhere :/

I'd need to check the performance hit for it, but if it's not too much, a breaking change (Cheshire 6.x) to switch to a map of options would be better for future development.

dakrone avatar Jul 09 '15 18:07 dakrone

I'll see if I can quickly make a branch with the options-map change.

gfredericks avatar Jul 09 '15 18:07 gfredericks

Okay, remember not to use destructuring as that (unfortunately) causes a big performance hit.

dakrone avatar Jul 09 '15 18:07 dakrone

do you just mean destructuring when you don't actually need all the keys? When I macroexpand a {:keys [foo bar]} destructuring the only extra thing I see is a (seq? m) check, is that the bad part?

gfredericks avatar Jul 09 '15 19:07 gfredericks

I haven't dug into why it's slower yet, only that it was slower in benchmarks (heck, maybe Clojure has caught up and it's better in recent versions?), so it would be good to test and see.

dakrone avatar Jul 09 '15 19:07 dakrone

okay; it ought to be identical to the macroexpanded code, but regardless I will leave it out so we don't have to wonder

gfredericks avatar Jul 09 '15 19:07 gfredericks

Okay I have some code here that seems to work except for some bug in the generate-seq code that I don't have time to chase down at the moment. Hopefully the non-seq code is correct and adequate for perf tests.

gfredericks avatar Jul 09 '15 19:07 gfredericks

(bug indicated by the one test failure)

gfredericks avatar Jul 09 '15 19:07 gfredericks

Okay I have some code here that seems to work

Great, thanks! No guarantees for when I will be able to look as my wife just had a baby and I am short on sleep/free-time, but I will try to take a look whenever I can! :)

dakrone avatar Jul 09 '15 20:07 dakrone

babies babies babies babies babies babies babies babies babies babies babies babies!

gfredericks avatar Jul 09 '15 21:07 gfredericks

@gfredericks I finally had a second to check out your branch. Performance-wise it looks good to me, would you be able to open a PR for moving to it?

dakrone avatar Sep 15 '15 17:09 dakrone

Will do

gfredericks avatar Sep 15 '15 18:09 gfredericks

In terms of an API for extending via opts, the code used in https://github.com/greglook/puget/issues/23 will probably be relevant.

gfredericks avatar Oct 05 '15 12:10 gfredericks

Any news on this? Looking also forward to this, config similar like the edn / transit readers?

ikitommi avatar Jul 15 '16 11:07 ikitommi