From 09db55f2542a6029b33f90649bbd1f4eaa5f88bb Mon Sep 17 00:00:00 2001 From: Philip Jenvey Date: Thu, 26 Oct 2023 11:34:52 -0700 Subject: [PATCH] fix: capture sentry events for the unidentified state (#484) also kill db settings defaults and move the chatty error! log to trace SYNC-3975 SYNC-3976 --- .../autoconnect-ws-sm/src/identified/mod.rs | 4 ++-- autoconnect/autoconnect-ws/src/error.rs | 16 +++++++++++++-- autoconnect/autoconnect-ws/src/handler.rs | 17 ++++++++-------- autopush-common/src/db/bigtable/mod.rs | 20 ------------------- autopush-common/src/db/dynamodb/mod.rs | 9 --------- 5 files changed, 25 insertions(+), 41 deletions(-) diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs index d7b0c76f..7be27c58 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs +++ b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs @@ -255,12 +255,12 @@ impl WebPushClient { } /// Add User information and tags for this Client to a Sentry Event - pub fn add_sentry_info(&self, event: &mut sentry::protocol::Event) { + pub fn add_sentry_info(self, event: &mut sentry::protocol::Event) { event.user = Some(sentry::User { id: Some(self.uaid.as_simple().to_string()), ..Default::default() }); - let ua_info = self.ua_info.clone(); + let ua_info = self.ua_info; event .tags .insert("ua_name".to_owned(), ua_info.browser_name); diff --git a/autoconnect/autoconnect-ws/src/error.rs b/autoconnect/autoconnect-ws/src/error.rs index 542b78db..5d656c44 100644 --- a/autoconnect/autoconnect-ws/src/error.rs +++ b/autoconnect/autoconnect-ws/src/error.rs @@ -3,8 +3,8 @@ use std::fmt; use actix_ws::CloseCode; use backtrace::Backtrace; -use autoconnect_ws_sm::SMError; -use autopush_common::errors::ReportableError; +use autoconnect_ws_sm::{SMError, WebPushClient}; +use autopush_common::{errors::ReportableError, sentry::event_from_error}; /// WebPush WebSocket Handler Errors #[derive(Debug, thiserror::Error)] @@ -51,6 +51,18 @@ impl WSError { pub fn close_description(&self) -> &str { self.kind.as_ref() } + + /// Emit an event for this Error to Sentry + pub fn capture_sentry_event(&self, client: Option) { + if !self.is_sentry_event() { + return; + } + let mut event = event_from_error(self); + if let Some(client) = client { + client.add_sentry_info(&mut event); + } + sentry::capture_event(event); + } } impl ReportableError for WSError { diff --git a/autoconnect/autoconnect-ws/src/handler.rs b/autoconnect/autoconnect-ws/src/handler.rs index fd8a2f24..3bce5706 100644 --- a/autoconnect/autoconnect-ws/src/handler.rs +++ b/autoconnect/autoconnect-ws/src/handler.rs @@ -7,7 +7,6 @@ use tokio::{select, time::timeout}; use autoconnect_common::protocol::{ServerMessage, ServerNotification}; use autoconnect_settings::AppState; use autoconnect_ws_sm::{UnidentifiedClient, WebPushClient}; -use autopush_common::{errors::ReportableError, sentry::event_from_error}; use crate::{ error::{WSError, WSErrorKind}, @@ -30,7 +29,7 @@ pub fn spawn_webpush_ws( let close_reason = webpush_ws(client, &mut session, msg_stream) .await .unwrap_or_else(|e| { - error!("spawn_webpush_ws: Error: {}", e); + trace!("spawn_webpush_ws: Error: {}", e); Some(CloseReason { code: e.close_code(), description: Some(e.close_description().to_owned()), @@ -61,7 +60,13 @@ pub(crate) async fn webpush_ws( // NOTE: UnidentifiedClient doesn't require shutdown/cleanup, so its // Error's propagated. We don't propagate Errors afterwards to handle // shutdown/cleanup of WebPushClient - let (mut client, smsgs) = unidentified_ws(client, &mut msg_stream).await?; + let (mut client, smsgs) = match unidentified_ws(client, &mut msg_stream).await { + Ok(t) => t, + Err(e) => { + e.capture_sentry_event(None); + return Err(e); + } + }; // Client now identified: add them to the registry to recieve ServerNotifications let mut snotif_stream = client.registry_connect().await; @@ -75,11 +80,7 @@ pub(crate) async fn webpush_ws( client.shutdown(result.as_ref().err().map(|e| e.to_string())); if let Err(ref e) = result { - if e.is_sentry_event() { - let mut event = event_from_error(e); - client.add_sentry_info(&mut event); - sentry::capture_event(event); - } + e.capture_sentry_event(Some(client)); } result } diff --git a/autopush-common/src/db/bigtable/mod.rs b/autopush-common/src/db/bigtable/mod.rs index c2ed5a28..35d28d39 100644 --- a/autopush-common/src/db/bigtable/mod.rs +++ b/autopush-common/src/db/bigtable/mod.rs @@ -30,7 +30,6 @@ use crate::db::error::DbError; /// The settings for accessing the BigTable contents. #[derive(Clone, Debug, Deserialize)] -#[serde(default)] pub struct BigTableDbSettings { /// The Table name matches the GRPC template for table paths. /// e.g. `projects/{projectid}/instances/{instanceid}/tables/{tablename}` @@ -48,25 +47,6 @@ pub struct BigTableDbSettings { pub message_topic_family: String, } -/// NOTE: autopush will not autogenerate these families. They should -/// be created when the table is first provisioned. See -/// [BigTable schema](https://cloud.google.com/bigtable/docs/schema-design) -/// -/// BE SURE TO CONFIRM the names of the families. These are not checked on -/// initialization, but will throw errors if not present or incorrectly -/// spelled. -/// -impl Default for BigTableDbSettings { - fn default() -> Self { - Self { - table_name: "autopush".to_owned(), - router_family: "router".to_owned(), - message_family: "message".to_owned(), - message_topic_family: "message_topic".to_owned(), - } - } -} - impl TryFrom<&str> for BigTableDbSettings { type Error = DbError; fn try_from(setting_string: &str) -> Result { diff --git a/autopush-common/src/db/dynamodb/mod.rs b/autopush-common/src/db/dynamodb/mod.rs index ac9a839a..6965862b 100644 --- a/autopush-common/src/db/dynamodb/mod.rs +++ b/autopush-common/src/db/dynamodb/mod.rs @@ -42,15 +42,6 @@ pub struct DynamoDbSettings { pub message_table: String, } -impl Default for DynamoDbSettings { - fn default() -> Self { - Self { - router_table: "router".to_string(), - message_table: "message".to_string(), - } - } -} - impl TryFrom<&str> for DynamoDbSettings { type Error = DbError; fn try_from(setting_string: &str) -> Result {