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

extended json serialization #511

Closed
wants to merge 1 commit into from
Closed

Conversation

antonkulaga
Copy link
Contributor

I added more types for json serialization and tests for them. The tests run only if the libraries are installed.

@itamarst
Copy link
Owner

itamarst commented Feb 3, 2025

Thank you!

I like the functionality. I'm a little worried this will make logging slower, but I guess you only hit the path if it's not one of the types orjson already supports, so that's a start.

Making things faster:

  • orjson already does some of these types (https://github.com/ijl/orjson?tab=readme-ov-file#serialize), so writing serializers for those may not be necessary. That's just deleting some unnecessary code from the PR.
  • Instead of doing lots of isinstance(), dispatch on the type. This makes it harder to deal with subclasses, though I guess you can do the thing where you get all parent classes and dispatch on each. Though again this'll make it slower, but hopefully not as much. Building on getting parent classes technique, it may be possible to use a little caching so once you've seen a type, you know how to serialize it, so in the common case it's self._prep_for_json[type(obj)](obj) which isn't bad. I'd be OK with this as a second PR, or just filing an issue if you don't want to do it.

Will try to take a deeper look later this week.

@antonkulaga
Copy link
Contributor Author

antonkulaga commented Feb 4, 2025

I checked and deleted those types that are serialized natively by orjson. It was a bit surprising that for example it could serialize datetime but failed on date and time separately. I do not think that several if-else will significantly slowdown the logging. If we can merge this PR I am open to continue and suggest the separate one with dispatch, but it should be separate PR as it will take time to agree on best apporach

@winternewt
Copy link

I'm also having a problem with Paths and other serializations. Wrapping random stuff in str() is tedious, what's missing in this PR for it to be merged? Isn't functools dispatch a bit slower than isinstance chain?

@itamarst
Copy link
Owner

itamarst commented Feb 8, 2025

@winternewt There's nothing stopping you from writing your own serialization code, Eliot has very easy ability to add custom destination for your logs: https://eliot.readthedocs.io/en/stable/outputting/output.html#configuring-logging-output.

@itamarst
Copy link
Owner

itamarst commented Feb 8, 2025

This will get merged when I have some free time.

@itamarst
Copy link
Owner

itamarst commented Feb 8, 2025

Oh, and the FileDestination constructor lets you override the json default function, so you can trivially hook in your own: https://github.com/itamarst/eliot/blob/master/eliot/_output.py#L456

So certainly it would be nicer to have this built-in, but there's really nothing stopping you from customizing JSON serialization in your own code.

@Livia-Zaharia
Copy link

Was looking through and found this PR. It could be a pretty useful feature.

@itamarst
Copy link
Owner

I've created my own branch based on this, going to go work on this now.

@itamarst
Copy link
Owner

Will re-open PR with new branch when done updating. Thank you for working on this!

@itamarst itamarst closed this Feb 28, 2025
@itamarst itamarst mentioned this pull request Feb 28, 2025
@itamarst
Copy link
Owner

This feature is now released, as part of 1.17.5. Thank you!

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.

4 participants