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 dynamics field #12

Closed
wants to merge 1 commit into from
Closed

Add dynamics field #12

wants to merge 1 commit into from

Conversation

killzoner
Copy link

Hi there.

I have been searching for a decently configurable crate for tracing in JSON, unfortunately the default one has very few options.
I turns out yours, while aimed at logback is much more flexible.

I'm having a use case where I need to add constant fields (for which with_constants is a perfect match), and I also have the need for a key "time" instead of "@timestamp" on all records.

I added with_dynamics to allow that and that makes this crate a very general purpose JSON formatter for tracing while defaulting to lockback format.

I have been wondering if it would be better to have Serialize from serde in with_dynamics signature, but googling a few time gave me only "you can't add bound trait in function signatures".

Happy to discuss the matter!

@killzoner
Copy link
Author

Tagging @gsson as I'm not sure you get notification without reviewer or assignee on the PR (sorry if it did)

@gsson
Copy link
Owner

gsson commented Jan 8, 2024

No I got the notification, I've just been offline for a little bit!

I like the idea and I don't see any reason not to include something like it.

I'll try to look at it today closer today -- the only thing that stands out is the use of fn() which means it can't close over any data. I'm wondering if it should be an Option<Fn()> or something, but I need some time to look at the PR properly first!

@killzoner
Copy link
Author

Thanks a lot. I fixed the lint issue in the meantime

@killzoner
Copy link
Author

I did some other PR trying to adjust to your suggestion here #13

@gsson
Copy link
Owner

gsson commented Jan 8, 2024

I'm taking a bit of a lunch break now so I took a proper look :)

Code looks pretty good, but one thing you mentioned originally;

I have been wondering if it would be better to have Serialize from serde in with_dynamics signature, but googling a few time gave me only "you can't add bound trait in function signatures".

It would be nice to be able to serialize arbitrary types, and potentially reduce allocations by not having to allocate separate String/Vec objects 🤔

I did a quick prototype that seems to do something; I posted a separate PR for it

Do you think that could do what you wanted? There's an example usage in the tests, but consider it a draft for now

@killzoner
Copy link
Author

Hello @gsson your PR is actually what I would have liked to do in the first place, it's just my Rust is a bit limited rn as it's not my day to day language 😅

This would be a great addition and avoid allocations at the same time compared to my proposal

@killzoner
Copy link
Author

Closing in favor of #13

@killzoner killzoner closed this Jan 8, 2024
@killzoner killzoner deleted the additionnal_fields branch January 8, 2024 13:58
@gsson
Copy link
Owner

gsson commented Jan 8, 2024

Thanks 😁

I'll see about adding a few more tests and docs for it and get it released as soon as I can find some time again

@gsson
Copy link
Owner

gsson commented Jan 8, 2024

@killzoner: Just released this in 0.7.0, give it a go and let me know how it works out!

@killzoner
Copy link
Author

Hey @gsson

I already adapted from your branch earlier today and it works well.

Plus, you get handling of nested constant values out of the box, which is really nice.

Will do the switch asap to released version

Thank you very much !

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.

2 participants