handlers::compat: don't rely on #[instrument(err)] for logging errors

crates/handlers/src/compat/login.rs

@@ -10,7 +10,7 @@ use axum::{Json, extract::State, response::IntoResponse};
 use axum_extra::typed_header::TypedHeader;
 use chrono::Duration;
 use hyper::StatusCode;
-use mas_axum_utils::sentry::SentryEventID;
+use mas_axum_utils::record_error;
 use mas_data_model::{
     CompatSession, CompatSsoLoginState, Device, SiteConfig, TokenType, User, UserAgent,
 };
@@ -210,7 +210,8 @@ impl_from_error_for_route!(mas_storage::RepositoryError);
 
 impl IntoResponse for RouteError {
     fn into_response(self) -> axum::response::Response {
-        let event_id = sentry::capture_error(&self);
+        let sentry_event_id =
+            record_error!(self, Self::Internal(_) | Self::ProvisionDeviceFailed(_));
         LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]);
         let response = match self {
             Self::Internal(_) | Self::ProvisionDeviceFailed(_) => MatrixError {
@@ -257,11 +258,11 @@ impl IntoResponse for RouteError {
             },
         };
 
-        (SentryEventID::from(event_id), response).into_response()
+        (sentry_event_id, response).into_response()
     }
 }
 
