Skip to content

Add support for Key-Value pairs#362

Merged
estk merged 4 commits intoestk:mainfrom
ellttBen:add-kv-support
May 26, 2025
Merged

Add support for Key-Value pairs#362
estk merged 4 commits intoestk:mainfrom
ellttBen:add-kv-support

Conversation

@ellttBen
Copy link
Copy Markdown
Contributor

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:

  • Everything is implemented behind a log_kv feature
  • Support is added in the two encoders json and pattern:
    • The json message has an attributes field which is a serialized map of the record's structured logs
    • The pattern encoder has a new K or key_value formatter which functions exactly like the mdc formatter.

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

@ellttBen ellttBen requested review from estk and gadunga as code owners March 20, 2024 07:31
Comment thread Cargo.toml
Copy link
Copy Markdown
Contributor

@gauntl3t12 gauntl3t12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Lets try and get some more tests in there and the documentation

Comment thread src/encode/json.rs Outdated
Comment thread src/encode/json.rs Outdated
Comment thread src/encode/json.rs
Comment thread src/encode/pattern/mod.rs Outdated
Comment thread src/encode/pattern/mod.rs Outdated
@ellttBen ellttBen requested a review from gauntl3t12 March 21, 2024 09:07
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 21, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 88.63636% with 5 lines in your changes missing coverage. Please review.

Project coverage is 62.35%. Comparing base (8ab1b34) to head (8081d03).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/encode/pattern/mod.rs 84.84% 5 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
- Coverage   63.39%   62.35%   -1.05%     
==========================================
  Files          24       25       +1     
  Lines        1557     1591      +34     
==========================================
+ Hits          987      992       +5     
- Misses        570      599      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/encode/json.rs Outdated
Comment thread src/encode/pattern/mod.rs Outdated
@ellttBen ellttBen force-pushed the add-kv-support branch 2 times, most recently from 207d3be to 230f2ea Compare May 7, 2024 15:18
@ellttBen
Copy link
Copy Markdown
Contributor Author

ellttBen commented May 7, 2024

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.

@gauntl3t12
Copy link
Copy Markdown
Contributor

gauntl3t12 commented May 12, 2024

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

Comment thread Cargo.toml
@ellttBen ellttBen requested a review from gauntl3t12 May 24, 2024 15:12
gauntl3t12
gauntl3t12 previously approved these changes May 30, 2024
@sdroege
Copy link
Copy Markdown

sdroege commented Jul 3, 2024

What's the status of this PR?

@gauntl3t12
Copy link
Copy Markdown
Contributor

@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
Copy link
Copy Markdown

sdroege commented Jul 3, 2024

That sounds great, thanks!

Copy link
Copy Markdown
Owner

@estk estk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments, but looking really good.

One more general question, did you attempt to wrap log::kv::Source in a newtype and impl serde::Serialize on it?

Just wondering because I was thinking about the tradeoffs of allocating a whole BTree vs the annoyance of adding lifetimes to Message.

Comment thread src/encode/json.rs Outdated
Comment thread src/encode/json.rs
Comment thread src/encode/pattern/mod.rs
Comment thread src/encode/pattern/mod.rs Outdated
Comment thread src/encode/pattern/mod.rs Outdated
Comment thread src/encode/pattern/mod.rs Outdated
…encoding. Factor pattern parsing code between MDC and key_value targets
@ellttBen
Copy link
Copy Markdown
Contributor Author

ellttBen commented Jul 9, 2024

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.

@ellttBen ellttBen requested a review from estk July 9, 2024 16:49
estk
estk previously approved these changes Jul 9, 2024
Copy link
Copy Markdown
Owner

@estk estk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nailed it

@estk
Copy link
Copy Markdown
Owner

estk commented Jul 9, 2024

blocked on #367

@SL-RU
Copy link
Copy Markdown

SL-RU commented Aug 10, 2024

Hi! What is the current status of the MR?

@gauntl3t12
Copy link
Copy Markdown
Contributor

Hi! What is the current status of the MR?

@SL-RU we're still waiting on the blocked PR

Comment thread Cargo.toml
@SL-RU
Copy link
Copy Markdown

SL-RU commented Oct 10, 2024

Hi! What is the current status of the MR?

@gauntl3t12
Copy link
Copy Markdown
Contributor

Hey! Sorry but all PRs are blocked by the PR fixing the deprecated API. I've been too busy of late to bug @estk and get it moving again. Please feel free to take a look at that PR and suggest options you might see in helping bring it to closure.

@SL-RU
Copy link
Copy Markdown

SL-RU commented Feb 28, 2025

Hi, what is current status? What blocks this PR now?

@SL-RU
Copy link
Copy Markdown

SL-RU commented Mar 19, 2025

It seems repo was abandoned. Isn't it?
Almost half of year without progress?

@gauntl3t12
Copy link
Copy Markdown
Contributor

@SL-RU yes, as far as I'm concerned this project is abandoned. The owner, was not happy with the solution to resolve #367 which blocks all further PRs. Until such a time as he turns the project over to be community driven, I don't foresee a solution to grow the project.

@SL-RU
Copy link
Copy Markdown

SL-RU commented Mar 19, 2025

@SL-RU yes, as far as I'm concerned this project is abandoned. The owner, was not happy with the solution to resolve #367 which blocks all further PRs. Until such a time as he turns the project over to be community driven, I don't foresee a solution to grow the project.

Thank you for the reply, do you know some project with same functionality?

@gauntl3t12
Copy link
Copy Markdown
Contributor

Unfortunately no I do not. I know the author moved to trace4rs with his employer. However, that project also appears to have died.

Repository owner deleted a comment from gauntl3t12 May 24, 2025
@estk
Copy link
Copy Markdown
Owner

estk commented May 24, 2025

Bryan had agreed to send me an email when he saw PRs as ready. My last email from him was Feb last year.

i am adding this review to my queue.

@estk
Copy link
Copy Markdown
Owner

estk commented May 25, 2025

@ellttBen can you please rebase?

@estk estk enabled auto-merge (rebase) May 26, 2025 05:56
@estk estk disabled auto-merge May 26, 2025 05:57
@estk estk merged commit 945a2bb into estk:main May 26, 2025
12 checks passed
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.

Support for Key-Value Pairs

6 participants