estk / log4rs

A highly configurable logging framework for Rust
Apache License 2.0
973 stars 143 forks source link

Add support for Key-Value pairs #362

Open ellttBen opened 4 months ago

ellttBen commented 4 months ago

Hi there, As discussed in #329, this PR implements support for the log::kv structured logging API. These are some user-facing choices I have made:

If this sounds okay, I can also edit the documentation.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 91.17647% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 64.09%. Comparing base (8ab1b34) to head (24257cf).

Files Patch % Lines
src/encode/pattern/mod.rs 88.46% 3 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #362 +/- ## ========================================== + Coverage 63.39% 64.09% +0.70% ========================================== Files 24 24 Lines 1557 1593 +36 ========================================== + Hits 987 1021 +34 - Misses 570 572 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ellttBen commented 2 months ago

Hi, sorry for the delay, I'm still keen to get this merged. I have rebased this PR and squashed the commits. If more documentation is needed I can get that in ASAP as well.

bconn98 commented 2 months ago

I'll take a look at this soon. I'll do my best to get a hold of the project owner and work through our recent batches of PRs

sdroege commented 2 weeks ago

What's the status of this PR?

bconn98 commented 2 weeks ago

@sdroege I am targeting next week to start working with the owner of the repo. Appreciate everyone's hard work on the recent batch of MRs and can't wait to get things moving again!

sdroege commented 2 weeks ago

That sounds great, thanks!

ellttBen commented 2 weeks ago

Thanks for the review @estk, I have followed your suggestion and investigated directly playing around with Serde traits. I implemented the suggested review changes, and also changed the approach for json encoding to directly (via a new wrapper type) serializing from the &log::kv::Source dynamic reference. The error handling is a bit finicky with transformations between log::kv::Error and the serializer errors (log::kv::Error::boxed requires Send+Sync, and the serde traits make no such guarantees about allowable errors), but I think overall it makes more sense.

estk commented 2 weeks ago

blocked on #367