From 7555d5d1554301ee7f75dbb82bfd17d602862ef1 Mon Sep 17 00:00:00 2001 From: Junaid Ahmed Date: Sat, 19 Sep 2026 11:46:00 +0100 Subject: [PATCH] Drop unused telemetry dimensions and share the HTTP response helpers Dimensions were always passed empty, so telemetry::emit and count collapse into one signature. lambdas/api now reuses shared::http's json/private/refused instead of redeclaring its own copies. --- CLAUDE.md | 2 +- crates/shared/src/http.rs | 34 +++++++++------ crates/shared/src/telemetry.rs | 60 +++++++++++--------------- lambdas/api/src/lib.rs | 36 +++------------- lambdas/reconcile/src/lib.rs | 2 +- lambdas/stripe-webhook/src/lib.rs | 2 +- lambdas/subscription-charge/src/lib.rs | 2 - 7 files changed, 54 insertions(+), 84 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1bc3d86..1fc1827 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -121,7 +121,7 @@ These hold on every change, whatever the task. - A handler that can answer 5xx logs it at error level with a `status` field. The `Http5xx` metric filter in `aws-cloud` reads that field, so the browse, checkout and webhook availability SLOs depend on it. -- Metrics are EMF lines through `shared::telemetry::{emit,count}` and carry no dimensions: a +- Metrics are EMF lines through `shared::telemetry::emit` and carry no dimensions: a dimension multiplies series. ## 7. Performance and memory diff --git a/crates/shared/src/http.rs b/crates/shared/src/http.rs index 1b668c4..13ae102 100644 --- a/crates/shared/src/http.rs +++ b/crates/shared/src/http.rs @@ -1,6 +1,6 @@ use chrono::{DateTime, Utc}; use lambda_http::http::Method; -use lambda_http::http::header::AUTHORIZATION; +use lambda_http::http::header::{AUTHORIZATION, CACHE_CONTROL, HeaderValue}; use lambda_http::{Body, Request, Response}; use serde::Serialize; use serde::de::DeserializeOwned; @@ -24,26 +24,32 @@ pub fn body(request: &Request) -> Result { pub fn answered(result: Result) -> Response { match result { - Ok(value) => private(200, &value), - Err(err) => { - let status = err.status_code(); - if status >= 500 { - tracing::error!(status, error = %err, "request failed"); - } else { - tracing::warn!(status, error = %err, "request rejected"); - } - - private(status, &json!({ "error": err.public_message() })) - } + Ok(value) => private(json(200, &value)), + Err(err) => private(refused(&err)), } } -fn private(status: u16, body: &T) -> Response { +pub fn refused(err: &AppError) -> Response { + let status = err.status_code(); + if status >= 500 { + tracing::error!(status, error = %err, "request failed"); + } else { + tracing::warn!(status, error = %err, "request rejected"); + } + + json(status, &json!({ "error": err.public_message() })) +} + +pub fn private(mut response: Response) -> Response { + response.headers_mut().insert(CACHE_CONTROL, HeaderValue::from_static("no-store")); + response +} + +pub fn json(status: u16, body: &T) -> Response { let payload = serde_json::to_string(body).unwrap_or_else(|_| "{}".to_string()); Response::builder() .status(status) - .header("cache-control", "no-store") .header("content-type", "application/json") .body(Body::from(payload)) .unwrap_or_else(|_| Response::new(Body::Empty)) diff --git a/crates/shared/src/telemetry.rs b/crates/shared/src/telemetry.rs index 63510b1..591d9a9 100644 --- a/crates/shared/src/telemetry.rs +++ b/crates/shared/src/telemetry.rs @@ -18,7 +18,7 @@ pub fn init_logging() { .init(); } -fn emf_line(namespace: &str, timestamp_millis: i64, values: &[(&str, f64)], dimensions: &[(&str, &str)], properties: &[(&str, &str)]) -> Value { +fn emf_line(namespace: &str, timestamp_millis: i64, values: &[(&str, f64)], properties: &[(&str, &str)]) -> Value { let mut line = Map::new(); line.insert( "_aws".into(), @@ -26,13 +26,13 @@ fn emf_line(namespace: &str, timestamp_millis: i64, values: &[(&str, f64)], dime "Timestamp": timestamp_millis, "CloudWatchMetrics": [{ "Namespace": namespace, - "Dimensions": [dimensions.iter().map(|(name, _)| *name).collect::>()], + "Dimensions": [[]], "Metrics": values.iter().map(|(name, _)| json!({ "Name": name, "Unit": "None" })).collect::>(), }], }), ); - for (name, value) in dimensions.iter().chain(properties) { + for (name, value) in properties { line.insert((*name).into(), json!(value)); } for (name, value) in values { @@ -41,12 +41,8 @@ fn emf_line(namespace: &str, timestamp_millis: i64, values: &[(&str, f64)], dime Value::Object(line) } -pub fn emit(values: &[(&str, f64)], dimensions: &[(&str, &str)], properties: &[(&str, &str)]) { - println!("{}", emf_line(&NAMESPACE, Utc::now().timestamp_millis(), values, dimensions, properties)); -} - -pub fn count(name: &str, dimensions: &[(&str, &str)], properties: &[(&str, &str)]) { - emit(&[(name, 1.0)], dimensions, properties); +pub fn emit(values: &[(&str, f64)], properties: &[(&str, &str)]) { + println!("{}", emf_line(&NAMESPACE, Utc::now().timestamp_millis(), values, properties)); } #[cfg(test)] @@ -54,37 +50,33 @@ mod tests { use super::*; #[test] - fn emf_line_declares_metrics_and_dimensions_and_carries_their_values() { + fn emf_line_declares_its_metrics_without_dimensions_beside_their_values_and_properties() { let line = emf_line( "raffle-test", 1_700_000_000_000, - &[("WebhookOutcome", 1.0)], - &[("Outcome", "Allocated")], - &[("eventId", "evt_1")], - ); - - assert_eq!(line["_aws"]["Timestamp"], 1_700_000_000_000_i64); - assert_eq!(line["_aws"]["CloudWatchMetrics"][0]["Namespace"], "raffle-test"); - assert_eq!(line["_aws"]["CloudWatchMetrics"][0]["Dimensions"], json!([["Outcome"]])); - assert_eq!(line["_aws"]["CloudWatchMetrics"][0]["Metrics"][0]["Name"], "WebhookOutcome"); - assert_eq!(line["Outcome"], "Allocated"); - assert_eq!(line["eventId"], "evt_1"); - assert_eq!(line["WebhookOutcome"], 1.0); - } - - #[test] - fn emf_line_without_dimensions_still_lists_an_empty_dimension_set() { - let line = emf_line( - "raffle-test", - 0, &[("SubscriptionsCharged", 3.0), ("SubscriptionsErrored", 0.0)], - &[], &[("raffleId", "winter")], ); + let declared = &line["_aws"]["CloudWatchMetrics"][0]; + let names: Vec<&Value> = declared["Metrics"] + .as_array() + .expect("a metric list") + .iter() + .map(|metric| &metric["Name"]) + .collect(); - assert_eq!(line["_aws"]["CloudWatchMetrics"][0]["Dimensions"], json!([[]])); - assert_eq!(line["_aws"]["CloudWatchMetrics"][0]["Metrics"].as_array().map(Vec::len), Some(2)); - assert_eq!(line["SubscriptionsCharged"], 3.0); - assert_eq!(line["raffleId"], "winter"); + assert_eq!( + line["_aws"]["Timestamp"], 1_700_000_000_000_i64, + "the timestamp CloudWatch files the line under" + ); + assert_eq!(declared["Namespace"], "raffle-test", "the namespace the metrics land in"); + assert_eq!(declared["Dimensions"], json!([[]]), "one empty dimension set, so no series multiplies"); + assert_eq!(names, ["SubscriptionsCharged", "SubscriptionsErrored"], "every value is declared as a metric"); + assert_eq!( + (&line["SubscriptionsCharged"], &line["SubscriptionsErrored"]), + (&json!(3.0), &json!(0.0)), + "each declared metric carries its value at the top level" + ); + assert_eq!(line["raffleId"], "winter", "a property rides along undeclared"); } } diff --git a/lambdas/api/src/lib.rs b/lambdas/api/src/lib.rs index beebfa4..19586ea 100644 --- a/lambdas/api/src/lib.rs +++ b/lambdas/api/src/lib.rs @@ -1,10 +1,10 @@ use chrono::{DateTime, NaiveDate, Utc}; use lambda_http::http::Method; -use lambda_http::http::header::{CACHE_CONTROL, HeaderValue}; use lambda_http::{Body, Request, Response}; use serde::{Deserialize, Serialize}; use shared::entrant::{Address, Entrant, GiftAidDeclaration, MarketingConsent}; use shared::error::AppError; +use shared::http::{body, json, private, refused}; use shared::order::{Order, OrderStatus, validate_purchase}; use shared::raffle::{Prize, Raffle, RaffleStatus}; use shared::random; @@ -163,19 +163,9 @@ impl Api { Self { repo, gateway } } + #[tracing::instrument(skip_all, fields(method = %request.method(), path = request.uri().path()))] pub async fn handle(&self, request: Request) -> Response { - self.route(&request, Utc::now()).await.unwrap_or_else(|err| { - let status = err.status_code(); - let method = request.method().as_str(); - let path = request.uri().path(); - - if status >= 500 { - tracing::error!(status, method, path, error = %err, "request failed"); - } else { - tracing::warn!(status, method, path, error = %err, "request rejected"); - } - json(err.status_code(), &serde_json::json!({ "error": err.public_message() })) - }) + self.route(&request, Utc::now()).await.unwrap_or_else(|err| refused(&err)) } async fn route(&self, request: &Request, now: DateTime) -> Result, AppError> { @@ -183,15 +173,8 @@ impl Api { match (request.method(), segments.as_slice()) { (&Method::GET, ["raffles", "current"]) => Ok(json(200, &self.current(now).await?)), - (&Method::GET, ["orders", order_id]) => { - let mut response = json(200, &self.order(order_id).await?); - response.headers_mut().insert(CACHE_CONTROL, HeaderValue::from_static("no-store")); - Ok(response) - } - (&Method::POST, ["raffles", raffle_id, "orders"]) => { - let body = serde_json::from_slice(request.body().as_ref()).map_err(|err| AppError::BadRequest(format!("invalid body: {err}")))?; - Ok(json(201, &self.create_order(raffle_id, body, now).await?)) - } + (&Method::GET, ["orders", order_id]) => Ok(private(json(200, &self.order(order_id).await?))), + (&Method::POST, ["raffles", raffle_id, "orders"]) => Ok(json(201, &self.create_order(raffle_id, body(request)?, now).await?)), _ => Err(AppError::NotFound("route".into())), } } @@ -322,15 +305,6 @@ fn payment_intent_request(order: &Order, raffle: &Raffle, entrant: &Entrant) -> } } -fn json(status: u16, body: &T) -> Response { - let payload = serde_json::to_string(body).unwrap_or_else(|_| "{}".to_string()); - Response::builder() - .status(status) - .header("content-type", "application/json") - .body(Body::from(payload)) - .unwrap_or_else(|_| Response::new(Body::Empty)) -} - #[cfg(test)] mod tests { use super::*; diff --git a/lambdas/reconcile/src/lib.rs b/lambdas/reconcile/src/lib.rs index 9b550e0..d9c619e 100644 --- a/lambdas/reconcile/src/lib.rs +++ b/lambdas/reconcile/src/lib.rs @@ -181,7 +181,7 @@ pub async fn run(repo: &DynamoRepo, gateway: &G, now: DateTim for found in &report.violations { tracing::error!(check = found.check, subject = %found.subject, detail = %found.detail, "integrity violation"); } - telemetry::emit(&[("IntegrityViolations", report.violations.len() as f64)], &[], &[]); + telemetry::emit(&[("IntegrityViolations", report.violations.len() as f64)], &[]); tracing::info!( report.raffles_checked, report.entries_checked, diff --git a/lambdas/stripe-webhook/src/lib.rs b/lambdas/stripe-webhook/src/lib.rs index 33e67db..9b74f42 100644 --- a/lambdas/stripe-webhook/src/lib.rs +++ b/lambdas/stripe-webhook/src/lib.rs @@ -151,7 +151,7 @@ impl Webhook { }; if let Some(metric) = outcome.metric() { - telemetry::count(metric, &[], &[("eventId", &event_id)]); + telemetry::emit(&[(metric, 1.0)], &[("eventId", &event_id)]); } tracing::info!(event_id = %event_id, kind = %kind, outcome = ?outcome, "webhook processed"); respond(200, format!("{outcome:?}")) diff --git a/lambdas/subscription-charge/src/lib.rs b/lambdas/subscription-charge/src/lib.rs index f3f834e..c3c1d5e 100644 --- a/lambdas/subscription-charge/src/lib.rs +++ b/lambdas/subscription-charge/src/lib.rs @@ -38,7 +38,6 @@ pub async fn run(repo: &DynamoRepo, gateway: &G, now: DateTim for raffle in raffles.iter().filter(|raffle| raffle.needs_subscription_charge(now)) { telemetry::emit( &[("SubscriptionChargeLagHours", charge_lag_hours(raffle, now))], - &[], &[("raffleId", &raffle.raffle_id)], ); runs.push(charge_raffle(repo, gateway, raffle, now).await?); @@ -71,7 +70,6 @@ async fn charge_raffle(repo: &DynamoRepo, gateway: &G, raffle ("SubscriptionsDeclined", f64::from(run.declined)), ("SubscriptionsErrored", f64::from(run.errored)), ], - &[], &[("raffleId", &raffle.raffle_id)], ); tracing::info!(raffle_id = %run.raffle_id, run.charged, run.declined, run.skipped, run.errored, "subscription charge run finished");