Skip to content

Conversation

@JoernMueller
Copy link

No description provided.

@JoernMueller
Copy link
Author

As I´m not that much accustomed to the expectations and requirements on FOSS PRs, please let me know if anything is missing or needs to be improved on this one to get it merged. Many thanks in advance.

@gabime
Copy link
Owner

gabime commented Apr 14, 2025

Thanks for the PR. However, since the MDC data is already included in the formatted output, adding it again would be redundant. I'd prefer to avoid the duplication, especially given the amount of added code.

@gabime gabime closed this Apr 14, 2025
@JoernMueller
Copy link
Author

@gabime Thanks a lot for your review and feedback.
I completely agree with the intention to avoid redundancy in the output. Nevertheless I would like to point out two things.

  1. The default parameter of the systemd_sinkfor the formatting is disabled, meaning all MDC data will be omittted.
  2. In spdlog we have a structured representation of the logging data and journald is desgined to handle such structured data. But if we now squash all this data into a custom formatted string this makes it much harder to do any evaluation and tooling on top of journald than it needs to be.

So my proposal would be to remove the formatting option from this sink entirely in favor of passing over structured data. At the same time it should be possible to consolidate the code a bit. (So far I had choosen to maintain the original state for the SPDLOG_NO_TLS builds as it seemed less intrusive to me.)

Please let me know if you consider this a viable approach. If so, I would be happy to do these changes and come up with another PR.

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