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.

Sign up for GitHub

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

feat(lib): Move from log to tracing in a backwards-compatible way #2204

Merged
merged 7 commits into from
Jul 7, 2020
Merged

feat(lib): Move from log to tracing in a backwards-compatible way #2204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c402eb7
feat(lib): Move from `log` to `tracing` in a backwards-compatible way
May 16, 2020
01a8cff
how? do? cargo? features? work?
May 16, 2020
dd99479
Add `log` feature to `tracing` to not break existing users
davidbarsky Jul 7, 2020
0c1b134
i don't really understand how cargo features work
davidbarsky Jul 7, 2020
f64b768
address review comments
davidbarsky Jul 7, 2020
aedf5bb
address eliza's comment
davidbarsky Jul 7, 2020
ae1b445
phew, maybe one more time?
davidbarsky Jul 7, 2020
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
4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ http-body = "0.3.1"
httparse = "1.0"
h2 = "0.2.2"
itoa = "0.4.1"
log = "0.4"
tracing = { version = "0.1", default-features = false, features = ["log", "std"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to expose the log feature optionally (though it should default to on so we don't break existing code that expects log records emitted by Hyper), so that tracing users who don't need log can disable it. Otherwise, depending on Hyper will always enable the log feature, even when users don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, solid point. will fix.

Copy link
Member

Choose a reason for hiding this comment

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

Something to consider, though, is that if the logging enabled becomes an optional feature, then those who have default-features = false will have the logs disabled when they upgrade.

(I find cargo's default-features to be annoying in this regard, but what we gonna do.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Darn, that's an annoying thing. Are you okay with breaking those people in a minor release? Or more generally, how should this be handled?

pin-project = "0.4.20"
time = "0.1"
tower-service = "0.3"
Expand Down Expand Up @@ -58,7 +58,7 @@ url = "1.0"
[features]
default = [
"runtime",
"stream",
"stream"
]
runtime = [
"tcp",
Expand Down
12 changes: 5 additions & 7 deletions src/client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,13 +793,11 @@ fn absolute_form(uri: &mut Uri) {
}

fn authority_form(uri: &mut Uri) {
if log_enabled!(::log::Level::Warn) {
if let Some(path) = uri.path_and_query() {
// `https://hyper.rs` would parse with `/` path, don't
// annoy people about that...
if path != "/" {
warn!("HTTP/1.1 CONNECT request stripping path: {:?}", path);
}
if let Some(path) = uri.path_and_query() {
// `https://hyper.rs` would parse with `/` path, don't
// annoy people about that...
if path != "/" {
warn!("HTTP/1.1 CONNECT request stripping path: {:?}", path);
}
}
*uri = match uri.authority() {
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
#[doc(hidden)]
pub use http;
#[macro_use]
extern crate log;
extern crate tracing;

#[cfg(all(test, feature = "nightly"))]
extern crate test;
Expand Down