-#[tracing::instrument(name = "handlers.compat.login.post", skip_all, err)]
+#[tracing::instrument(name = "handlers.compat.login.post", skip_all)]
 pub(crate) async fn post(
     mut rng: BoxRng,
     clock: BoxClock,

crates/handlers/src/compat/login_sso_complete.rs

@@ -48,7 +48,6 @@ pub struct Params {
     name = "handlers.compat.login_sso_complete.get",
     fields(compat_sso_login.id = %id),
     skip_all,
-    err,
 )]
 pub async fn get(
     PreferredLanguage(locale): PreferredLanguage,
@@ -121,7 +120,6 @@ pub async fn get(
     name = "handlers.compat.login_sso_complete.post",
     fields(compat_sso_login.id = %id),
     skip_all,
-    err,
 )]
 pub async fn post(
     mut rng: BoxRng,

crates/handlers/src/compat/login_sso_redirect.rs

@@ -9,7 +9,7 @@ use axum::{
     response::IntoResponse,
 };
 use hyper::StatusCode;
-use mas_axum_utils::sentry::SentryEventID;
+use mas_axum_utils::record_error;
 use mas_router::{CompatLoginSsoAction, CompatLoginSsoComplete, UrlBuilder};
 use mas_storage::{BoxClock, BoxRepository, BoxRng, compat::CompatSsoLoginRepository};
 use rand::distributions::{Alphanumeric, DistString};
@@ -43,17 +43,16 @@ impl_from_error_for_route!(mas_storage::RepositoryError);
 
 impl IntoResponse for RouteError {
     fn into_response(self) -> axum::response::Response {
-        let event_id = sentry::capture_error(&self);
-        (
-            StatusCode::INTERNAL_SERVER_ERROR,
-            SentryEventID::from(event_id),
-            format!("{self}"),
-        )
-            .into_response()
+        let sentry_event_id = record_error!(self, Self::Internal(_));
+        let status_code = match &self {
+            Self::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR,
+            Self::MissingRedirectUrl | Self::InvalidRedirectUrl => StatusCode::BAD_REQUEST,
+        };
+        (status_code, sentry_event_id, format!("{self}")).into_response()
     }
 }
 
-#[tracing::instrument(name = "handlers.compat.login_sso_redirect.get", skip_all, err)]
+#[tracing::instrument(name = "handlers.compat.login_sso_redirect.get", skip_all)]
 pub async fn get(
     mut rng: BoxRng,
     clock: BoxClock,

crates/handlers/src/compat/logout.rs

@@ -10,7 +10,7 @@ use axum::{Json, response::IntoResponse};
 use axum_extra::typed_header::TypedHeader;
 use headers::{Authorization, authorization::Bearer};
 use hyper::StatusCode;
-use mas_axum_utils::sentry::SentryEventID;
+use mas_axum_utils::record_error;
 use mas_data_model::TokenType;
 use mas_storage::{
     BoxClock, BoxRepository, BoxRng, Clock, RepositoryAccess,
@@ -51,7 +51,7 @@ impl_from_error_for_route!(mas_storage::RepositoryError);
 
 impl IntoResponse for RouteError {
     fn into_response(self) -> axum::response::Response {
-        let event_id = sentry::capture_error(&self);
+        let sentry_event_id = record_error!(self, Self::Internal(_));
         LOGOUT_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]);
         let response = match self {
             Self::Internal(_) => MatrixError {
@@ -71,11 +71,11 @@ impl IntoResponse for RouteError {
             },
         };
 
-        (SentryEventID::from(event_id), response).into_response()
+        (sentry_event_id, response).into_response()
     }
 }
 
-#[tracing::instrument(name = "handlers.compat.logout.post", skip_all, err)]
+#[tracing::instrument(name = "handlers.compat.logout.post", skip_all)]
 pub(crate) async fn post(
     clock: BoxClock,
     mut rng: BoxRng,

crates/handlers/src/compat/mod.rs

@@ -14,7 +14,7 @@ use axum::{
     response::IntoResponse,
 };
 use hyper::{StatusCode, header};
-use mas_axum_utils::sentry::SentryEventID;
+use mas_axum_utils::record_error;
 use serde::{Serialize, de::DeserializeOwned};
 use thiserror::Error;
 
@@ -59,7 +59,7 @@ pub enum MatrixJsonBodyRejection {
 
 impl IntoResponse for MatrixJsonBodyRejection {
     fn into_response(self) -> axum::response::Response {
-        let event_id = sentry::capture_error(&self);
+        let sentry_event_id = record_error!(self, !);
         let response = match self {
             Self::InvalidContentType | Self::ContentTypeNotJson(_) => MatrixError {
                 errcode: "M_NOT_JSON",
@@ -102,7 +102,7 @@ impl IntoResponse for MatrixJsonBodyRejection {
             },
         };
 
-        (SentryEventID::from(event_id), response).into_response()
+        (sentry_event_id, response).into_response()
     }
 }
 

crates/handlers/src/compat/refresh.rs

@@ -7,7 +7,7 @@
 use axum::{Json, extract::State, response::IntoResponse};
 use chrono::Duration;
 use hyper::StatusCode;
-use mas_axum_utils::sentry::SentryEventID;
+use mas_axum_utils::record_error;
 use mas_data_model::{SiteConfig, TokenFormatError, TokenType};
 use mas_storage::{
     BoxClock, BoxRepository, BoxRng, Clock,
@@ -16,6 +16,7 @@ use mas_storage::{
 use serde::{Deserialize, Serialize};
 use serde_with::{DurationMilliSeconds, serde_as};
 use thiserror::Error;
+use ulid::Ulid;
 
 use super::MatrixError;
 use crate::{BoundActivityTracker, impl_from_error_for_route};
@@ -31,46 +32,50 @@ pub enum RouteError {
     Internal(Box<dyn std::error::Error + Send + Sync + 'static>),
 
     #[error("invalid token")]
-    InvalidToken,
+    InvalidToken(#[from] TokenFormatError),
 
-    #[error("refresh token already consumed")]
-    RefreshTokenConsumed,
+    #[error("unknown token")]
+    UnknownToken,
 
-    #[error("invalid session")]
-    InvalidSession,
+    #[error("invalid token type {0}, expected a compat refresh token")]
+    InvalidTokenType(TokenType),
 
-    #[error("unknown session")]
-    UnknownSession,
+    #[error("refresh token already consumed {0}")]
+    RefreshTokenConsumed(Ulid),
+
+    #[error("invalid compat session {0}")]
+    InvalidSession(Ulid),
+
+    #[error("unknown comapt session {0}")]
+    UnknownSession(Ulid),
 }
 
 impl IntoResponse for RouteError {
     fn into_response(self) -> axum::response::Response {
-        let event_id = sentry::capture_error(&self);
+        let sentry_event_id = record_error!(self, Self::Internal(_) | Self::UnknownSession(_));
         let response = match self {
-            Self::Internal(_) | Self::UnknownSession => MatrixError {
+            Self::Internal(_) | Self::UnknownSession(_) => MatrixError {
                 errcode: "M_UNKNOWN",
                 error: "Internal error",
                 status: StatusCode::INTERNAL_SERVER_ERROR,
             },
-            Self::InvalidToken | Self::InvalidSession | Self::RefreshTokenConsumed => MatrixError {
+            Self::InvalidToken(_)
+            | Self::UnknownToken
+            | Self::InvalidTokenType(_)
+            | Self::InvalidSession(_)
+            | Self::RefreshTokenConsumed(_) => MatrixError {
                 errcode: "M_UNKNOWN_TOKEN",
                 error: "Invalid refresh token",
                 status: StatusCode::UNAUTHORIZED,
             },
         };
 
-        (SentryEventID::from(event_id), response).into_response()
+        (sentry_event_id, response).into_response()
     }
 }
 
 impl_from_error_for_route!(mas_storage::RepositoryError);
 
-impl From<TokenFormatError> for RouteError {
-    fn from(_e: TokenFormatError) -> Self {
-        Self::InvalidToken
-    }
-}
-
 #[serde_as]
 #[derive(Debug, Serialize)]
 pub struct ResponseBody {
@@ -80,7 +85,7 @@ pub struct ResponseBody {
     expires_in_ms: Duration,
 }
 
-#[tracing::instrument(name = "handlers.compat.refresh.post", skip_all, err)]
+#[tracing::instrument(name = "handlers.compat.refresh.post", skip_all)]
 pub(crate) async fn post(
     mut rng: BoxRng,
     clock: BoxClock,
@@ -92,27 +97,27 @@ pub(crate) async fn post(
     let token_type = TokenType::check(&input.refresh_token)?;
 
     if token_type != TokenType::CompatRefreshToken {
-        return Err(RouteError::InvalidToken);
+        return Err(RouteError::InvalidTokenType(token_type));
     }
 
     let refresh_token = repo
         .compat_refresh_token()
         .find_by_token(&input.refresh_token)
         .await?
-        .ok_or(RouteError::InvalidToken)?;
+        .ok_or(RouteError::UnknownToken)?;
 
     if !refresh_token.is_valid() {
-        return Err(RouteError::RefreshTokenConsumed);
+        return Err(RouteError::RefreshTokenConsumed(refresh_token.id));
     }
 
     let session = repo
         .compat_session()
         .lookup(refresh_token.session_id)
         .await?
-        .ok_or(RouteError::UnknownSession)?;
+        .ok_or(RouteError::UnknownSession(refresh_token.session_id))?;
 
     if !session.is_valid() {
-        return Err(RouteError::InvalidSession);
+        return Err(RouteError::InvalidSession(refresh_token.session_id));
     }
 
     activity_tracker