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 feature flag to minimize RecordLocation unknowns #19

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,19 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## 4.2.0 - 2021-7-12
### Changed

* Add feature flag `record_location_unstable` to expand record location support.
Be warned that this is accomplished by leaking the record location strings.

## 4.1.0 - 2020-10-21
### Changed

* Require `slog` 2.4 or greater
* Remove dependency `crossbeam`
* Require `log` 0.4.11 or greater

## 4.0.0 - 2018-08-13
### Changed

Expand Down
4 changes: 3 additions & 1 deletion Cargo.toml
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"]
Expand All @@ -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 }
Copy link
Contributor

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?

Copy link
Author

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 or string_interner crates if you'd prefer those, just let me know.

Copy link
Contributor

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).


[dev-dependencies]
slog-term = "2"
Expand Down
40 changes: 34 additions & 6 deletions lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be non_static_info: Option<&'str>?

Copy link
Contributor

Choose a reason for hiding this comment

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

The point of this function is to turn &'a str into &'static str

Copy link
Author

Choose a reason for hiding this comment

The 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.
So I have to clone it before it gets passed into this function. But it only does that clone in the case where it needs to (a static string doesn't already exist).

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 {
Expand Down Expand Up @@ -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!())))
Expand Down Expand Up @@ -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(())
}
Expand Down