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 support for log target #50

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Add support for log target #50

merged 1 commit into from
Dec 19, 2023

Conversation

Lancern
Copy link
Collaborator

@Lancern Lancern commented Nov 18, 2023

The log crate support specifying log target in the syntax below:

info!(target: "log-target", "log body");
//    ^~~~~~~~~~~~~~~~~~~~

The log target is a string that can be fully customized. Before this PR, this field is not used in spdlog-rs. Actually it can be used as a way to announce the logger name when you want to emit logs in a library through the log crate. Thus, in this PR, I used this information as another source of logger names.

Specifically, when constructing a spdlog::Record from a log::Record with a specific spdlog::Logger: (see also source)

  • If the spdlog::Logger has a logger name configured, the logger name of the constructed spdlog::Record will be that name.
  • Otherwise, if the log::Record has a non-empty target, the logger name of the constructed spdlog::Record will be that value.
  • Otherwise, the logger name of the constructed spdlog::Record will be None.

API Breaking

This PR indeed has potential impacts. It's actually API breaking because the signature of spdlog::Record::logger_name is updated from:

impl<'a> Record<'a> {
    fn logger_name(&self) -> Option<&'a str>;
}

to:

impl<'a> Record<'a> {
    fn logger_name(&self) -> Option<&str>;
}

. This is because we have to change the way we store the logger name in spdlog::Record. Before this PR, the logger name is simply represented by a &'a str. In this PR, I need to update its type to Cow<&'a, str> because I have to store an owned version of the log target name if the record is constructed from a log crate record.

But I believe the impact of this change should be minimal.

@Lancern Lancern added enhancement New feature or request log-crate-proxy labels Nov 18, 2023
@Lancern Lancern requested a review from SpriteOvO November 18, 2023 16:00
Copy link
Owner

@SpriteOvO SpriteOvO left a comment

Choose a reason for hiding this comment

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

I'm a bit worried that mixing concepts logger-name and target (log crate) will cause some problems in the future, but considering that it's a very small change (no public API change except that lifetime), and can be reverted at the any time if it does cause problems.

Therefore, LGTM, will merge it when we start preparing v0.4.0. Thank you.

@Lancern Lancern added this to the v0.4.0 milestone Nov 21, 2023
@SpriteOvO SpriteOvO merged commit 57ae0a4 into SpriteOvO:main Dec 19, 2023
32 checks passed
@Lancern Lancern deleted the log-target branch December 19, 2023 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request log-crate-proxy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants