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

Jump to bottom

set OTLP service name and namespace #210

Merged
merged 3 commits into from
Nov 30, 2021
Merged

set OTLP service name and namespace #210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06cf6af
set OTLP service name and namespace
Geal Nov 29, 2021
062e699
rollback tracing-opentelemetry and tracing-subscriber updates
Geal Nov 29, 2021
0b8757e
keep the same parent span for prepare, execute and format
Geal Nov 29, 2021
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
65 changes: 47 additions & 18 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion apollo-router-core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ mockall = "0.10.2"
static_assertions = "1"
test-log = { version = "0.2.8", default-features = false, features = ["trace"] }
tokio = { version = "1", features = ["full"] }
tracing-subscriber = { version = "0.3.2", default-features = false, features = [
# don't bump to 0.3 until we bump tracing-opentelemetry to 0.16
tracing-subscriber = { version = "0.2.25", default-features = false, features = [
Geal marked this conversation as resolved.
Show resolved Hide resolved
"env-filter",
"fmt",
] }
1 change: 0 additions & 1 deletion apollo-router-core/src/query_planner/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,6 @@ impl PlanNode {
Arc::clone(&service_registry),
Arc::clone(&schema),
)
.instrument(tracing::info_span!("execution"))
.await;

// TODO: this is not great but there is no other way
Expand Down
9 changes: 6 additions & 3 deletions apollo-router/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,11 @@ tokio = { version = "1.14.0", features = ["full"] }
tonic = { version = "0.5.2", optional = true }
tracing = "0.1.29"
tracing-futures = "0.2.5"
tracing-opentelemetry = "0.16.0"
tracing-subscriber = { version = "0.3.2", features = ["json"] }
# don't bump to 0.16 until reqwest-tracing does
tracing-opentelemetry = "0.15.0"
# don't bump to 0.3 until we bump tracing-opentelemetry to 0.16
tracing-subscriber = { version = "0.2.25", features = ["json"] }

typed-builder = "0.9.1"
url = { version = "2.2.2", features = ["serde"] }
warp = { version = "0.3.2", default-features = false, features = [
Expand All @@ -73,7 +76,7 @@ maplit = "1.0.2"
mockall = "0.10.2"
reqwest = { version = "0.11.6", features = ["json", "stream"] }
test-log = { version = "0.2.8", default-features = false, features = ["trace"] }
tracing-subscriber = { version = "0.3.2", default-features = false, features = [
tracing-subscriber = { version = "0.2.25", default-features = false, features = [
"env-filter",
"fmt",
] }
Expand Down
12 changes: 9 additions & 3 deletions apollo-router/src/apollo_router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use apollo_router_core::prelude::graphql::*;
use derivative::Derivative;
use futures::prelude::*;
use std::sync::Arc;
use tracing::{Instrument, Span};
use tracing_futures::WithSubscriber;

/// The default router of Apollo, suitable for most use cases.
Expand Down Expand Up @@ -88,6 +89,7 @@ pub struct ApolloPreparedQuery {
impl PreparedQuery for ApolloPreparedQuery {
#[tracing::instrument(level = "debug")]
async fn execute(self, request: Arc<Request>) -> ResponseStream {
let span = Span::current();
stream::once(
async move {
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if we attach the current span to this future? Would that solve the problem? There is a dedicated function for that https://docs.rs/tracing/0.1.29/tracing/trait.Instrument.html#method.in_current_span

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I tried that, but it makes the future non Send. I think I found another way without setting the parent explicitely, I'll try this afternoon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have thought of that earlier 😅 23020ae

Maybe we can simplify ResponseStream now

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! ^_^ some improvements coming then? I'm glad I looked at this PR even if it was a bit late

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's always room for improvement 😀

let response_task = self
Expand All @@ -98,13 +100,17 @@ impl PreparedQuery for ApolloPreparedQuery {
Arc::clone(&request),
Arc::clone(&self.service_registry),
Arc::clone(&self.schema),
);
let query_task = self.query_cache.get_query(&request.query);
)
.instrument(tracing::info_span!(parent: &span, "execution"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this instrumentation shouldn't stay in model.rs 🤔 validate_request does that already. Does using the decorator fix the issue? I like the decorator ^_^

    #[tracing::instrument(name = "validation", level = "debug")]
    pub fn validate_request(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attribute does not allow overriding the parent span: https://docs.rs/tracing/0.1.29/tracing/attr.instrument.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this comment was not about overriding parent span and stuff. More a question of:

  1. where should go the trace: at the source or at the caller
  2. is the decorator more esthetic, should we use it, can we

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that topic should go in #213

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine either way, as long as we make sure we don't serialize too much info with the attribute

let query_task = self
.query_cache
.get_query(&request.query)
.instrument(tracing::info_span!(parent: &span, "query_parsing"));

let (mut response, query) = tokio::join!(response_task, query_task);

if let Some(query) = query {
tracing::debug_span!("format_response").in_scope(|| {
tracing::debug_span!(parent: &span, "format_response").in_scope(|| {
query.format_response(&mut response, request.operation_name.as_deref())
});
}
Expand Down
10 changes: 7 additions & 3 deletions apollo-router/src/configuration/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,8 @@ pub enum OpenTelemetry {
#[derivative(Default)]
pub struct Jaeger {
pub collector_endpoint: Option<Url>,
#[serde(default = "default_jaeger_service_name")]
#[derivative(Default(value = "default_jaeger_service_name()"))]
#[serde(default = "default_service_name")]
#[derivative(Default(value = "default_service_name()"))]
pub service_name: String,
#[serde(skip, default = "default_jaeger_username")]
#[derivative(Default(value = "default_jaeger_username()"))]
Expand All @@ -219,10 +219,14 @@ pub struct Jaeger {
pub password: Option<String>,
}

fn default_jaeger_service_name() -> String {
fn default_service_name() -> String {
"router".to_string()
}

fn default_service_namespace() -> String {
"apollo".to_string()
}

fn default_jaeger_username() -> Option<String> {
std::env::var("JAEGER_USERNAME").ok()
}
Expand Down
22 changes: 15 additions & 7 deletions apollo-router/src/configuration/otlp/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ mod http;
pub use self::grpc::*;
#[cfg(feature = "otlp-http")]
pub use self::http::*;
use super::{default_service_name, default_service_namespace};
use crate::configuration::ConfigurationError;
use opentelemetry::sdk::resource::Resource;
use opentelemetry::sdk::trace::{Sampler, Tracer};
use opentelemetry::KeyValue;
use opentelemetry_otlp::{Protocol, WithExportConfig};
use serde::{Deserialize, Deserializer, Serialize};
use std::time::Duration;
Expand Down Expand Up @@ -79,9 +81,8 @@ impl Tracing {

pipeline = pipeline.with_exporter(self.exporter.exporter()?);

if let Some(config) = self.trace_config.as_ref() {
pipeline = pipeline.with_trace_config(config.trace_config());
}
pipeline = pipeline
.with_trace_config(self.trace_config.clone().unwrap_or_default().trace_config());

pipeline
.install_batch(opentelemetry::runtime::Tokio)
Expand Down Expand Up @@ -147,7 +148,7 @@ where
Ok(Some(url))
}

#[derive(Debug, Clone, Deserialize, Serialize)]
#[derive(Debug, Default, Clone, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct TraceConfig {
pub sampler: Option<Sampler>,
Expand Down Expand Up @@ -180,9 +181,16 @@ impl TraceConfig {
if let Some(n) = self.max_attributes_per_link {
trace_config = trace_config.with_max_attributes_per_link(n);
}
if let Some(resource) = self.resource.clone() {
trace_config = trace_config.with_resource(resource);
}

let resource = self.resource.clone().unwrap_or_else(|| {
Resource::new(vec![
KeyValue::new("service.name", default_service_name()),
KeyValue::new("service.namespace", default_service_namespace()),
])
});

trace_config = trace_config.with_resource(resource);

trace_config
}
}
4 changes: 2 additions & 2 deletions licenses.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4215,6 +4215,7 @@ <h4>Used by:</h4>
<li><a href=" https://github.com/yoshuawuyts/miow ">miow</a></li>
<li><a href=" https://github.com/havarnov/multimap ">multimap</a></li>
<li><a href=" https://github.com/deprecrated/net2-rs ">net2</a></li>
<li><a href=" https://github.com/rust-num/num-integer ">num-integer</a></li>
<li><a href=" https://github.com/rust-num/num-traits ">num-traits</a></li>
<li><a href=" https://github.com/seanmonstar/num_cpus ">num_cpus</a></li>
<li><a href=" https://github.com/matklad/once_cell ">once_cell</a></li>
Expand Down Expand Up @@ -6590,6 +6591,7 @@ <h3 id="Apache-2.0">Apache License 2.0</h3>
<h4>Used by:</h4>
<ul class="license-used-by">
<li><a href=" https://crates.io/crates/apollo-parser ">apollo-parser</a></li>
<li><a href=" https://github.com/chronotope/chrono ">chrono</a></li>
<li><a href=" https://github.com/tokio-rs/prost ">prost-build</a></li>
<li><a href=" https://github.com/tokio-rs/prost ">prost-derive</a></li>
<li><a href=" https://github.com/tokio-rs/prost ">prost-types</a></li>
Expand Down Expand Up @@ -7821,10 +7823,8 @@ <h4>Used by:</h4>
<li><a href=" https://github.com/tokio-rs/tracing ">tracing-futures</a></li>
<li><a href=" https://github.com/tokio-rs/tracing ">tracing-log</a></li>
<li><a href=" https://github.com/tokio-rs/tracing ">tracing-opentelemetry</a></li>
<li><a href=" https://github.com/tokio-rs/tracing ">tracing-opentelemetry</a></li>
<li><a href=" https://github.com/tokio-rs/tracing ">tracing-serde</a></li>
<li><a href=" https://github.com/tokio-rs/tracing ">tracing-subscriber</a></li>
<li><a href=" https://github.com/tokio-rs/tracing ">tracing-subscriber</a></li>
</ul>
<pre class="license-text">Copyright (c) 2019 Tokio Contributors

Expand Down