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

test: e2e testing for OpenTelemetry Tracing #17458

Merged
merged 4 commits into from
Feb 1, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions Cargo.lock

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

2 changes: 2 additions & 0 deletions src/environmentd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ rand = "0.8.5"
reqwest = { version = "0.11.13", features = ["json"] }
rlimit = "0.8.3"
sentry = { version = "0.29.1", optional = true }
sentry-tracing = "0.29.1"
serde = { version = "1.0.152", features = ["derive"] }
serde_json = "1.0.89"
shell-words = "1.1.0"
Expand All @@ -79,6 +80,7 @@ tokio-postgres = { git = "https://github.com/MaterializeInc/rust-postgres" }
tokio-stream = { version = "0.1.11", features = ["net"] }
tower-http = { version = "0.3.5", features = ["cors"] }
tracing = "0.1.37"
tracing-core = "0.1.30"
tracing-opentelemetry = { git = "https://github.com/MaterializeInc/tracing.git", branch = "v0.1.x" }
tracing-subscriber = "0.3.16"
tungstenite = { version = "0.18.0", features = ["native-tls"] }
Expand Down
34 changes: 34 additions & 0 deletions src/environmentd/tests/sql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2274,3 +2274,37 @@ fn test_isolation_level_notice() {
})
.unwrap();
}

#[test]
fn test_emit_tracing_notice() {
let config = util::Config::default().with_enable_tracing(true);
let server = util::start_server(config).unwrap();

let (tx, mut rx) = futures::channel::mpsc::unbounded();

let mut client = server
.pg_config()
.notice_callback(move |notice| {
tx.unbounded_send(notice).unwrap();
})
.connect(postgres::NoTls)
.unwrap();

client
.execute("SET emit_trace_id_notice = true", &[])
.unwrap();
let _row = client.query_one("SELECT 1;", &[]).unwrap();

let tracing_re = Regex::new("trace id: (.*)").unwrap();
match rx.try_next() {
Ok(Some(msg)) => {
// assert the NOTICE we recieved contained a trace_id
let captures = tracing_re.captures(msg.message()).expect("no matches?");
let trace_id = captures.get(1).expect("trace_id not captured?").as_str();

assert!(trace_id.is_ascii());
assert_eq!(trace_id.len(), 32);
}
x => panic!("failed to read message from channel, {:?}", x),
}
}
41 changes: 39 additions & 2 deletions src/environmentd/tests/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,15 @@ use mz_ore::metrics::MetricsRegistry;
use mz_ore::now::{EpochMillis, NowFn, SYSTEM_TIME};
use mz_ore::retry::Retry;
use mz_ore::task;
use mz_ore::tracing::TracingHandle;
use mz_ore::tracing::{TracingHandle, TracingConfig, StderrLogConfig, StderrLogFormat, OpenTelemetryConfig, TracingGuard};
use mz_persist_client::cache::PersistClientCache;
use mz_persist_client::cfg::PersistConfig;
use mz_persist_client::PersistLocation;
use mz_secrets::SecretsController;
use mz_sql::catalog::EnvironmentId;
use mz_stash::StashFactory;
use mz_storage_client::types::connections::ConnectionContext;
use tracing_subscriber::filter::Targets;

pub static KAFKA_ADDRS: Lazy<String> =
Lazy::new(|| env::var("KAFKA_ADDRS").unwrap_or_else(|_| "localhost:9092".into()));
Expand All @@ -128,6 +129,7 @@ pub struct Config {
default_cluster_replica_size: String,
builtin_cluster_replica_size: String,
propagate_crashes: bool,
enable_tracing: bool,
}

impl Default for Config {
Expand All @@ -144,6 +146,7 @@ impl Default for Config {
default_cluster_replica_size: "1".to_string(),
builtin_cluster_replica_size: "1".to_string(),
propagate_crashes: false,
enable_tracing: false,
}
}
}
Expand Down Expand Up @@ -216,6 +219,11 @@ impl Config {
self.propagate_crashes = propagate_crashes;
self
}

