From 53933356542768200289cca518f23e3494675c80 Mon Sep 17 00:00:00 2001 From: Ethan Donowitz Date: Wed, 5 Jan 2022 23:04:23 +0000 Subject: [PATCH 1/4] feat: add Tokenserver metrics Closes #1108 --- src/server/metrics.rs | 22 +++++-- src/server/mod.rs | 8 ++- src/tokenserver/db/models.rs | 18 +++++- src/tokenserver/db/pool.rs | 14 ++-- src/tokenserver/db/results.rs | 1 + src/tokenserver/extractors.rs | 22 ++++++- src/tokenserver/handlers.rs | 12 +++- src/tokenserver/logging.rs | 74 +++++++++++++++++++++ src/tokenserver/mod.rs | 117 +++++++++++++++++++++++++++++++++- src/web/extractors.rs | 10 +-- 10 files changed, 271 insertions(+), 27 deletions(-) create mode 100644 src/tokenserver/logging.rs diff --git a/src/server/metrics.rs b/src/server/metrics.rs index 742dbd2cb4..50fae4b87b 100644 --- a/src/server/metrics.rs +++ b/src/server/metrics.rs @@ -1,10 +1,17 @@ use std::net::UdpSocket; use std::time::Instant; -use actix_web::{error::ErrorInternalServerError, web::Data, Error, HttpRequest}; +use actix_web::{ + error::ErrorInternalServerError, + dev::Payload, + web::Data, + Error, FromRequest, HttpRequest, +}; use cadence::{ BufferedUdpMetricSink, Counted, Metric, NopMetricSink, QueuingMetricSink, StatsdClient, Timed, }; +use futures::future; +use futures::future::Ready; use crate::error::ApiError; use crate::server::ServerState; @@ -54,12 +61,17 @@ impl Drop for Metrics { } } -impl From<&HttpRequest> for Metrics { - fn from(req: &HttpRequest) -> Self { +impl FromRequest for Metrics { + type Config = (); + type Error = Error; + type Future = Ready>; + + fn from_request(req: &HttpRequest, _: &mut Payload) -> Self::Future { let exts = req.extensions(); let def_tags = Tags::from(req.head()); let tags = exts.get::().unwrap_or(&def_tags); - Metrics { + + future::ok(Metrics { client: match req.app_data::>() { Some(v) => Some(*v.metrics.clone()), None => { @@ -69,7 +81,7 @@ impl From<&HttpRequest> for Metrics { }, tags: Some(tags.clone()), timer: None, - } + }) } } diff --git a/src/server/mod.rs b/src/server/mod.rs index f8781bd233..c92bc3c55e 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -73,6 +73,7 @@ macro_rules! build_app { .wrap(ErrorHandlers::new().handler(StatusCode::NOT_FOUND, ApiError::render_404)) // These are our wrappers .wrap(middleware::weave::WeaveTimestamp::new()) + .wrap(tokenserver::logging::LoggingWrapper::new()) .wrap(middleware::sentry::SentryWrapper::default()) .wrap(middleware::rejectua::RejectUA::default()) .wrap($cors) @@ -176,6 +177,7 @@ macro_rules! build_app_without_syncstorage { .wrap(ErrorHandlers::new().handler(StatusCode::NOT_FOUND, ApiError::render_404)) // These are our wrappers .wrap(middleware::sentry::SentryWrapper::default()) + .wrap(tokenserver::logging::LoggingWrapper::new()) .wrap(middleware::rejectua::RejectUA::default()) // Followed by the "official middleware" so they run first. // actix is getting increasingly tighter about CORS headers. Our server is @@ -239,6 +241,7 @@ impl Server { let tokenserver_state = if settings.tokenserver.enabled { Some(tokenserver::ServerState::from_settings( &settings.tokenserver, + metrics.clone(), )?) } else { None @@ -283,8 +286,9 @@ impl Server { let settings_copy = settings.clone(); let host = settings.host.clone(); let port = settings.port; - let secrets = Arc::new(settings.master_secret); - let tokenserver_state = tokenserver::ServerState::from_settings(&settings.tokenserver)?; + let secrets = Arc::new(settings.master_secret.clone()); + let metrics = metrics::metrics_from_opts(&settings)?; + let tokenserver_state = tokenserver::ServerState::from_settings(&settings.tokenserver, metrics)?; let server = HttpServer::new(move || { build_app_without_syncstorage!( Some(tokenserver_state.clone()), diff --git a/src/tokenserver/db/models.rs b/src/tokenserver/db/models.rs index 2912b25d24..33908df14f 100644 --- a/src/tokenserver/db/models.rs +++ b/src/tokenserver/db/models.rs @@ -19,6 +19,7 @@ use std::{ use super::{params, results}; use crate::db::error::{DbError, DbErrorKind}; use crate::error::ApiError; +use crate::server::metrics::Metrics; use crate::sync_db_method; /// The maximum possible generation number. Used as a tombstone to mark users that have been @@ -39,6 +40,7 @@ pub struct TokenserverDb { /// conn. structs are !Sync (Arc requires both for Send). See the Send impl /// below. pub(super) inner: Arc, + metrics: Metrics, } /// Despite the db conn structs being !Sync (see Arc above) we @@ -61,7 +63,7 @@ impl TokenserverDb { // get IDs from records created during other requests. const LAST_INSERT_ID_QUERY: &'static str = "SELECT LAST_INSERT_ID() AS id"; - pub fn new(conn: Conn) -> Self { + pub fn new(conn: Conn, metrics: &Metrics) -> Self { let inner = DbInner { #[cfg(not(test))] conn, @@ -71,6 +73,7 @@ impl TokenserverDb { Self { inner: Arc::new(inner), + metrics: metrics.clone() } } @@ -211,6 +214,9 @@ impl TokenserverDb { "#; const DEFAULT_CAPACITY_RELEASE_RATE: f32 = 0.1; + let mut metrics = self.metrics.clone(); + metrics.start_timer("tokenserver.storage.get_best_node", None); + // We may have to retry the query if we need to release more capacity. This loop allows // a maximum of five retries before bailing out. for _ in 0..5 { @@ -310,6 +316,7 @@ impl TokenserverDb { keys_changed_at: params.keys_changed_at, created_at: allocate_user_result.created_at, replaced_at: None, + first_seen_at: allocate_user_result.created_at, old_client_states: vec![], }) } else { @@ -342,6 +349,8 @@ impl TokenserverDb { } } + let first_seen_at = raw_users[raw_users.len() - 1].created_at; + match (raw_user.replaced_at, raw_user.node) { // If the most up-to-date user is marked as replaced or does not have a node // assignment, allocate a new user. Note that, if the current user is marked @@ -371,6 +380,7 @@ impl TokenserverDb { keys_changed_at: raw_user.keys_changed_at, created_at: allocate_user_result.created_at, replaced_at: None, + first_seen_at, old_client_states, }) } @@ -385,6 +395,7 @@ impl TokenserverDb { keys_changed_at: raw_user.keys_changed_at, created_at: raw_user.created_at, replaced_at: None, + first_seen_at, old_client_states, }), // The most up-to-date user doesn't have a node and is retired. @@ -395,6 +406,9 @@ impl TokenserverDb { /// Creates a new user and assigns them to a node. fn allocate_user_sync(&self, params: params::AllocateUser) -> DbResult { + let mut metrics = self.metrics.clone(); + metrics.start_timer("tokenserver.storage.allocate_user", None); + // Get the least-loaded node let node = self.get_best_node_sync(params::GetBestNode { service_id: params.service_id, @@ -1914,6 +1928,6 @@ mod tests { let tokenserver_settings = test_settings().tokenserver; let use_test_transactions = true; - TokenserverPool::new(&tokenserver_settings, use_test_transactions) + TokenserverPool::new(&tokenserver_settings, &Metrics::noop(), use_test_transactions) } } diff --git a/src/tokenserver/db/pool.rs b/src/tokenserver/db/pool.rs index 3d5626fe24..348caf4f88 100644 --- a/src/tokenserver/db/pool.rs +++ b/src/tokenserver/db/pool.rs @@ -11,6 +11,7 @@ use std::time::Duration; use super::models::{Db, DbResult, TokenserverDb}; use crate::db::{error::DbError, DbErrorKind}; +use crate::server::metrics::Metrics; use crate::tokenserver::settings::Settings; #[cfg(test)] @@ -37,10 +38,11 @@ pub fn run_embedded_migrations(database_url: &str) -> DbResult<()> { pub struct TokenserverPool { /// Pool of db connections inner: Pool>, + metrics: Metrics, } impl TokenserverPool { - pub fn new(settings: &Settings, _use_test_transactions: bool) -> DbResult { + pub fn new(settings: &Settings, metrics: &Metrics, _use_test_transactions: bool) -> DbResult { run_embedded_migrations(&settings.database_url)?; let manager = ConnectionManager::::new(settings.database_url.clone()); @@ -60,13 +62,14 @@ impl TokenserverPool { Ok(Self { inner: builder.build(manager)?, + metrics: metrics.clone() }) } pub fn get_sync(&self) -> Result { let conn = self.inner.get().map_err(DbError::from)?; - Ok(TokenserverDb::new(conn)) + Ok(TokenserverDb::new(conn, &self.metrics)) } #[cfg(test)] @@ -74,7 +77,7 @@ impl TokenserverPool { let pool = self.clone(); let conn = block(move || pool.inner.get().map_err(DbError::from)).await?; - Ok(TokenserverDb::new(conn)) + Ok(TokenserverDb::new(conn, &self.metrics)) } } @@ -92,10 +95,13 @@ impl From> for DbError { #[async_trait] impl DbPool for TokenserverPool { async fn get(&self) -> Result, DbError> { + let mut metrics = self.metrics.clone(); + metrics.start_timer("tokenserver.storage.get_pool", None); + let pool = self.clone(); let conn = block(move || pool.inner.get().map_err(DbError::from)).await?; - Ok(Box::new(TokenserverDb::new(conn)) as Box) + Ok(Box::new(TokenserverDb::new(conn, &self.metrics)) as Box) } fn box_clone(&self) -> Box { diff --git a/src/tokenserver/db/results.rs b/src/tokenserver/db/results.rs index 7bfe0d9b0f..370b8779d0 100644 --- a/src/tokenserver/db/results.rs +++ b/src/tokenserver/db/results.rs @@ -45,6 +45,7 @@ pub struct GetOrCreateUser { pub keys_changed_at: Option, pub created_at: i64, pub replaced_at: Option, + pub first_seen_at: i64, pub old_client_states: Vec, } diff --git a/src/tokenserver/extractors.rs b/src/tokenserver/extractors.rs index 0f18193809..ef659ef024 100644 --- a/src/tokenserver/extractors.rs +++ b/src/tokenserver/extractors.rs @@ -22,9 +22,9 @@ use sha2::Sha256; use super::db::{models::Db, params, pool::DbPool, results}; use super::error::{ErrorLocation, TokenserverError}; use super::support::TokenData; -use super::NodeType; -use super::ServerState; +use super::{LogItemsMutator, NodeType, ServerState}; use crate::settings::Secrets; +use crate::server::metrics::Metrics; lazy_static! { static ref CLIENT_STATE_REGEX: Regex = Regex::new("^[a-zA-Z0-9._-]{1,32}$").unwrap(); @@ -143,6 +143,7 @@ impl FromRequest for TokenserverRequest { let req = req.clone(); Box::pin(async move { + let mut log_items_mutator = LogItemsMutator::from(&req); let token_data = TokenData::extract(&req).await?; // XXX: Tokenserver state will no longer be an Option once the Tokenserver @@ -154,8 +155,10 @@ impl FromRequest for TokenserverRequest { let fxa_uid = token_data.user; let hashed_fxa_uid = { let hashed_fxa_uid_full = fxa_metrics_hash(&fxa_uid, fxa_metrics_hash_secret); + log_items_mutator.insert("uid".to_owned(), hashed_fxa_uid_full.clone()); hashed_fxa_uid_full[0..32].to_owned() }; + log_items_mutator.insert("metrics_uid".to_owned(), hashed_fxa_uid.clone()); let hashed_device_id = { let device_id = "none".to_string(); hash_device_id(&hashed_fxa_uid, &device_id, fxa_metrics_hash_secret) @@ -210,6 +213,8 @@ impl FromRequest for TokenserverRequest { capacity_release_rate: state.node_capacity_release_rate, }) .await?; + log_items_mutator.insert("first_seen_at".to_owned(), user.first_seen_at.to_string()); + let duration = { let params = Query::::extract(&req).await?; @@ -324,6 +329,9 @@ impl FromRequest for TokenData { let state = get_server_state(&req)?.as_ref().as_ref().unwrap(); let oauth_verifier = state.oauth_verifier.clone(); + let mut metrics = Metrics::extract(&req).await?; + metrics.start_timer("tokenserver.oauth_token_verification", None); + web::block(move || oauth_verifier.verify_token(auth.token())) .await .map_err(TokenserverError::from) @@ -463,7 +471,8 @@ mod tests { use lazy_static::lazy_static; use serde_json; - use crate::settings::{Secrets, ServerLimits}; + use crate::server::metrics; + use crate::settings::{Secrets, ServerLimits, Settings}; use crate::tokenserver::{ db::mock::MockDbPool as MockTokenserverPool, MockOAuthVerifier, ServerState, }; @@ -830,6 +839,7 @@ mod tests { keys_changed_at: Some(1234), replaced_at: None, created_at: 1234, + first_seen_at: 1234, old_client_states: vec![], }, fxa_uid: "test".to_owned(), @@ -862,6 +872,7 @@ mod tests { node: "node".to_owned(), keys_changed_at: Some(1234), created_at: 1234, + first_seen_at: 1234, replaced_at: None, old_client_states: vec![], }, @@ -894,6 +905,7 @@ mod tests { node: "node".to_owned(), keys_changed_at: Some(1234), created_at: 1234, + first_seen_at: 1234, replaced_at: None, old_client_states: vec![], }, @@ -927,6 +939,7 @@ mod tests { node: "node".to_owned(), keys_changed_at: Some(1234), created_at: 1234, + first_seen_at: 1234, replaced_at: None, old_client_states: vec!["626262".to_owned()], }, @@ -960,6 +973,7 @@ mod tests { node: "node".to_owned(), keys_changed_at: Some(1234), created_at: 1234, + first_seen_at: 1234, replaced_at: None, old_client_states: vec![], }, @@ -993,6 +1007,7 @@ mod tests { node: "node".to_owned(), keys_changed_at: Some(1234), created_at: 1234, + first_seen_at: 1234, replaced_at: None, old_client_states: vec![], }, @@ -1028,6 +1043,7 @@ mod tests { node_capacity_release_rate: None, node_type: NodeType::default(), service_id: None, + metrics: Box::new(metrics::metrics_from_opts(&Settings::default()).unwrap()), } } } diff --git a/src/tokenserver/handlers.rs b/src/tokenserver/handlers.rs index 2fc3fa608f..09a4e6b09b 100644 --- a/src/tokenserver/handlers.rs +++ b/src/tokenserver/handlers.rs @@ -14,6 +14,7 @@ use super::error::TokenserverError; use super::extractors::TokenserverRequest; use super::support::{self, Tokenlib}; use super::NodeType; +use crate::server::metrics::Metrics; use crate::tokenserver::support::MakeTokenPlaintext; #[derive(Debug, Serialize)] @@ -31,6 +32,7 @@ pub struct TokenserverResult { pub async fn get_tokenserver_result( req: TokenserverRequest, db: Box, + mut metrics: Metrics, ) -> Result { let updates = update_user(&req, db).await?; @@ -41,14 +43,18 @@ pub async fn get_tokenserver_result( // Derive the node-specific secret that will be used to derive the token and secret to be // returned to the client - let secrets = + let secrets = { + metrics.start_timer("tokenserver.node_secret_derivation", None); + support::derive_node_secrets(vec![&hex::encode(req.shared_secret)], &req.user.node) .map_err(|_| { error!("⚠️ Failed to derive node secret"); TokenserverError::internal_error() - })?; - + })? + }; + + metrics.start_timer("tokenserver.token_creation", None); // Get the token and secret Tokenlib::get_token_and_derived_secret(token_plaintext, &secrets[secrets.len() - 1])? }; diff --git a/src/tokenserver/logging.rs b/src/tokenserver/logging.rs new file mode 100644 index 0000000000..c7006c7d56 --- /dev/null +++ b/src/tokenserver/logging.rs @@ -0,0 +1,74 @@ +use std::task::Context; +use std::{cell::RefCell, rc::Rc}; + +use actix_web::{ + dev::{Service, ServiceRequest, ServiceResponse, Transform}, + Error, HttpMessage, +}; +use futures::future::{self, LocalBoxFuture, TryFutureExt}; +use std::task::Poll; + +use super::LogItems; + +#[derive(Default)] +pub struct LoggingWrapper; + +impl LoggingWrapper { + pub fn new() -> Self { + LoggingWrapper::default() + } +} + +impl Transform for LoggingWrapper +where + S: Service, Error = Error> + 'static, + S::Future: 'static, + B: 'static, +{ + type Request = ServiceRequest; + type Response = ServiceResponse; + type Error = Error; + type InitError = (); + type Transform = LoggingWrapperMiddleware; + type Future = LocalBoxFuture<'static, Result>; + + fn new_transform(&self, service: S) -> Self::Future { + Box::pin(future::ok(LoggingWrapperMiddleware { + service: Rc::new(RefCell::new(service)), + })) + } +} + +#[derive(Debug)] +pub struct LoggingWrapperMiddleware { + service: Rc>, +} + +impl Service for LoggingWrapperMiddleware +where + S: Service, Error = Error> + 'static, + S::Future: 'static, + B: 'static, +{ + type Request = ServiceRequest; + type Response = ServiceResponse; + type Error = Error; + type Future = LocalBoxFuture<'static, Result>; + + fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { + self.service.poll_ready(cx) + } + + fn call(&mut self, sreq: ServiceRequest) -> Self::Future { + let items = LogItems::from(sreq.head()); + sreq.extensions_mut().insert(items); + + Box::pin(self.service.call(sreq).and_then(move |sresp| { + if let Some(items) = sresp.request().extensions().get::() { + info!("{}", items); + } + + future::ok(sresp) + })) + } +} diff --git a/src/tokenserver/mod.rs b/src/tokenserver/mod.rs index 6b839b5ce7..84629968b9 100644 --- a/src/tokenserver/mod.rs +++ b/src/tokenserver/mod.rs @@ -2,19 +2,32 @@ pub mod db; pub mod error; pub mod extractors; pub mod handlers; +pub mod logging; pub mod settings; pub mod support; pub use self::support::{MockOAuthVerifier, OAuthVerifier, TestModeOAuthVerifier, VerifyToken}; +use actix_web::{ + dev::RequestHead, + http::header::USER_AGENT, + HttpRequest, +}; +use cadence::StatsdClient; use db::{ params, pool::{DbPool, TokenserverPool}, }; -use serde::{Deserialize, Serialize}; +use serde::{ + ser::{SerializeMap, Serializer}, + Deserialize, Serialize, +}; use settings::Settings; use crate::error::ApiError; +use crate::server::{metrics::Metrics, user_agent}; + +use std::{collections::HashMap, fmt}; #[derive(Clone)] pub struct ServerState { @@ -25,10 +38,11 @@ pub struct ServerState { pub node_capacity_release_rate: Option, pub node_type: NodeType, pub service_id: Option, + pub metrics: Box } impl ServerState { - pub fn from_settings(settings: &Settings) -> Result { + pub fn from_settings(settings: &Settings, metrics: StatsdClient) -> Result { let oauth_verifier: Box = if settings.test_mode_enabled { #[cfg(feature = "tokenserver_test_mode")] let oauth_verifier = Box::new(TestModeOAuthVerifier); @@ -46,7 +60,7 @@ impl ServerState { }; let use_test_transactions = false; - TokenserverPool::new(settings, use_test_transactions) + TokenserverPool::new(settings, &Metrics::from(&metrics), use_test_transactions) .map(|db_pool| { let service_id = db_pool .get_sync() @@ -65,6 +79,7 @@ impl ServerState { db_pool: Box::new(db_pool), node_capacity_release_rate: settings.node_capacity_release_rate, node_type: settings.node_type, + metrics: Box::new(metrics), service_id, } }) @@ -85,3 +100,99 @@ impl Default for NodeType { Self::Spanner } } + +#[derive(Clone, Debug)] +struct LogItems(HashMap); + +impl LogItems { + fn new() -> Self { + Self(HashMap::new()) + } + + pub fn insert(&mut self, k: String, v: String) { + self.0.insert(k, v); + } + + fn insert_if_not_empty(&mut self, k: &str, v: &str) { + if !v.is_empty() { + self.0.insert(k.to_owned(), v.to_owned()); + } + } +} + +impl From<&RequestHead> for LogItems { + fn from(req_head: &RequestHead) -> Self { + let mut items = Self::new(); + if let Some(ua) = req_head.headers().get(USER_AGENT) { + if let Ok(uas) = ua.to_str() { + let (ua_result, metrics_os, metrics_browser) = user_agent::parse_user_agent(uas); + items.insert_if_not_empty("ua.os.family", metrics_os); + items.insert_if_not_empty("ua.browser.family", metrics_browser); + items.insert_if_not_empty("ua.name", ua_result.name); + items.insert_if_not_empty("ua.os.ver", &ua_result.os_version.to_owned()); + items.insert_if_not_empty("ua.browser.ver", ua_result.version); + items.insert_if_not_empty("ua", ua_result.version); + } + } + items.insert("uri.method".to_owned(), req_head.method.to_string()); + items.insert("uri.path".to_owned(), req_head.uri.to_string()); + + items + } +} + +impl<'a> IntoIterator for &'a LogItems { + type Item = (&'a String, &'a String); + type IntoIter = <&'a HashMap as IntoIterator>::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.0.iter() + } +} + +impl Serialize for LogItems { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + let mut seq = serializer.serialize_map(Some(self.0.len()))?; + for item in self { + if !item.1.is_empty() { + seq.serialize_entry(&item.0, &item.1)?; + } + } + seq.end() + } +} + +impl fmt::Display for LogItems { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{}", + serde_json::to_string(&self).map_err(|_| fmt::Error)? + ) + } +} + +struct LogItemsMutator<'a>(&'a HttpRequest); + +impl<'a> LogItemsMutator<'a> { + pub fn insert(&mut self, k: String, v: String) { + let mut exts = self.0.extensions_mut(); + + if !exts.contains::() { + exts.insert(LogItems::from(self.0.head())); + } + + let log_items = exts.get_mut::().unwrap(); + + log_items.insert(k, v); + } +} + +impl<'a> From<&'a HttpRequest> for LogItemsMutator<'a> { + fn from(req: &'a HttpRequest) -> Self { + Self(req) + } +} diff --git a/src/web/extractors.rs b/src/web/extractors.rs index 7f36d07f4c..15b99b356f 100644 --- a/src/web/extractors.rs +++ b/src/web/extractors.rs @@ -633,7 +633,7 @@ impl FromRequest for MetaRequest { Ok(MetaRequest { user_id, - metrics: metrics::Metrics::from(&req), + metrics: metrics::Metrics::extract(&req).await?, }) } .boxed_local() @@ -695,7 +695,7 @@ impl FromRequest for CollectionRequest { user_id, query, reply, - metrics: metrics::Metrics::from(&req), + metrics: metrics::Metrics::extract(&req).await?, }) } .boxed_local() @@ -791,7 +791,7 @@ impl FromRequest for CollectionPostRequest { query, bsos, batch: batch.opt, - metrics: metrics::Metrics::from(&req), + metrics: metrics::Metrics::extract(&req).await?, quota_enabled: state.quota_enabled, }) }) @@ -832,7 +832,7 @@ impl FromRequest for BsoRequest { user_id, query, bso: bso.bso, - metrics: metrics::Metrics::from(&req), + metrics: metrics::Metrics::extract(&req).await?, }) }) } @@ -856,11 +856,11 @@ impl FromRequest for BsoPutRequest { type Future = LocalBoxFuture<'static, Result>; fn from_request(req: &HttpRequest, payload: &mut Payload) -> Self::Future { - let metrics = metrics::Metrics::from(req); let req = req.clone(); let mut payload = payload.take(); async move { + let metrics = metrics::Metrics::extract(&req).await?; let (user_id, collection, query, bso, body) = <( HawkIdentifier, From 9c4907de099dd6880988927298837c17894e16b3 Mon Sep 17 00:00:00 2001 From: Ethan Donowitz Date: Wed, 5 Jan 2022 23:08:18 +0000 Subject: [PATCH 2/4] fmt --- src/server/metrics.rs | 5 +---- src/server/mod.rs | 3 ++- src/tokenserver/db/models.rs | 8 ++++++-- src/tokenserver/db/pool.rs | 8 ++++++-- src/tokenserver/extractors.rs | 4 ++-- src/tokenserver/handlers.rs | 2 +- src/tokenserver/mod.rs | 8 ++------ 7 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/server/metrics.rs b/src/server/metrics.rs index 50fae4b87b..99ce8cefd9 100644 --- a/src/server/metrics.rs +++ b/src/server/metrics.rs @@ -2,10 +2,7 @@ use std::net::UdpSocket; use std::time::Instant; use actix_web::{ - error::ErrorInternalServerError, - dev::Payload, - web::Data, - Error, FromRequest, HttpRequest, + dev::Payload, error::ErrorInternalServerError, web::Data, Error, FromRequest, HttpRequest, }; use cadence::{ BufferedUdpMetricSink, Counted, Metric, NopMetricSink, QueuingMetricSink, StatsdClient, Timed, diff --git a/src/server/mod.rs b/src/server/mod.rs index c92bc3c55e..be62aacc72 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -288,7 +288,8 @@ impl Server { let port = settings.port; let secrets = Arc::new(settings.master_secret.clone()); let metrics = metrics::metrics_from_opts(&settings)?; - let tokenserver_state = tokenserver::ServerState::from_settings(&settings.tokenserver, metrics)?; + let tokenserver_state = + tokenserver::ServerState::from_settings(&settings.tokenserver, metrics)?; let server = HttpServer::new(move || { build_app_without_syncstorage!( Some(tokenserver_state.clone()), diff --git a/src/tokenserver/db/models.rs b/src/tokenserver/db/models.rs index 33908df14f..8dcb8fe101 100644 --- a/src/tokenserver/db/models.rs +++ b/src/tokenserver/db/models.rs @@ -73,7 +73,7 @@ impl TokenserverDb { Self { inner: Arc::new(inner), - metrics: metrics.clone() + metrics: metrics.clone(), } } @@ -1928,6 +1928,10 @@ mod tests { let tokenserver_settings = test_settings().tokenserver; let use_test_transactions = true; - TokenserverPool::new(&tokenserver_settings, &Metrics::noop(), use_test_transactions) + TokenserverPool::new( + &tokenserver_settings, + &Metrics::noop(), + use_test_transactions, + ) } } diff --git a/src/tokenserver/db/pool.rs b/src/tokenserver/db/pool.rs index 348caf4f88..50ca2aa76d 100644 --- a/src/tokenserver/db/pool.rs +++ b/src/tokenserver/db/pool.rs @@ -42,7 +42,11 @@ pub struct TokenserverPool { } impl TokenserverPool { - pub fn new(settings: &Settings, metrics: &Metrics, _use_test_transactions: bool) -> DbResult { + pub fn new( + settings: &Settings, + metrics: &Metrics, + _use_test_transactions: bool, + ) -> DbResult { run_embedded_migrations(&settings.database_url)?; let manager = ConnectionManager::::new(settings.database_url.clone()); @@ -62,7 +66,7 @@ impl TokenserverPool { Ok(Self { inner: builder.build(manager)?, - metrics: metrics.clone() + metrics: metrics.clone(), }) } diff --git a/src/tokenserver/extractors.rs b/src/tokenserver/extractors.rs index ef659ef024..2be4d3fd84 100644 --- a/src/tokenserver/extractors.rs +++ b/src/tokenserver/extractors.rs @@ -23,8 +23,8 @@ use super::db::{models::Db, params, pool::DbPool, results}; use super::error::{ErrorLocation, TokenserverError}; use super::support::TokenData; use super::{LogItemsMutator, NodeType, ServerState}; -use crate::settings::Secrets; use crate::server::metrics::Metrics; +use crate::settings::Secrets; lazy_static! { static ref CLIENT_STATE_REGEX: Regex = Regex::new("^[a-zA-Z0-9._-]{1,32}$").unwrap(); @@ -143,7 +143,7 @@ impl FromRequest for TokenserverRequest { let req = req.clone(); Box::pin(async move { - let mut log_items_mutator = LogItemsMutator::from(&req); + let mut log_items_mutator = LogItemsMutator::from(&req); let token_data = TokenData::extract(&req).await?; // XXX: Tokenserver state will no longer be an Option once the Tokenserver diff --git a/src/tokenserver/handlers.rs b/src/tokenserver/handlers.rs index 09a4e6b09b..e305682713 100644 --- a/src/tokenserver/handlers.rs +++ b/src/tokenserver/handlers.rs @@ -53,7 +53,7 @@ pub async fn get_tokenserver_result( TokenserverError::internal_error() })? }; - + metrics.start_timer("tokenserver.token_creation", None); // Get the token and secret Tokenlib::get_token_and_derived_secret(token_plaintext, &secrets[secrets.len() - 1])? diff --git a/src/tokenserver/mod.rs b/src/tokenserver/mod.rs index 84629968b9..eefc82a041 100644 --- a/src/tokenserver/mod.rs +++ b/src/tokenserver/mod.rs @@ -8,11 +8,7 @@ pub mod support; pub use self::support::{MockOAuthVerifier, OAuthVerifier, TestModeOAuthVerifier, VerifyToken}; -use actix_web::{ - dev::RequestHead, - http::header::USER_AGENT, - HttpRequest, -}; +use actix_web::{dev::RequestHead, http::header::USER_AGENT, HttpRequest}; use cadence::StatsdClient; use db::{ params, @@ -38,7 +34,7 @@ pub struct ServerState { pub node_capacity_release_rate: Option, pub node_type: NodeType, pub service_id: Option, - pub metrics: Box + pub metrics: Box, } impl ServerState { From 7bad3311eb043eabb5cdf1346d313f8044ba3153 Mon Sep 17 00:00:00 2001 From: Ethan Donowitz Date: Wed, 19 Jan 2022 21:33:39 +0000 Subject: [PATCH 3/4] use different statsd label --- src/server/metrics.rs | 16 +++++++++------- src/server/mod.rs | 23 ++++++++++++++++++----- src/settings.rs | 1 + src/tokenserver/extractors.rs | 11 ++++++++++- src/tokenserver/settings.rs | 4 ++++ src/web/extractors.rs | 9 ++++++++- 6 files changed, 50 insertions(+), 14 deletions(-) diff --git a/src/server/metrics.rs b/src/server/metrics.rs index 99ce8cefd9..a178ce1f15 100644 --- a/src/server/metrics.rs +++ b/src/server/metrics.rs @@ -12,7 +12,6 @@ use futures::future::Ready; use crate::error::ApiError; use crate::server::ServerState; -use crate::settings::Settings; use crate::web::tags::Tags; #[derive(Debug, Clone)] @@ -176,18 +175,21 @@ pub fn metrics_from_req(req: &HttpRequest) -> Result, Error> { .clone()) } -/// Create a cadence StatsdClient from the given options -pub fn metrics_from_opts(opts: &Settings) -> Result { - let builder = if let Some(statsd_host) = opts.statsd_host.as_ref() { +pub fn metrics_from_opts( + label: String, + host: Option, + port: u16, +) -> Result { + let builder = if let Some(statsd_host) = host.as_ref() { let socket = UdpSocket::bind("0.0.0.0:0")?; socket.set_nonblocking(true)?; - let host = (statsd_host.as_str(), opts.statsd_port); + let host = (statsd_host.as_str(), port); let udp_sink = BufferedUdpMetricSink::from(host, socket)?; let sink = QueuingMetricSink::from(udp_sink); - StatsdClient::builder(opts.statsd_label.as_ref(), sink) + StatsdClient::builder(label.as_ref(), sink) } else { - StatsdClient::builder(opts.statsd_label.as_ref(), NopMetricSink) + StatsdClient::builder(label.as_ref(), NopMetricSink) }; Ok(builder .with_error_handler(|err| { diff --git a/src/server/mod.rs b/src/server/mod.rs index be62aacc72..72be62be8d 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -224,7 +224,11 @@ macro_rules! build_app_without_syncstorage { impl Server { pub async fn with_settings(settings: Settings) -> Result { let settings_copy = settings.clone(); - let metrics = metrics::metrics_from_opts(&settings)?; + let metrics = metrics::metrics_from_opts( + settings.statsd_label.clone(), + settings.statsd_host.clone(), + settings.statsd_port, + )?; let host = settings.host.clone(); let port = settings.port; let db_pool = pool_from_settings(&settings, &Metrics::from(&metrics)).await?; @@ -241,7 +245,11 @@ impl Server { let tokenserver_state = if settings.tokenserver.enabled { Some(tokenserver::ServerState::from_settings( &settings.tokenserver, - metrics.clone(), + metrics::metrics_from_opts( + settings.tokenserver.statsd_label.clone(), + settings.statsd_host, + settings.statsd_port, + )?, )?) } else { None @@ -287,9 +295,14 @@ impl Server { let host = settings.host.clone(); let port = settings.port; let secrets = Arc::new(settings.master_secret.clone()); - let metrics = metrics::metrics_from_opts(&settings)?; - let tokenserver_state = - tokenserver::ServerState::from_settings(&settings.tokenserver, metrics)?; + let tokenserver_state = tokenserver::ServerState::from_settings( + &settings.tokenserver, + metrics::metrics_from_opts( + settings.tokenserver.statsd_label.clone(), + settings.statsd_host, + settings.statsd_port, + )?, + )?; let server = HttpServer::new(move || { build_app_without_syncstorage!( Some(tokenserver_state.clone()), diff --git a/src/settings.rs b/src/settings.rs index 1fae3bf58f..ee4b16ddd2 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -193,6 +193,7 @@ impl Settings { s.set_default("tokenserver.fxa_metrics_hash_secret", "secret")?; s.set_default("tokenserver.test_mode_enabled", false)?; s.set_default("tokenserver.node_type", "spanner")?; + s.set_default("tokenserver.statsd_label", "syncstorage.tokenserver")?; // Set Cors defaults s.set_default( diff --git a/src/tokenserver/extractors.rs b/src/tokenserver/extractors.rs index 2be4d3fd84..c37b8fe414 100644 --- a/src/tokenserver/extractors.rs +++ b/src/tokenserver/extractors.rs @@ -1035,6 +1035,8 @@ mod tests { } fn make_state(verifier: MockOAuthVerifier) -> ServerState { + let settings = Settings::default(); + ServerState { fxa_email_domain: "test.com".to_owned(), fxa_metrics_hash_secret: "".to_owned(), @@ -1043,7 +1045,14 @@ mod tests { node_capacity_release_rate: None, node_type: NodeType::default(), service_id: None, - metrics: Box::new(metrics::metrics_from_opts(&Settings::default()).unwrap()), + metrics: Box::new( + metrics::metrics_from_opts( + settings.tokenserver.statsd_label, + settings.statsd_host, + settings.statsd_port, + ) + .unwrap(), + ), } } } diff --git a/src/tokenserver/settings.rs b/src/tokenserver/settings.rs index ed0f238d26..4745270af4 100644 --- a/src/tokenserver/settings.rs +++ b/src/tokenserver/settings.rs @@ -36,6 +36,9 @@ pub struct Settings { /// The type of the storage nodes used by this instance of Tokenserver. pub node_type: NodeType, + + /// The label to be used when reporting Metrics. + pub statsd_label: String, } impl Default for Settings { @@ -52,6 +55,7 @@ impl Default for Settings { test_mode_enabled: false, node_capacity_release_rate: None, node_type: NodeType::Spanner, + statsd_label: "syncstorage.tokenserver".to_owned(), } } } diff --git a/src/web/extractors.rs b/src/web/extractors.rs index 15b99b356f..716324abb3 100644 --- a/src/web/extractors.rs +++ b/src/web/extractors.rs @@ -1725,7 +1725,14 @@ mod tests { limits: Arc::clone(&SERVER_LIMITS), limits_json: serde_json::to_string(&**SERVER_LIMITS).unwrap(), port: 8000, - metrics: Box::new(metrics::metrics_from_opts(&settings).unwrap()), + metrics: Box::new( + metrics::metrics_from_opts( + settings.statsd_label, + settings.statsd_host, + settings.statsd_port, + ) + .unwrap(), + ), quota_enabled: settings.enable_quota, deadman: Arc::new(RwLock::new(Deadman::default())), } From 392611143cb23f1725e8ea465e4a576617d9935e Mon Sep 17 00:00:00 2001 From: Ethan Donowitz Date: Mon, 24 Jan 2022 16:08:15 +0000 Subject: [PATCH 4/4] use references --- src/server/metrics.rs | 12 ++++++------ src/server/mod.rs | 12 ++++++------ src/tokenserver/extractors.rs | 4 ++-- src/web/extractors.rs | 4 ++-- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/server/metrics.rs b/src/server/metrics.rs index a178ce1f15..66d0d77eb5 100644 --- a/src/server/metrics.rs +++ b/src/server/metrics.rs @@ -176,20 +176,20 @@ pub fn metrics_from_req(req: &HttpRequest) -> Result, Error> { } pub fn metrics_from_opts( - label: String, - host: Option, + label: &str, + host: Option<&str>, port: u16, ) -> Result { - let builder = if let Some(statsd_host) = host.as_ref() { + let builder = if let Some(statsd_host) = host { let socket = UdpSocket::bind("0.0.0.0:0")?; socket.set_nonblocking(true)?; - let host = (statsd_host.as_str(), port); + let host = (statsd_host, port); let udp_sink = BufferedUdpMetricSink::from(host, socket)?; let sink = QueuingMetricSink::from(udp_sink); - StatsdClient::builder(label.as_ref(), sink) + StatsdClient::builder(label, sink) } else { - StatsdClient::builder(label.as_ref(), NopMetricSink) + StatsdClient::builder(label, NopMetricSink) }; Ok(builder .with_error_handler(|err| { diff --git a/src/server/mod.rs b/src/server/mod.rs index 72be62be8d..3cc4394d1d 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -225,8 +225,8 @@ impl Server { pub async fn with_settings(settings: Settings) -> Result { let settings_copy = settings.clone(); let metrics = metrics::metrics_from_opts( - settings.statsd_label.clone(), - settings.statsd_host.clone(), + &settings.statsd_label, + settings.statsd_host.as_deref(), settings.statsd_port, )?; let host = settings.host.clone(); @@ -246,8 +246,8 @@ impl Server { Some(tokenserver::ServerState::from_settings( &settings.tokenserver, metrics::metrics_from_opts( - settings.tokenserver.statsd_label.clone(), - settings.statsd_host, + &settings.tokenserver.statsd_label, + settings.statsd_host.as_deref(), settings.statsd_port, )?, )?) @@ -298,8 +298,8 @@ impl Server { let tokenserver_state = tokenserver::ServerState::from_settings( &settings.tokenserver, metrics::metrics_from_opts( - settings.tokenserver.statsd_label.clone(), - settings.statsd_host, + &settings.tokenserver.statsd_label, + settings.statsd_host.as_deref(), settings.statsd_port, )?, )?; diff --git a/src/tokenserver/extractors.rs b/src/tokenserver/extractors.rs index c37b8fe414..c8d436250d 100644 --- a/src/tokenserver/extractors.rs +++ b/src/tokenserver/extractors.rs @@ -1047,8 +1047,8 @@ mod tests { service_id: None, metrics: Box::new( metrics::metrics_from_opts( - settings.tokenserver.statsd_label, - settings.statsd_host, + &settings.tokenserver.statsd_label, + settings.statsd_host.as_deref(), settings.statsd_port, ) .unwrap(), diff --git a/src/web/extractors.rs b/src/web/extractors.rs index 716324abb3..4ab9535fee 100644 --- a/src/web/extractors.rs +++ b/src/web/extractors.rs @@ -1727,8 +1727,8 @@ mod tests { port: 8000, metrics: Box::new( metrics::metrics_from_opts( - settings.statsd_label, - settings.statsd_host, + &settings.statsd_label, + settings.statsd_host.as_deref(), settings.statsd_port, ) .unwrap(),