-
Notifications
You must be signed in to change notification settings - Fork 26
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 feature flag to minimize RecordLocation unknowns #19
base: master
Are you sure you want to change the base?
Changes from all commits
8934f31
3335ffd
95ad3ea
d6305fe
3f278ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
[package] | ||
name = "slog-stdlog" | ||
version = "4.1.0" | ||
version = "4.2.0" | ||
authors = ["Dawid Ciężarkiewicz <[email protected]>"] | ||
description = "`log` crate adapter for slog-rs" | ||
keywords = ["slog", "logging", "json", "log"] | ||
|
@@ -17,11 +17,13 @@ path = "lib.rs" | |
[features] | ||
default = [] | ||
kv_unstable = ["log/kv_unstable_std", "slog/dynamic-keys"] | ||
record_location_unstable = ["cached"] | ||
|
||
[dependencies] | ||
slog = "2.4" | ||
slog-scope = "4" | ||
log = { version = "0.4.11", features = ["std"] } | ||
cached = { version = "0.23.0", optional = true } | ||
|
||
[dev-dependencies] | ||
slog-term = "2" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,8 +53,8 @@ extern crate log; | |
#[cfg(feature = "kv_unstable")] | ||
mod kv; | ||
|
||
use slog::{b, Level, KV}; | ||
use std::{fmt, io}; | ||
use slog::{Level, KV, b}; | ||
|
||
struct Logger; | ||
|
||
|
@@ -68,9 +68,33 @@ fn log_to_slog_level(level: log::Level) -> Level { | |
} | ||
} | ||
|
||
/// Gets a static string for Record location information. | ||
/// Uses a cache to avoid leaking each string more than once. | ||
#[cfg(feature = "record_location_unstable")] | ||
#[cached::proc_macro::cached] | ||
fn get_static_info(non_static_info: Option<String>) -> &'static str { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The point of this function is to turn There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately it can't be. If I understand correctly, it's because it can't cache based on a non_static string as the cache key would be dropped. This is similar to issues I had with the interning libraries. |
||
match non_static_info { | ||
Some(s) => Box::leak(s.into_boxed_str()), | ||
None => "unknown", | ||
} | ||
} | ||
|
||
fn record_as_location(r: &log::Record) -> slog::RecordLocation { | ||
#[cfg(not(feature = "record_location_unstable"))] | ||
let module = r.module_path_static().unwrap_or("<unknown>"); | ||
#[cfg(not(feature = "record_location_unstable"))] | ||
let file = r.file_static().unwrap_or("<unknown>"); | ||
|
||
// Warning: expands Record module and file names for non-static strings by leaking strings | ||
#[cfg(feature = "record_location_unstable")] | ||
let module = r | ||
.module_path_static() | ||
.unwrap_or_else(|| get_static_info(r.module_path().map(|s| s.to_string()))); | ||
#[cfg(feature = "record_location_unstable")] | ||
let file = r | ||
.file_static() | ||
.unwrap_or_else(|| get_static_info(r.file().map(|s| s.to_string()))); | ||
|
||
let line = r.line().unwrap_or_default(); | ||
|
||
slog::RecordLocation { | ||
|
@@ -100,10 +124,10 @@ impl log::Log for Logger { | |
}; | ||
#[cfg(feature = "kv_unstable")] | ||
{ | ||
let key_values = r.key_values(); | ||
let mut visitor = kv::Visitor::new(); | ||
key_values.visit(&mut visitor).unwrap(); | ||
slog_scope::with_logger(|logger| logger.log(&slog::Record::new(&s, args, b!(visitor)))) | ||
let key_values = r.key_values(); | ||
let mut visitor = kv::Visitor::new(); | ||
key_values.visit(&mut visitor).unwrap(); | ||
slog_scope::with_logger(|logger| logger.log(&slog::Record::new(&s, args, b!(visitor)))) | ||
} | ||
#[cfg(not(feature = "kv_unstable"))] | ||
slog_scope::with_logger(|logger| logger.log(&slog::Record::new(&s, args, b!()))) | ||
|
@@ -252,7 +276,11 @@ impl slog::Drain for StdLog { | |
let lazy = LazyLogString::new(info, logger_values); | ||
// Please don't yell at me for this! :D | ||
// https://github.com/rust-lang-nursery/log/issues/95 | ||
log::__private_api_log(format_args!("{}", lazy), level, &(target, info.module(), info.file(), info.line())); | ||
log::__private_api_log( | ||
format_args!("{}", lazy), | ||
level, | ||
&(target, info.module(), info.file(), info.line()), | ||
); | ||
|
||
Ok(()) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW. I skimmed through
cached
and I don't understand why do you use it. Doesn't seem made for interning strings or anything. Can you explain what's the reasoning behind it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never done string interning in particular so I was looking for an ergonomic caching library which I thought would accomplish the same result.
I can do more digging on
string_cache
orstring_interner
crates if you'd prefer those, just let me know.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in depth on interning, but I think they are good reasons why there are specialized libraries for them).