pub fn with_enable_tracing(mut self, enable_tracing: bool) -> Self {
self.enable_tracing = enable_tracing;
self
}
}

pub fn start_server(config: Config) -> Result<Server, anyhow::Error> {
Expand Down Expand Up @@ -277,6 +285,33 @@ pub fn start_server(config: Config) -> Result<Server, anyhow::Error> {
let postgres_factory = StashFactory::new(&metrics_registry);
let secrets_controller = Arc::clone(&orchestrator);
let connection_context = ConnectionContext::for_tests(orchestrator.reader());
let (tracing_handle, tracing_guard) = if config.enable_tracing {
let config = TracingConfig::<fn(&tracing::Metadata) -> sentry_tracing::EventFilter> {
service_name: "environmentd",
stderr_log: StderrLogConfig {
format: StderrLogFormat::Json,
filter: Targets::default(),
},
opentelemetry: Some(OpenTelemetryConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be None?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you referring to the opentelemetry field? That cannot be None because otherwise we wouldn't setup the opentelemetry tracing layer which is necessary for the test

endpoint: "http://fake_address_for_testing:8080".to_string(),
headers: http::HeaderMap::new(),
filter: Targets::default().with_default(tracing_core::Level::DEBUG),
resource: opentelemetry::sdk::resource::Resource::default(),
start_enabled: true,
}),
#[cfg(feature = "tokio-console")]
tokio_console: None,
sentry: None,
build_version: &mz_environmentd::BUILD_INFO.version,
build_sha: &mz_environmentd::BUILD_INFO.sha,
build_time: &mz_environmentd::BUILD_INFO.time,
};
let (tracing_handle, tracing_guard) = runtime.block_on(mz_ore::tracing::configure(config))?;
(tracing_handle, Some(tracing_guard))
} else {
(TracingHandle::disabled(), None)
};

let inner = runtime.block_on(mz_environmentd::serve(mz_environmentd::Config {
adapter_stash_url,
controller: ControllerConfig {
Expand Down Expand Up @@ -315,7 +350,7 @@ pub fn start_server(config: Config) -> Result<Server, anyhow::Error> {
bootstrap_system_parameters: Default::default(),
availability_zones: Default::default(),
connection_context,
tracing_handle: TracingHandle::disabled(),
tracing_handle,
storage_usage_collection_interval: config.storage_usage_collection_interval,
segment_api_key: None,
egress_ips: vec![],
Expand All @@ -330,6 +365,7 @@ pub fn start_server(config: Config) -> Result<Server, anyhow::Error> {
runtime,
metrics_registry,
_temp_dir: temp_dir,
_tracing_guard: tracing_guard,
};
Ok(server)
}
Expand All @@ -339,6 +375,7 @@ pub struct Server {
pub runtime: Arc<Runtime>,
_temp_dir: Option<TempDir>,
pub metrics_registry: MetricsRegistry,
pub _tracing_guard: Option<TracingGuard>,
Copy link
Member

Choose a reason for hiding this comment

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

Tiniest little nit, for future reference: the leading underscore is meant to silence the unused field checker! If this is pub it doesn't need the leading underscore, because Rust is smart enough to realize that some downstream crate might use it. (But probably this shouldn't be pub?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch! Definitely doesn't need to be pub, let me fix that.

It also feels like this should be a Clippy lint, so I put up rust-lang/rust-clippy#10283, we'll see what the team thinks :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I know you're interested :) that Clippy lint finally got merged!

}

impl Server {
Expand Down
1 change: 1 addition & 0 deletions src/ore/src/tracing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ impl std::fmt::Debug for TracingHandle {
/// A guard for the tracing infrastructure configured with [`configure`].
///
/// This guard should be kept alive for the lifetime of the program.
#[must_use = "Must hold for the lifetime of the program, otherwise tracing will be shutdown"]
pub struct TracingGuard {
_sentry_guard: Option<sentry::ClientInitGuard>,
}
Expand Down