From 1117d49746b5e9635478816dfbd165ebf8e57f23 Mon Sep 17 00:00:00 2001 From: Alphonse Paix Date: Sun, 28 Sep 2025 03:37:23 +0200 Subject: [PATCH] Update telemetry --- src/authentication.rs | 12 ++---- src/idempotency/key.rs | 1 + src/idempotency/persistance.rs | 5 +++ src/issue_delivery_worker.rs | 8 ++-- src/routes.rs | 4 +- src/routes/admin.rs | 2 +- src/routes/admin/change_password.rs | 18 ++++----- src/routes/admin/dashboard.rs | 1 + src/routes/admin/newsletters.rs | 17 +++++---- src/routes/admin/posts.rs | 16 ++++---- src/routes/admin/subscribers.rs | 44 ++++++++++++++++------ src/routes/health_check.rs | 4 +- src/routes/home.rs | 11 +++--- src/routes/login.rs | 1 + src/routes/posts.rs | 4 ++ src/routes/subscriptions.rs | 14 ++----- src/routes/subscriptions_confirm.rs | 58 ++++++++++------------------- src/routes/unsubscribe.rs | 10 +++-- src/startup.rs | 2 +- tests/api/subscriptions_confirm.rs | 4 +- 20 files changed, 120 insertions(+), 116 deletions(-) diff --git a/src/authentication.rs b/src/authentication.rs index 73dd586..ec0bcdb 100644 --- a/src/authentication.rs +++ b/src/authentication.rs @@ -56,10 +56,7 @@ fn compute_pasword_hash(password: SecretString) -> Result for IdempotencyKey { diff --git a/src/idempotency/persistance.rs b/src/idempotency/persistance.rs index facb520..36b48bd 100644 --- a/src/idempotency/persistance.rs +++ b/src/idempotency/persistance.rs @@ -16,6 +16,10 @@ struct HeaderPairRecord { value: Vec, } +#[tracing::instrument( + name = "Fetching saved response in database if it exists", + skip(connection_pool) +)] pub async fn get_saved_response( connection_pool: &PgPool, idempotency_key: &IdempotencyKey, @@ -53,6 +57,7 @@ pub async fn get_saved_response( } } +#[tracing::instrument(name = "Saving response in database", skip(transaction, response))] pub async fn save_response( mut transaction: Transaction<'static, Postgres>, idempotency_key: &IdempotencyKey, diff --git a/src/issue_delivery_worker.rs b/src/issue_delivery_worker.rs index bd3e218..8dbe6f9 100644 --- a/src/issue_delivery_worker.rs +++ b/src/issue_delivery_worker.rs @@ -56,7 +56,7 @@ pub async fn try_execute_task( let mut issue = get_issue(connection_pool, task.newsletter_issue_id).await?; issue.inject_unsubscribe_token(&task.unsubscribe_token); if task.kind == EmailType::NewPost.to_string() { - issue.create_tracking_info(&mut transaction).await?; + issue.inject_tracking_info(&mut transaction).await?; } if let Err(e) = email_client .send_email( @@ -68,14 +68,14 @@ pub async fn try_execute_task( .await { tracing::error!( - error.message = %e, + error = %e, "Failed to deliver issue to confirmed subscriber. Skipping." ); } } Err(e) => { tracing::error!( - error.message = %e, + error = %e, "Skipping a subscriber. Their stored contact details are invalid." ); } @@ -103,7 +103,7 @@ impl NewsletterIssue { self.html_content = self.html_content.replace("UNSUBSCRIBE_TOKEN", token); } - async fn create_tracking_info( + async fn inject_tracking_info( &mut self, transaction: &mut Transaction<'static, Postgres>, ) -> Result<(), anyhow::Error> { diff --git a/src/routes.rs b/src/routes.rs index 474c00f..e81d17c 100644 --- a/src/routes.rs +++ b/src/routes.rs @@ -160,12 +160,12 @@ impl From for AppError { } pub async fn not_found() -> Response { - (StatusCode::NOT_FOUND, not_found_html()).into_response() + not_found_html() } pub fn not_found_html() -> Response { let template = HtmlTemplate(NotFoundTemplate); - template.into_response() + (StatusCode::NOT_FOUND, template).into_response() } pub struct Path(T); diff --git a/src/routes/admin.rs b/src/routes/admin.rs index 147fc68..2cc0474 100644 --- a/src/routes/admin.rs +++ b/src/routes/admin.rs @@ -25,7 +25,7 @@ pub enum AdminError { #[error("Trying to access admin dashboard without authentication.")] NotAuthenticated, #[error("Updating password failed.")] - ChangePassword(String), + ChangePassword(anyhow::Error), #[error("Could not publish newsletter.")] Publish(#[source] anyhow::Error), #[error("The idempotency key was invalid.")] diff --git a/src/routes/admin/change_password.rs b/src/routes/admin/change_password.rs index 0fcd0ad..70f623b 100644 --- a/src/routes/admin/change_password.rs +++ b/src/routes/admin/change_password.rs @@ -31,16 +31,16 @@ pub async fn change_password( password: form.current_password, }; if form.new_password.expose_secret() != form.new_password_check.expose_secret() { - Err(AdminError::ChangePassword( - "You entered two different passwords - the field values must match.".to_string(), - ) + Err(AdminError::ChangePassword(anyhow::anyhow!( + "You entered two different passwords - the field values must match." + )) .into()) } else if let Err(e) = validate_credentials(credentials, &connection_pool).await { match e { AuthError::UnexpectedError(error) => Err(AdminError::UnexpectedError(error).into()), - AuthError::InvalidCredentials(_) => Err(AdminError::ChangePassword( - "The current password is incorrect.".to_string(), - ) + AuthError::InvalidCredentials(_) => Err(AdminError::ChangePassword(anyhow::anyhow!( + "The current password is incorrect." + )) .into()), } } else if let Err(e) = verify_password(form.new_password.expose_secret()) { @@ -48,7 +48,7 @@ pub async fn change_password( } else { authentication::change_password(user_id, form.new_password, &connection_pool) .await - .map_err(|e| AdminError::ChangePassword(e.to_string()))?; + .map_err(AdminError::ChangePassword)?; let template = MessageTemplate::Success { message: "Your password has been changed.".to_string(), }; @@ -56,9 +56,9 @@ pub async fn change_password( } } -fn verify_password(password: &str) -> Result<(), String> { +fn verify_password(password: &str) -> Result<(), anyhow::Error> { if password.len() < 12 || password.len() > 128 { - return Err("The password must contain between 12 and 128 characters.".into()); + anyhow::bail!("The password must contain between 12 and 128 characters."); } Ok(()) } diff --git a/src/routes/admin/dashboard.rs b/src/routes/admin/dashboard.rs index a883b34..87d3095 100644 --- a/src/routes/admin/dashboard.rs +++ b/src/routes/admin/dashboard.rs @@ -57,6 +57,7 @@ pub async fn admin_dashboard( Ok(Html(template.render().unwrap()).into_response()) } +#[tracing::instrument("Computing dashboard stats", skip_all)] async fn get_stats(connection_pool: &PgPool) -> Result { let subscribers = sqlx::query_scalar!("SELECT count(*) FROM subscriptions WHERE status = 'confirmed'") diff --git a/src/routes/admin/newsletters.rs b/src/routes/admin/newsletters.rs index ffbc1f8..8a7c608 100644 --- a/src/routes/admin/newsletters.rs +++ b/src/routes/admin/newsletters.rs @@ -1,5 +1,3 @@ -use std::fmt::Display; - use crate::{ authentication::AuthenticatedUser, idempotency::{IdempotencyKey, save_response, try_processing}, @@ -15,6 +13,7 @@ use axum::{ response::{Html, IntoResponse, Response}, }; use sqlx::{Executor, Postgres, Transaction}; +use std::fmt::Display; use uuid::Uuid; #[derive(serde::Deserialize)] @@ -25,13 +24,14 @@ pub struct BodyData { idempotency_key: String, } -#[tracing::instrument(skip_all)] +#[tracing::instrument(name = "Creating newsletter isue", skip_all, fields(issue_id = tracing::field::Empty))] pub async fn insert_newsletter_issue( transaction: &mut Transaction<'static, Postgres>, title: &str, email_template: &dyn EmailTemplate, ) -> Result { let newsletter_issue_id = Uuid::new_v4(); + tracing::Span::current().record("issue_id", newsletter_issue_id.to_string()); let query = sqlx::query!( r#" INSERT INTO newsletter_issues ( @@ -48,6 +48,7 @@ pub async fn insert_newsletter_issue( Ok(newsletter_issue_id) } +#[derive(Debug)] pub enum EmailType { NewPost, Newsletter, @@ -62,7 +63,7 @@ impl Display for EmailType { } } -#[tracing::instrument(skip_all)] +#[tracing::instrument(name = "Adding new task to queue", skip(transaction))] pub async fn enqueue_delivery_tasks( transaction: &mut Transaction<'static, Postgres>, newsletter_issue_id: Uuid, @@ -87,7 +88,7 @@ pub async fn enqueue_delivery_tasks( Ok(()) } -#[tracing::instrument(name = "Publishing a newsletter", skip(connection_pool, form))] +#[tracing::instrument(name = "Publishing a newsletter", skip_all, fields(title = %form.title))] pub async fn publish_newsletter( State(AppState { connection_pool, @@ -134,12 +135,12 @@ pub async fn publish_newsletter( Ok(response) } -fn validate_form(form: &BodyData) -> Result<(), &'static str> { +fn validate_form(form: &BodyData) -> Result<(), anyhow::Error> { if form.title.is_empty() { - return Err("The title was empty."); + anyhow::bail!("The title was empty."); } if form.html.is_empty() || form.text.is_empty() { - return Err("The content was empty."); + anyhow::bail!("The content was empty."); } Ok(()) } diff --git a/src/routes/admin/posts.rs b/src/routes/admin/posts.rs index b18aca5..1c1193f 100644 --- a/src/routes/admin/posts.rs +++ b/src/routes/admin/posts.rs @@ -33,7 +33,11 @@ fn validate_form(form: &CreatePostForm) -> Result<(), anyhow::Error> { } } -#[tracing::instrument(name = "Creating a post", skip(connection_pool, form))] +#[tracing::instrument( + name = "Publishing new blog post", + skip(connection_pool, base_url, form) + fields(title = %form.title) +)] pub async fn create_post( State(AppState { connection_pool, @@ -79,10 +83,7 @@ pub async fn create_post( Ok(response) } -#[tracing::instrument( - name = "Saving new post in the database", - skip(transaction, title, content, author) -)] +#[tracing::instrument(name = "Saving new blog post in the database", skip_all)] pub async fn insert_post( transaction: &mut Transaction<'static, Postgres>, title: &str, @@ -105,10 +106,7 @@ pub async fn insert_post( Ok(post_id) } -#[tracing::instrument( - name = "Creating newsletter for new post", - skip(transaction, post_title, post_id) -)] +#[tracing::instrument(name = "Creating newsletter for new post", skip_all)] pub async fn create_newsletter( transaction: &mut Transaction<'static, Postgres>, base_url: &str, diff --git a/src/routes/admin/subscribers.rs b/src/routes/admin/subscribers.rs index 9c26d1f..2db78cf 100644 --- a/src/routes/admin/subscribers.rs +++ b/src/routes/admin/subscribers.rs @@ -15,6 +15,10 @@ use uuid::Uuid; const SUBS_PER_PAGE: i64 = 5; +#[tracing::instrument( + name = "Retrieving most recent subscribers from database", + skip(connection_pool) +)] pub async fn get_subscribers_page( State(AppState { connection_pool, .. @@ -38,34 +42,52 @@ pub async fn get_subscribers_page( Ok(Html(template.render().unwrap()).into_response()) } +#[tracing::instrument( + name = "Deleting subscriber from database", + skip(connection_pool), + fields(email=tracing::field::Empty) +)] pub async fn delete_subscriber( State(AppState { connection_pool, .. }): State, Path(subscriber_id): Path, ) -> Result { - let res = sqlx::query!("DELETE FROM subscriptions WHERE id = $1", subscriber_id) - .execute(&connection_pool) - .await - .context("Failed to delete subscriber from database.") - .map_err(AppError::unexpected_message)?; - if res.rows_affected() > 1 { + let res = sqlx::query!( + "DELETE FROM subscriptions WHERE id = $1 RETURNING email", + subscriber_id + ) + .fetch_optional(&connection_pool) + .await + .context("Failed to delete subscriber from database.") + .map_err(AppError::unexpected_message)?; + if let Some(record) = res { + tracing::Span::current().record("email", tracing::field::display(&record.email)); + let template = MessageTemplate::Success { + message: format!( + "The subscriber with email '{}' has been deleted.", + record.email + ), + }; + Ok(template.render().unwrap().into_response()) + } else { Err(AppError::unexpected_message(anyhow::anyhow!( "We could not find the subscriber in the database." ))) - } else { - let template = MessageTemplate::Success { - message: "The subscriber has been deleted.".into(), - }; - Ok(template.render().unwrap().into_response()) } } +#[tracing::instrument( + name = "Retrieving next subscribers in database", + skip(connection_pool), + fields(offset = tracing::field::Empty) +)] pub async fn get_subs( connection_pool: &PgPool, page: i64, ) -> Result, sqlx::Error> { let offset = (page - 1) * SUBS_PER_PAGE; + tracing::Span::current().record("offset", tracing::field::display(&offset)); let subscribers = sqlx::query_as!( SubscriberEntry, "SELECT * FROM subscriptions ORDER BY subscribed_at DESC LIMIT $1 OFFSET $2", diff --git a/src/routes/health_check.rs b/src/routes/health_check.rs index be39b52..64fd1f8 100644 --- a/src/routes/health_check.rs +++ b/src/routes/health_check.rs @@ -1,5 +1,5 @@ -use axum::{http::StatusCode, response::IntoResponse}; +use axum::http::StatusCode; -pub async fn health_check() -> impl IntoResponse { +pub async fn health_check() -> StatusCode { StatusCode::OK } diff --git a/src/routes/home.rs b/src/routes/home.rs index ac833a6..dccb493 100644 --- a/src/routes/home.rs +++ b/src/routes/home.rs @@ -1,8 +1,7 @@ -use askama::Template; -use axum::response::Html; +use crate::templates::{HomeTemplate, HtmlTemplate}; +use axum::response::{IntoResponse, Response}; -use crate::templates::HomeTemplate; - -pub async fn home() -> Html { - Html(HomeTemplate.render().unwrap()) +pub async fn home() -> Response { + let template = HtmlTemplate(HomeTemplate); + template.into_response() } diff --git a/src/routes/login.rs b/src/routes/login.rs index bacd58b..9bdd16e 100644 --- a/src/routes/login.rs +++ b/src/routes/login.rs @@ -35,6 +35,7 @@ pub async fn get_login(session: TypedSession) -> Result { } } +#[tracing::instrument(name = "Authenticating user", skip_all, fields(name = %form.username))] pub async fn post_login( session: TypedSession, State(AppState { diff --git a/src/routes/posts.rs b/src/routes/posts.rs index f001ec8..3191e2c 100644 --- a/src/routes/posts.rs +++ b/src/routes/posts.rs @@ -15,6 +15,7 @@ use uuid::Uuid; const NUM_PER_PAGE: i64 = 3; +#[tracing::instrument(name = "Fetching most recent posts from database", skip_all)] pub async fn list_posts( State(AppState { connection_pool, .. @@ -67,6 +68,7 @@ pub struct PostParams { origin: Option, } +#[tracing::instrument(name = "Fetching post from database", skip(connection_pool, origin))] pub async fn see_post( State(AppState { connection_pool, .. @@ -94,6 +96,7 @@ pub async fn see_post( } } +#[tracing::instrument(name = "Mark email notification as opened", skip(connection_pool))] async fn mark_email_as_opened(connection_pool: &PgPool, email_id: Uuid) -> Result<(), AppError> { sqlx::query!( "UPDATE notifications_delivered SET opened = TRUE WHERE email_id = $1", @@ -129,6 +132,7 @@ pub struct LoadMoreParams { page: i64, } +#[tracing::instrument(name = "Fetching next posts in the database", skip(connection_pool))] pub async fn load_more( State(AppState { connection_pool, .. diff --git a/src/routes/subscriptions.rs b/src/routes/subscriptions.rs index 54be568..943a3f3 100644 --- a/src/routes/subscriptions.rs +++ b/src/routes/subscriptions.rs @@ -19,9 +19,9 @@ use uuid::Uuid; #[tracing::instrument( name = "Adding a new subscriber", - skip(connection_pool, email_client, base_url, form), + skip_all, fields( - subscriber_email = %form.email, + email = %form.email, ) )] pub async fn subscribe( @@ -72,10 +72,7 @@ pub async fn subscribe( Ok(Html(template.render().unwrap()).into_response()) } -#[tracing::instrument( - name = "Saving new subscriber details in the database", - skip(transaction, new_subscriber) -)] +#[tracing::instrument(name = "Saving new subscriber details in the database", skip_all)] pub async fn insert_subscriber( transaction: &mut Transaction<'_, Postgres>, new_subscriber: &NewSubscriber, @@ -123,10 +120,7 @@ async fn store_token( Ok(()) } -#[tracing::instrument( - name = "Send a confirmation email to a new subscriber", - skip(email_client, new_subscriber, base_url, subscription_token) -)] +#[tracing::instrument(name = "Send confirmation email to the new subscriber", skip_all)] pub async fn send_confirmation_email( email_client: &EmailClient, new_subscriber: &NewSubscriber, diff --git a/src/routes/subscriptions_confirm.rs b/src/routes/subscriptions_confirm.rs index c45a208..2c3fa06 100644 --- a/src/routes/subscriptions_confirm.rs +++ b/src/routes/subscriptions_confirm.rs @@ -1,48 +1,39 @@ use crate::{ - routes::{Query, generate_token}, + routes::{AppError, Query, generate_token, not_found_html}, startup::AppState, - templates::ConfirmTemplate, + templates::{ConfirmTemplate, HtmlTemplate}, }; -use askama::Template; +use anyhow::Context; use axum::{ extract::State, - http::StatusCode, - response::{Html, IntoResponse, Response}, + response::{IntoResponse, Response}, }; use serde::Deserialize; use sqlx::PgPool; use uuid::Uuid; -#[tracing::instrument(name = "Confirming new subscriber", skip(params))] +#[tracing::instrument(name = "Confirming new subscriber", skip_all)] pub async fn confirm( State(AppState { connection_pool, .. }): State, Query(params): Query, -) -> Response { - let Ok(subscriber_id) = - get_subscriber_id_from_token(&connection_pool, ¶ms.subscription_token).await - else { - return StatusCode::INTERNAL_SERVER_ERROR.into_response(); - }; - if let Some(subscriber_id) = subscriber_id { - if confirm_subscriber(&connection_pool, &subscriber_id) +) -> Result { + let subscriber_id = get_subscriber_id_from_token(&connection_pool, ¶ms.subscription_token) + .await + .context("Could not fetch subscriber id given subscription token.")?; + if let Some(id) = subscriber_id { + confirm_subscriber(&connection_pool, &id) .await - .is_err() - { - StatusCode::INTERNAL_SERVER_ERROR.into_response() - } else { - Html(ConfirmTemplate.render().unwrap()).into_response() - } + .context("Failed to update subscriber status.")?; + let template = HtmlTemplate(ConfirmTemplate); + Ok(template.into_response()) } else { - StatusCode::UNAUTHORIZED.into_response() + Ok(not_found_html()) } } -#[tracing::instrument( - name = "Mark subscriber as confirmed", - skip(connection_pool, subscriber_id) -)] +#[tracing::instrument(name = "Mark subscriber as confirmed", skip(connection_pool))] async fn confirm_subscriber( connection_pool: &PgPool, subscriber_id: &Uuid, @@ -53,18 +44,11 @@ async fn confirm_subscriber( subscriber_id ) .execute(connection_pool) - .await - .map_err(|e| { - tracing::error!("Failed to execute query: {:?}", e); - e - })?; + .await?; Ok(()) } -#[tracing::instrument( - name = "Get subscriber_id from token", - skip(connection, subscription_token) -)] +#[tracing::instrument(name = "Get subscriber id from token", skip(connection))] async fn get_subscriber_id_from_token( connection: &PgPool, subscription_token: &str, @@ -74,11 +58,7 @@ async fn get_subscriber_id_from_token( subscription_token ) .fetch_optional(connection) - .await - .map_err(|e| { - tracing::error!("Failed to execute query: {:?}", e); - e - })?; + .await?; Ok(saved.map(|r| r.subscriber_id)) } diff --git a/src/routes/unsubscribe.rs b/src/routes/unsubscribe.rs index e098b57..b33a99a 100644 --- a/src/routes/unsubscribe.rs +++ b/src/routes/unsubscribe.rs @@ -31,6 +31,10 @@ pub struct UnsubFormData { email: String, } +#[tracing::instrument( + name = "Removing subscriber from database", + skip(connection_pool, email_client, base_url) +)] pub async fn post_unsubscribe( State(AppState { connection_pool, @@ -54,7 +58,7 @@ pub async fn post_unsubscribe( Ok(Html(template.render().unwrap()).into_response()) } -#[tracing::instrument(name = "Fetching unsubscribe token from the database", skip_all)] +#[tracing::instrument(name = "Fetching unsubscribe token from database", skip_all)] async fn fetch_unsubscribe_token( connection_pool: &PgPool, subscriber_email: &SubscriberEmail, @@ -69,7 +73,7 @@ async fn fetch_unsubscribe_token( Ok(r.and_then(|r| r.unsubscribe_token)) } -#[tracing::instrument(name = "Send an unsubscribe confirmation email", skip_all)] +#[tracing::instrument(name = "Send an confirmation email", skip_all)] pub async fn send_unsubscribe_email( email_client: &EmailClient, subscriber_email: &SubscriberEmail, @@ -102,7 +106,7 @@ If you did not request this, you can safely ignore this email."#, .await } -#[tracing::instrument(name = "Removing user from database if he exists", skip_all)] +#[tracing::instrument(name = "Removing user from database", skip(connection_pool))] pub async fn unsubscribe_confirm( Query(UnsubQueryParams { token }): Query, State(AppState { diff --git a/src/startup.rs b/src/startup.rs index 04f0437..3ab257d 100644 --- a/src/startup.rs +++ b/src/startup.rs @@ -177,7 +177,7 @@ pub async fn favicon() -> Response { .unwrap() } Err(e) => { - tracing::error!("Error while reading favicon.ico: {}", e); + tracing::error!(error = %e, "Error while reading favicon.ico."); StatusCode::NOT_FOUND.into_response() } } diff --git a/tests/api/subscriptions_confirm.rs b/tests/api/subscriptions_confirm.rs index 0af840d..96827ba 100644 --- a/tests/api/subscriptions_confirm.rs +++ b/tests/api/subscriptions_confirm.rs @@ -3,13 +3,13 @@ use sqlx::PgPool; use wiremock::ResponseTemplate; #[sqlx::test] -async fn confirmation_links_without_token_are_rejected_with_a_400(connection_pool: PgPool) { +async fn confirmation_links_without_token_are_rejected_with_a_404(connection_pool: PgPool) { let app = TestApp::spawn(connection_pool).await; let response = reqwest::get(&format!("{}/subscriptions/confirm", &app.address)) .await .unwrap(); - assert_eq!(400, response.status().as_u16()); + assert_eq!(404, response.status().as_u16()); } #[sqlx::test]