Improve Labrinth Sentry integration (#5174)

* Improve Sentry integration

* remove debug routes

* fix ci

* sentry tracing stuff

* Add spans to Sentry logging

* Fix CI

* Redis op instrumentation

* pr comments

Author: aecsocket

Cargo.lock

@@ -150,7 +150,6 @@ sentry = { version = "0.45.0", default-features = false, features = [
   "reqwest",
   "rustls",
 ] }
-sentry-actix = "0.45.0"
 serde = "1.0.228"
 serde_bytes = "0.11.19"
 serde_cbor = "0.11.2"
@@ -239,7 +238,7 @@ manual_assert = "warn"
 manual_instant_elapsed = "warn"
 manual_is_variant_and = "warn"
 manual_let_else = "warn"
-map_unwrap_or = "warn"
+map_unwrap_or = "allow"
 match_bool = "warn"
 needless_collect = "warn"
 negative_feature_names = "warn"

Cargo.toml

@@ -1,6 +1,8 @@
 DEBUG=true
 RUST_LOG=info,sqlx::query=warn
 SENTRY_DSN=none
+SENTRY_ENVIRONMENT=development
+SENTRY_TRACES_SAMPLE_RATE=0.1
 
 SITE_URL=http://localhost:3000
 # This CDN URL matches the local storage backend set below, which uses MOCK_FILE_PATH

apps/labrinth/.env.docker-compose

@@ -1,6 +1,8 @@
 DEBUG=true
 RUST_LOG=info,sqlx::query=warn
 SENTRY_DSN=none
+SENTRY_ENVIRONMENT=development
+SENTRY_TRACES_SAMPLE_RATE=0.1
 
 SITE_URL=http://localhost:3000
 # This CDN URL matches the local storage backend set below, which uses MOCK_FILE_PATH

apps/labrinth/.env.local

@@ -12,7 +12,7 @@ path = "src/main.rs"
 [dependencies]
 actix-cors = { workspace = true }
 actix-files = { workspace = true }
-actix-http = { workspace = true, optional = true }
+actix-http = { workspace = true }
 actix-multipart = { workspace = true }
 actix-rt = { workspace = true }
 actix-web = { workspace = true }
@@ -70,7 +70,7 @@ itertools = { workspace = true }
 json-patch = { workspace = true }
 lettre = { workspace = true }
 meilisearch-sdk = { workspace = true, features = ["reqwest"] }
-modrinth-util = { workspace = true, features = ["decimal", "utoipa"] }
+modrinth-util = { workspace = true, features = ["decimal", "sentry", "utoipa"] }
 muralpay = { workspace = true, features = ["client", "mock", "utoipa"] }
 murmur2 = { workspace = true }
 paste = { workspace = true }
@@ -95,7 +95,6 @@ rust-s3 = { workspace = true }
 rustls.workspace = true
 rusty-money = { workspace = true }
 sentry = { workspace = true }
-sentry-actix = { workspace = true }
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
 serde_with = { workspace = true }
@@ -149,7 +148,7 @@ tikv-jemallocator = { workspace = true, features = [
 ] }
 
 [features]
-test = ["dep:actix-http"]
+test = []
 
 [lints]
 workspace = true

apps/labrinth/Cargo.toml

@@ -71,6 +71,7 @@ where
     Ok((scopes, db_user))
 }
 
+#[tracing::instrument(skip(req, executor, redis, session_queue))]
 pub async fn get_user_from_headers<'a, E>(
     req: &HttpRequest,
     executor: E,

apps/labrinth/src/auth/validate.rs

@@ -128,13 +128,15 @@ impl RedisPool {
         Ok(())
     }
 
+    #[tracing::instrument(skip(self))]
     pub async fn connect(&self) -> Result<RedisConnection, DatabaseError> {
         Ok(RedisConnection {
             connection: self.pool.get().await?,
             meta_namespace: self.meta_namespace.clone(),
         })
     }
 
+    #[tracing::instrument(skip(self, closure))]
     pub async fn get_cached_keys<F, Fut, T, K>(
         &self,
         namespace: &str,
@@ -162,6 +164,7 @@ impl RedisPool {
             .collect())
     }
 
