Skip to content
Closed
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
59 changes: 57 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ httpdate = "1.0"
httparse = "1.5.1"
h2 = { version = "0.3.3", optional = true }
itoa = "0.4.1"
tracing = { version = "0.1", default-features = false, features = ["std"] }
pin-project-lite = "0.2.4"
tower-service = "0.3"
tokio = { version = "1", features = ["sync"] }
Expand All @@ -42,7 +41,18 @@ want = "0.3"
# Optional

libc = { version = "0.2", optional = true }
log = { version = "0.4.14", 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.

why is there now a dependency on the log crate?

Copy link
Author

@taqtiqa-mark taqtiqa-mark Nov 19, 2021

Choose a reason for hiding this comment

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

why is there now a dependency on the log crate?

Is required to turn off tracing in an app that uses hyper (the example).
also from memory I believe the dependency was already there but just not surfaced/explicit. May be wrong in that recollection.

opentelemetry = { version = "0.16.0", features = ["rt-tokio", "trace"], optional = true }
opentelemetry-jaeger = { version = "0.15.0", features = ["reqwest_collector_client","rt-tokio"], optional = true }
serde_json = { version = "1.0", optional = true }
socket2 = { version = "0.4", optional = true }
reqwest = {version = "0.11", optional = true }
tracing = { version = "0.1", default-features = false, features = ["std", "log"], optional = true }
tracing-core = { version = "0.1", default-features = false, optional = true }
tracing-log = { version = "0.1", default-features = false, optional = true }
tracing-opentelemetry = { version = "0.16", optional = true }
tracing-subscriber = { version = "0.3", features = ["std", "env-filter"], optional = true }
tracing-tree = { version = "0.1", default-features = false, optional = true }

[dev-dependencies]
futures-util = { version = "0.3", default-features = false, features = ["alloc"] }
Expand Down Expand Up @@ -80,9 +90,10 @@ full = [
"client",
"http1",
"http2",
"layers",
"runtime",
"server",
"stream",
"runtime",
]

# HTTP versions
Expand All @@ -108,6 +119,21 @@ tcp = [
"tokio/time",
]

# Tracing layers
layers = [
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the feature flag to enable tracing support called layers? i would expect it to be called tracing or something...

Copy link
Author

Choose a reason for hiding this comment

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

why is the feature flag to enable tracing support called layers? i would expect it to be called tracing or something...

I believe that is where I started - again from memory - you can't have a feature name that is congruent to a crate name - but I'm not a Rust expert

Copy link
Contributor

Choose a reason for hiding this comment

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

you can't have a feature name that is congruent to a crate name - but I'm not a Rust expert

An optional dependency is equivalent to a feature flag that only enables one dependency. I think that tracing should just be an optional dependency and the instrumentation points should all be flagged with #[cfg(feature = "tracing")]. If we do end up exposing other functionality, like the OpenTelemetry layer you've implemented, it should be under a separate feature flag.

Copy link
Author

Choose a reason for hiding this comment

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

the instrumentation points should all be flagged with #[cfg(feature = "tracing")]

What do you think of using two flags debug and trace or some such. Where the use cases are distinct, Use debug when if you are investigating the integration of your app and hyper.
Use trace when you are investigating hyper internals.
The debug spans would then be constrained to around only public functions.

Not sure if that type of distinction can work out?

If we do end up exposing other functionality, like the OpenTelemetry layer you've implemented, it should be under a separate feature flag.

Agreed, or probably better to have a separate crate

"log",
"opentelemetry",
"opentelemetry-jaeger",
"serde_json",
"reqwest",
"tracing",
"tracing-core",
"tracing-log",
"tracing-opentelemetry",
"tracing-subscriber",
"tracing-tree",
Comment on lines +124 to +134
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think that the feature that enables tracing instrumentation in Hyper should enable all of these dependencies. suppose I want to log tracing spans from hyper using tracing-subscriber, in the plaintext format. in that case, i would want to enable tracing instrumentation in hyper...but i would not want hyper to suddenly depend on log, serde_json, reqwest, tracing_opentelemetry, opentelemetry, opentelemetry-jaeger, and tracing-tree.

Copy link
Author

Choose a reason for hiding this comment

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

i don't think that the feature that enables tracing instrumentation in Hyper should enable all of these dependencies. suppose I want to log tracing spans from hyper using tracing-subscriber, in the plaintext format. in that case, i would want to enable tracing instrumentation in hyper...but i would not want hyper to suddenly depend on log, serde_json, reqwest, tracing_opentelemetry, opentelemetry, opentelemetry-jaeger, and tracing-tree.

Summarizing - response to the question of introducing a feature: No objection, in fact please consider features that allow more fine grained control. Accurate summary?

]

# C-API support (currently unstable (no semver))
ffi = ["libc"]

Expand Down Expand Up @@ -140,11 +166,40 @@ name = "client_json"
path = "examples/client_json.rs"
required-features = ["full"]

[[example]]
name = "client_json_tracing_json"
path = "examples/client_json_tracing_json.rs"
required-features = ["full",
"tracing/max_level_trace",
]

[[example]]
name = "client_json_tracing_off"
path = "examples/client_json_tracing_off.rs"
required-features = ["full",
"tracing/max_level_off",
"log/max_level_off",
]

[[example]]
name = "client_json_tracing_otel"
path = "examples/client_json_tracing_otel.rs"
required-features = ["full",
"tracing/max_level_trace",
]

Comment on lines +169 to +190
Copy link
Contributor

Choose a reason for hiding this comment

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

take it or leave it: i think it might be worth putting all the tracing examples in a examples/tracing directory, so it's clear they're all related to various tracing-related configurations?

Copy link
Author

Choose a reason for hiding this comment

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

take it or leave it: i think it might be worth putting all the tracing examples in a examples/tracing directory, so it's clear they're all related to various tracing-related configurations?

OT but I'll take it into account closer to the time a final PR is ready.

[[example]]
name = "echo"
path = "examples/echo.rs"
required-features = ["full"]

[[example]]
name = "echo_tracing_otel"
path = "examples/echo_tracing_otel.rs"
required-features = ["full",
"tracing/max_level_trace",
]

[[example]]
name = "gateway"
path = "examples/gateway.rs"
Expand Down
64 changes: 64 additions & 0 deletions examples/client_json_tracing_json.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#![deny(warnings)]
#![warn(rust_2018_idioms)]

// Statically compile tracing events and spans - "compile out" tracing code.
//
// Usage:
//
// $ cargo run --features="full tracing/max_level_info" --example client_json_tracing
// ...
// Running `target/debug/examples/client_json_tracing`

// etc.

use hyper::body::Buf;
use hyper::Client;
use hyper::JsonLayer;
use serde::Deserialize;
use tracing::info;
use tracing_subscriber::prelude::*;

// A simple type alias so as to DRY.
type Result<T> = std::result::Result<T, Box<dyn std::error::Error + Send + Sync>>;


#[tokio::main]
async fn main() -> Result<()> {
// Set up `tracing-subscriber` to process tracing data.
tracing_subscriber::registry().with(JsonLayer).init();

// Log a `tracing` "event".
info!(status = true, answer = 42, message = "first event");

let url = "http://jsonplaceholder.typicode.com/users".parse().unwrap();
let users = fetch_json(url).await?;
// print users
println!("users: {:#?}", users);

// print the sum of ids
let sum = users.iter().fold(0, |acc, user| acc + user.id);
println!("sum of ids: {}", sum);
Ok(())
}

async fn fetch_json(url: hyper::Uri) -> Result<Vec<User>> {
let client = Client::new();

// Fetch the url...
let res = client.get(url).await?;

// asynchronously aggregate the chunks of the body
let body = hyper::body::aggregate(res).await?;

// try to parse as json with serde_json
let users = serde_json::from_reader(body.reader())?;

Ok(users)
}

#[derive(Deserialize, Debug)]
struct User {
id: i32,
#[allow(unused)]
name: String,
}
76 changes: 76 additions & 0 deletions examples/client_json_tracing_off.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#![deny(warnings)]
#![warn(rust_2018_idioms)]

// Statically compile out all tracing and logging events and spans
// - that is, "compile out" all tracing and logging code.
//
// Usage:
//
// $ cargo run --features="full tracing/max_level_off log/max_level_off" --example client_json_tracing_off
// ...
// Running `target/debug/examples/client_json_tracing_off`
// users: [
// User {
// id: 1,
// name: "Leanne Graham",
// },
// User {
// id: 2,
// name: "Ervin Howell",
// etc.

use hyper::body::Buf;
use hyper::Client;
use hyper::PrintLayer;
// use hyper::JsonLayer;
use serde::Deserialize;
use tracing::info;
use tracing_subscriber::prelude::*;

// A simple type alias so as to DRY.
type Result<T> = std::result::Result<T, Box<dyn std::error::Error + Send + Sync>>;


#[tokio::main]
async fn main() -> Result<()> {
// Set up `tracing-subscriber` to process tracing data.
// Note:
// We silence tracing via compile/build time features - see `Cargo.toml`.
// Hence, no change is required from the `client_json_tracing` example.
tracing_subscriber::registry().with(PrintLayer).init();

// "Log" a `tracing` "event".
info!(status = true, answer = 42, message = "first event");

let url = "http://jsonplaceholder.typicode.com/users".parse().unwrap();
let users = fetch_json(url).await?;
// print users
println!("users: {:#?}", users);

// print the sum of ids
let sum = users.iter().fold(0, |acc, user| acc + user.id);
println!("sum of ids: {}", sum);
Ok(())
}

async fn fetch_json(url: hyper::Uri) -> Result<Vec<User>> {
let client = Client::new();

// Fetch the url...
let res = client.get(url).await?;

// asynchronously aggregate the chunks of the body
let body = hyper::body::aggregate(res).await?;

// try to parse as json with serde_json
let users = serde_json::from_reader(body.reader())?;

Ok(users)
}

#[derive(Deserialize, Debug)]
struct User {
id: i32,
#[allow(unused)]
name: String,
}
107 changes: 107 additions & 0 deletions examples/client_json_tracing_otel.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
#![deny(warnings)]
#![warn(rust_2018_idioms)]

// Statically compile tracing events and spans - "compile out" tracing code.
//
// Usage:
//
// $ cargo run --features="full tracing/max_level_info" --example client_otel_tracing
// ...
// Running `target/debug/examples/client_otel_tracing_off`
// Hyper tracing event:
// level=Level(Info)
// target="client_otel_tracing"
// name="event examples/client_otel_tracing.rs:24"
// field=status
// field=answer
// field=message
// etc.

use hyper::body::Buf;
use hyper::Client;
//use hyper::OtelLayer;
use serde::Deserialize;
use tracing::info;
use tracing_subscriber::layer::SubscriberExt;
use tracing_subscriber::prelude::*;
//use tracing_subscriber::Layer;
//use tracing_subscriber::Registry;

// A simple type alias so as to DRY.
type Result<T> = std::result::Result<T, Box<dyn std::error::Error + Send + Sync>>;

#[tokio::main]
async fn main() -> Result<()> {
// First create propagators
let baggage_propagator = opentelemetry::sdk::propagation::BaggagePropagator::new();
let trace_context_propagator = opentelemetry::sdk::propagation::TraceContextPropagator::new();
let jaeger_propagator = opentelemetry_jaeger::Propagator::new();
opentelemetry::global::set_text_map_propagator(
opentelemetry::sdk::propagation::TraceContextPropagator::new(),
);

// Second compose propagators
let _composite_propagator = opentelemetry::sdk::propagation::TextMapCompositePropagator::new(vec![
Box::new(baggage_propagator),
Box::new(trace_context_propagator),
Box::new(jaeger_propagator),
]);
// Third create Jaeger pipeline
let tracer = opentelemetry_jaeger::new_pipeline()
.with_service_name("client_json2")
.install_batch(opentelemetry::runtime::Tokio)
.unwrap();
// Initialize `tracing` using `opentelemetry-tracing` and configure stdout logging
tracing_subscriber::Registry::default()
.with(tracing_subscriber::EnvFilter::new("TRACE"))
.with(hyper::OtelLayer::new(tracer))
.with(tracing_subscriber::fmt::layer())
//.with(tracing_tree::HierarchicalLayer::new(2))
.init();

// Trace executed (async) code
// tracing::subscriber::with_default(subscriber, || async {
// Create a span and enter it, returning a guard....
let root_span = tracing::span!(tracing::Level::INFO, "root_span_echo").entered();
root_span.in_scope(|| async {
// Log a `tracing` "event".
info!(status = true, answer = 42, message = "first event");

let url = "http://jsonplaceholder.typicode.com/users".parse().unwrap();
let users = fetch_json(url).await.expect("Vector of user data");
// print users
println!("users: {:#?}", users);

// print the sum of ids
let sum = users.iter().fold(0, |acc, user| acc + user.id);
println!("sum of ids: {}", sum);
Comment on lines +73 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

were these println!s intended to be tracing events? otherwise, i'm not sure if i understand the motivation behind including them in the example...


// ...but, it can also be exited explicitly, returning the `Span`
// struct:
//let _root_span = root_span.exit();
}).await;
Comment on lines +65 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

this is incorrect. Span::in_scope here is not instrumenting the async block, but the closure that returns it...which, in this case, doesn't contain any tracing events, as it immediately returns the async block. So, calling in_scope here is not doing anything. instead, these events occur within root_span's scope because of the Span::entered call prior to the closure...but entered should not be used in asynchronous code. see the documentation for details on why.

instead, this should be:

Suggested change
let root_span = tracing::span!(tracing::Level::INFO, "root_span_echo").entered();
root_span.in_scope(|| async {
// Log a `tracing` "event".
info!(status = true, answer = 42, message = "first event");
let url = "http://jsonplaceholder.typicode.com/users".parse().unwrap();
let users = fetch_json(url).await.expect("Vector of user data");
// print users
println!("users: {:#?}", users);
// print the sum of ids
let sum = users.iter().fold(0, |acc, user| acc + user.id);
println!("sum of ids: {}", sum);
// ...but, it can also be exited explicitly, returning the `Span`
// struct:
//let _root_span = root_span.exit();
}).await;
use tracing::instrument::Instrument;
let root_span = tracing::span!(tracing::Level::INFO, "root_span_echo);
async {
// Log a `tracing` "event".
info!(status = true, answer = 42, message = "first event");
let url = "http://jsonplaceholder.typicode.com/users".parse().unwrap();
let users = fetch_json(url).await.expect("Vector of user data");
// print users
println!("users: {:#?}", users);
// print the sum of ids
let sum = users.iter().fold(0, |acc, user| acc + user.id);
println!("sum of ids: {}", sum);
}
.instrument(root_span)
.await;

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the correction. I think this makes the point that examples are required - ideally someone should be able to get tracing without having to become an expert in the tracing stack - 80/20 rule - some standard boiler plate setup should suffice for 80%+ of use cases.

opentelemetry::global::shutdown_tracer_provider();
Ok(())
}

async fn fetch_json(url: hyper::Uri) -> Result<Vec<User>> {
let client = Client::new();

// Fetch the url...
let res = client.get(url).await?;

// asynchronously aggregate the chunks of the body
let body = hyper::body::aggregate(res).await?;

// try to parse as json with serde_json
let users = serde_json::from_reader(body.reader())?;

Ok(users)
}

#[derive(Deserialize, Debug)]
struct User {
id: i32,
#[allow(unused)]
name: String,
}
Loading