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

Use std::panic::Location for file & line information #410

Closed
wants to merge 3 commits into from
Closed

Use std::panic::Location for file & line information #410

wants to merge 3 commits into from

Conversation

pwoolcoc
Copy link

@pwoolcoc pwoolcoc commented Aug 27, 2020

this commit adds a feature, track_caller, that allows the user to opt-in to using location information from std::panic::Location instead of the built-in file! and line! macros. This will allow log record location information to be manipulated in the same way that #[track_caller] allows panic location information to be manipulated

this commit adds a feature, `track_caller`, that allows the user to
opt-in to using location information from `std::panic::Location` instead
of the built-in `file!` and `line!` macros. This will allow log record
location information to be manipulated in the same way that
`#[track_caller]` allows panic location information to be manipulated
@Thomasdezeeuw
Copy link
Collaborator

Is there a benefit to doing this?

@Thomasdezeeuw
Copy link
Collaborator

Ah, sorry didn't see the second part of your comment, ignore my previous comment.

@pwoolcoc
Copy link
Author

It allows you to manipulate location information for log messages in the same way that you can for panic! messages. For example, one of my applications has a generic "retry-er" that takes a closure and a retry policy, and will run the closure until it gets a successful return value from the closure or until it hits it's max retries. Currently, any logging we do from the retryer (like "got error Foo, retry 1/10") will include location information from inside the retryer, but no identifying information about exactly which closure generated the error. Of course, we could simply pass more information to the retryer to help us identify the piece of code that is erroring out, but I thought it would be possible to use #[track_caller] for this instead, to keep the API cleaner. Turns out I couldn't use log::error!, log::warn!, etc, because those macros use file!() and line!(). I can do it if I write my own log::Record-generating code, but I thought having this functionality built-in to log might be useful for others.

src/macros.rs Outdated
Comment on lines 252 to 256
if cfg!(feature = "track_caller") {
::std::panic::Location::caller().file()
} else {
file!()
}
Copy link

Choose a reason for hiding this comment

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

This will break compilation of log with rustc < 1.46, even without the track_caller feature enabled. The following will allow compilation to succeed without that feature enabled:

        #[cfg(feature = "track_caller")]
        {
            ::std::panic::Location::caller().file()
        }
        #[cfg(not(feature = "track_caller"))]
        {
            file!()
        }

src/macros.rs Outdated
file!()
#[cfg(feature = "track_caller")]
{
::std::panic::Location::caller().file()
Copy link
Member

Choose a reason for hiding this comment

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

This cfg is evaluated in the context of the crate that invokes a log macro, not the context of the log crate so this won't work.

Copy link
Author

Choose a reason for hiding this comment

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

thanks @sfackler, I pushed an update so they're evaluated in the correct context

@pwoolcoc
Copy link
Author

pwoolcoc commented Sep 1, 2020

r? @sfackler @jdm

@jdm
Copy link

jdm commented Sep 1, 2020

My earlier concern has been addressed, but otherwise I'm just a bystander :)

@pwoolcoc
Copy link
Author

pwoolcoc commented Sep 1, 2020

no problem, thanks!

@pwoolcoc
Copy link
Author

bump, can someone take a look at this?

Copy link
Contributor

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Thanks @pwoolcoc! If MSRVs weren't a concern this is probably something we'd want unconditionally, wouldn't we? If that's the case I think we could use our build.rs to detect 1.46.0 support and just enable it quietly instead of using an explicit feature.

@@ -48,6 +48,9 @@ std = []
kv_unstable = []
kv_unstable_sval = ["kv_unstable", "sval/fmt"]

# requires 1.46.0
track_caller = []
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could detect 1.46.0 in our build.rs and quietly enable this.

@KodrAus
Copy link
Contributor

KodrAus commented Nov 15, 2021

Just coming in through some triage. I think the only outstanding thing here was using our build script to automatically use track_caller if we're on at least 1.46.0.

EFanZh pushed a commit to EFanZh/log that referenced this pull request Jul 23, 2023
Bumps [thiserror](https://github.com/dtolnay/thiserror) from 1.0.34 to 1.0.35.
- [Release notes](https://github.com/dtolnay/thiserror/releases)
- [Commits](dtolnay/thiserror@1.0.34...1.0.35)

---
updated-dependencies:
- dependency-name: thiserror
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@jrose-signal
Copy link

Since this PR was left off, the MSRV has been bumped to 1.60. I think that means this can/should be turned on unconditionally!

@Thomasdezeeuw
Copy link
Collaborator

@pwoolcoc is this something you're still interested in? We've updated our MSRV to 1.60 so this can be done now.

@pwoolcoc
Copy link
Author

@Thomasdezeeuw yes, but as I deleted my original fork, I can't seem to update this PR, so I'll have to open a new PR.

@pwoolcoc
Copy link
Author

Closing in favor of #599

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.

None yet

6 participants