+    #[tracing::instrument(skip(self, closure))]
     pub async fn get_cached_keys_raw<F, Fut, T, K>(
         &self,
         namespace: &str,
@@ -197,6 +200,7 @@ impl RedisPool {
         .await
     }
 
+    #[tracing::instrument(skip(self, closure))]
     pub async fn get_cached_keys_with_slug<F, Fut, T, I, K, S>(
         &self,
         namespace: &str,
@@ -233,6 +237,7 @@ impl RedisPool {
             .collect())
     }
 
+    #[tracing::instrument(skip(self, closure))]
     pub async fn get_cached_keys_raw_with_slug<F, Fut, T, I, K, S>(
         &self,
         namespace: &str,
@@ -585,6 +590,7 @@ impl RedisPool {
 }
 
 impl RedisConnection {
+    #[tracing::instrument(skip(self))]
     pub async fn set(
         &mut self,
         namespace: &str,
@@ -607,6 +613,7 @@ impl RedisConnection {
         Ok(())
     }
 
+    #[tracing::instrument(skip(self, id, data))]
     pub async fn set_serialized_to_json<Id, D>(
         &mut self,
         namespace: &str,
@@ -627,6 +634,7 @@ impl RedisConnection {
         .await
     }
 
+    #[tracing::instrument(skip(self))]
     pub async fn get(
         &mut self,
         namespace: &str,
@@ -642,6 +650,7 @@ impl RedisConnection {
         Ok(res)
     }
 
+    #[tracing::instrument(skip(self))]
     pub async fn get_many(
         &mut self,
         namespace: &str,
@@ -659,6 +668,7 @@ impl RedisConnection {
         Ok(res)
     }
 
+    #[tracing::instrument(skip(self))]
     pub async fn get_deserialized_from_json<R>(
         &mut self,
         namespace: &str,
@@ -673,6 +683,7 @@ impl RedisConnection {
             .and_then(|x| serde_json::from_str(&x).ok()))
     }
 
+    #[tracing::instrument(skip(self))]
     pub async fn get_many_deserialized_from_json<R>(
         &mut self,
         namespace: &str,
@@ -689,6 +700,7 @@ impl RedisConnection {
             .collect::<Vec<_>>())
     }
 
+    #[tracing::instrument(skip(self, id))]
     pub async fn delete<T1>(
         &mut self,
         namespace: &str,
@@ -707,6 +719,7 @@ impl RedisConnection {
         Ok(())
     }
 
+    #[tracing::instrument(skip(self, iter))]
     pub async fn delete_many(
         &mut self,
         iter: impl IntoIterator<Item = (&str, Option<String>)>,
@@ -731,6 +744,7 @@ impl RedisConnection {
         Ok(())
     }
 
+    #[tracing::instrument(skip(self, value))]
     pub async fn lpush(
         &mut self,
         namespace: &str,
@@ -742,6 +756,7 @@ impl RedisConnection {
         Ok(())
     }
 
+    #[tracing::instrument(skip(self))]
     pub async fn brpop(
         &mut self,
         namespace: &str,

apps/labrinth/src/database/redis.rs

@@ -375,6 +375,8 @@ pub fn check_env_vars() -> bool {
         check
     }
 
+    failed |= check_var::<String>("SENTRY_ENVIRONMENT");
+    failed |= check_var::<String>("SENTRY_TRACES_SAMPLE_RATE");
     failed |= check_var::<String>("SITE_URL");
     failed |= check_var::<String>("CDN_URL");
     failed |= check_var::<String>("LABRINTH_ADMIN_KEY");

apps/labrinth/src/lib.rs

@@ -54,10 +54,7 @@ struct Args {
     run_background_task: Option<BackgroundTask>,
 }
 
-#[actix_rt::main]
-async fn main() -> std::io::Result<()> {
-    let args = Args::parse();
-
+fn main() -> std::io::Result<()> {
     color_eyre::install().expect("failed to install `color-eyre`");
     dotenvy::dotenv().ok();
     modrinth_util::log::init().expect("failed to initialize logging");
@@ -67,15 +64,17 @@ async fn main() -> std::io::Result<()> {
         std::process::exit(1);
     }
 
-    rustls::crypto::aws_lc_rs::default_provider()
-        .install_default()
-        .unwrap();
-
+    // Sentry must be set up before the async runtime is started
+    // <https://docs.sentry.io/platforms/rust/guides/actix-web/>
     // DSN is from SENTRY_DSN env variable.
     // Has no effect if not set.
     let sentry = sentry::init(sentry::ClientOptions {
         release: sentry::release_name!(),
-        traces_sample_rate: 0.1,
+        traces_sample_rate: dotenvy::var("SENTRY_TRACES_SAMPLE_RATE")
+            .unwrap()
+            .parse()
+            .expect("failed to parse `SENTRY_TRACES_SAMPLE_RATE` as number"),
+        environment: Some(dotenvy::var("SENTRY_ENVIRONMENT").unwrap().into()),
         ..Default::default()
     });
     if sentry.is_enabled() {
@@ -85,6 +84,20 @@ async fn main() -> std::io::Result<()> {
         }
     }
 
+    actix_rt::System::new().block_on(app())?;
+
+    // Sentry guard must live until the end of the app
+    drop(sentry);
+    Ok(())
+}
+
+async fn app() -> std::io::Result<()> {
+    let args = Args::parse();
+
+    rustls::crypto::aws_lc_rs::default_provider()
+        .install_default()
+        .unwrap();
+
     if args.run_background_task.is_none() {
         info!(
             "Starting labrinth on {}",
@@ -245,7 +258,12 @@ async fn main() -> std::io::Result<()> {
             .wrap(prometheus.clone())
             .wrap(from_fn(rate_limit_middleware))
             .wrap(actix_web::middleware::Compress::default())
-            .wrap(sentry_actix::Sentry::new())
+            // Sentry integration
+            // `sentry_actix::Sentry` provides an Actix middleware for making
+            // transactions out of HTTP requests. However, we have to use our
+            // own - See `sentry::SentryErrorReporting` for why.
+            .wrap(labrinth::util::sentry::SentryErrorReporting)
+            // Use `utoipa` for OpenAPI generation
             .into_utoipa_app()
             .configure(|cfg| utoipa_app_config(cfg, labrinth_config.clone()))
             .openapi_service(|api| SwaggerUi::new("/docs/swagger-ui/{_:.*}")
@@ -280,11 +298,11 @@ impl utoipa::Modify for SecurityAddon {
 fn log_error(err: &actix_web::Error) {
     if err.as_response_error().status_code().is_client_error() {
         tracing::debug!(
-            "Error encountered while processing the incoming HTTP request: {err:#?}"
+            "Error encountered while processing the incoming HTTP request: {err:#}"
         );
     } else {
         tracing::error!(
-            "Error encountered while processing the incoming HTTP request: {err:#?}"
+            "Error encountered while processing the incoming HTTP request: {err:#}"
         );
     }
 }

apps/labrinth/src/main.rs

@@ -16,5 +16,6 @@ pub mod ip;
 pub mod ratelimit;
 pub mod redis;
 pub mod routes;
+pub mod sentry;
 pub mod validate;
 pub mod webhook;