Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename MySQL error to use a more generic name #1619

Merged
merged 5 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 19 additions & 23 deletions syncserver-db-common/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ use http::StatusCode;
use syncserver_common::{from_error, impl_fmt_display, ReportableError};
use thiserror::Error;

/// Error specific to any MySQL database backend. These errors are not related to the syncstorage
/// Error specific to any SQL database backend. These errors are not related to the syncstorage
/// or tokenserver application logic; rather, they are lower-level errors arising from diesel.
Copy link
Member

@pjenvey pjenvey Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment still stands, "lower-level errors arising from diesel" and I think suggests a clearer name for this: DieselError, how does that sound?

Copy link
Contributor Author

@Eragonfr Eragonfr Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a good name too. As all errors are from diesel or it's dependencies (r2d2 or diesel_migrations).

But if we use SqlError that means that we could replace diesel with an other ORM.
If someone wants to support a database that's not supported by Diesel. But maybe in that case they'll need to do something similar to the spanner backend.

Idk. If @jrconlin and @taddes prefer DieselError I can change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'm fine either way, but I do appreciate the concept of having a more generic SqlError for the reason that @Eragonfr notes. I don't really see us replacing diesel any time soon, but the sources of the error being reported are coming from the underlying SQL system. DieselError type should be reserved for errors arising from the diesel system directly (e.g. failure to connect to the data store, or some configuration break).

#[derive(Debug)]
pub struct MysqlError {
kind: MysqlErrorKind,
pub struct SqlError {
kind: SqlErrorKind,
pub status: StatusCode,
pub backtrace: Backtrace,
}

#[derive(Debug, Error)]
enum MysqlErrorKind {
enum SqlErrorKind {
#[error("A database error occurred: {}", _0)]
DieselQuery(#[from] diesel::result::Error),

Expand All @@ -29,8 +29,8 @@ enum MysqlErrorKind {
Migration(diesel_migrations::RunMigrationsError),
}

impl From<MysqlErrorKind> for MysqlError {
fn from(kind: MysqlErrorKind) -> Self {
impl From<SqlErrorKind> for SqlError {
fn from(kind: SqlErrorKind) -> Self {
Self {
kind,
status: StatusCode::INTERNAL_SERVER_ERROR,
Expand All @@ -39,22 +39,22 @@ impl From<MysqlErrorKind> for MysqlError {
}
}

impl ReportableError for MysqlError {
impl ReportableError for SqlError {
fn is_sentry_event(&self) -> bool {
#[allow(clippy::match_like_matches_macro)]
match &self.kind {
MysqlErrorKind::Pool(_) => false,
SqlErrorKind::Pool(_) => false,
_ => true,
}
}

fn metric_label(&self) -> Option<String> {
Some(
match self.kind {
MysqlErrorKind::DieselQuery(_) => "storage.mysql.error.diesel_query",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These metric changes do not impact our dashboards.

MysqlErrorKind::DieselConnection(_) => "storage.mysql.error.diesel_connection",
MysqlErrorKind::Pool(_) => "storage.mysql.error.pool",
MysqlErrorKind::Migration(_) => "storage.mysql.error.migration",
SqlErrorKind::DieselQuery(_) => "storage.sql.error.diesel_query",
SqlErrorKind::DieselConnection(_) => "storage.sql.error.diesel_connection",
SqlErrorKind::Pool(_) => "storage.sql.error.pool",
SqlErrorKind::Migration(_) => "storage.sql.error.migration",
}
.to_string(),
)
Expand All @@ -65,21 +65,17 @@ impl ReportableError for MysqlError {
}
}

impl_fmt_display!(MysqlError, MysqlErrorKind);
impl_fmt_display!(SqlError, SqlErrorKind);

from_error!(
diesel::result::Error,
MysqlError,
MysqlErrorKind::DieselQuery
);
from_error!(diesel::result::Error, SqlError, SqlErrorKind::DieselQuery);
from_error!(
diesel::result::ConnectionError,
MysqlError,
MysqlErrorKind::DieselConnection
SqlError,
SqlErrorKind::DieselConnection
);
from_error!(diesel::r2d2::PoolError, MysqlError, MysqlErrorKind::Pool);
from_error!(diesel::r2d2::PoolError, SqlError, SqlErrorKind::Pool);
from_error!(
diesel_migrations::RunMigrationsError,
MysqlError,
MysqlErrorKind::Migration
SqlError,
SqlErrorKind::Migration
);
12 changes: 6 additions & 6 deletions syncstorage-mysql/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::fmt;
use backtrace::Backtrace;
use http::StatusCode;
use syncserver_common::{from_error, impl_fmt_display, InternalError, ReportableError};
use syncserver_db_common::error::MysqlError;
use syncserver_db_common::error::SqlError;
use syncstorage_db_common::error::{DbErrorIntrospect, SyncstorageDbError};
use thiserror::Error;

Expand Down Expand Up @@ -49,7 +49,7 @@ enum DbErrorKind {
Common(SyncstorageDbError),

#[error("{}", _0)]
Mysql(MysqlError),
Mysql(SqlError),
}

impl From<DbErrorKind> for DbError {
Expand Down Expand Up @@ -140,24 +140,24 @@ from_error!(SyncstorageDbError, DbError, DbErrorKind::Common);
from_error!(
diesel::result::Error,
DbError,
|error: diesel::result::Error| DbError::from(DbErrorKind::Mysql(MysqlError::from(error)))
|error: diesel::result::Error| DbError::from(DbErrorKind::Mysql(SqlError::from(error)))
);
from_error!(
diesel::result::ConnectionError,
DbError,
|error: diesel::result::ConnectionError| DbError::from(DbErrorKind::Mysql(MysqlError::from(
|error: diesel::result::ConnectionError| DbError::from(DbErrorKind::Mysql(SqlError::from(
error
)))
);
from_error!(
diesel::r2d2::PoolError,
DbError,
|error: diesel::r2d2::PoolError| DbError::from(DbErrorKind::Mysql(MysqlError::from(error)))
|error: diesel::r2d2::PoolError| DbError::from(DbErrorKind::Mysql(SqlError::from(error)))
);
from_error!(
diesel_migrations::RunMigrationsError,
DbError,
|error: diesel_migrations::RunMigrationsError| DbError::from(DbErrorKind::Mysql(
MysqlError::from(error)
SqlError::from(error)
))
);
26 changes: 12 additions & 14 deletions tokenserver-db/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::fmt;
use backtrace::Backtrace;
use http::StatusCode;
use syncserver_common::{from_error, impl_fmt_display, InternalError, ReportableError};
use syncserver_db_common::error::MysqlError;
use syncserver_db_common::error::SqlError;
use thiserror::Error;
use tokenserver_common::TokenserverError;

Expand All @@ -28,21 +28,21 @@ impl DbError {
impl ReportableError for DbError {
fn backtrace(&self) -> Option<&Backtrace> {
match &self.kind {
DbErrorKind::Mysql(e) => e.backtrace(),
DbErrorKind::Sql(e) => e.backtrace(),
_ => Some(&self.backtrace),
}
}

fn is_sentry_event(&self) -> bool {
match &self.kind {
DbErrorKind::Mysql(e) => e.is_sentry_event(),
DbErrorKind::Sql(e) => e.is_sentry_event(),
_ => true,
}
}

fn metric_label(&self) -> Option<String> {
match &self.kind {
DbErrorKind::Mysql(e) => e.metric_label(),
DbErrorKind::Sql(e) => e.metric_label(),
_ => None,
}
}
Expand All @@ -51,7 +51,7 @@ impl ReportableError for DbError {
#[derive(Debug, Error)]
enum DbErrorKind {
#[error("{}", _0)]
Mysql(MysqlError),
Sql(SqlError),

#[error("Unexpected error: {}", _0)]
Internal(String),
Expand All @@ -60,7 +60,7 @@ enum DbErrorKind {
impl From<DbErrorKind> for DbError {
fn from(kind: DbErrorKind) -> Self {
match kind {
DbErrorKind::Mysql(ref mysql_error) => Self {
DbErrorKind::Sql(ref mysql_error) => Self {
status: mysql_error.status,
backtrace: Box::new(mysql_error.backtrace.clone()),
kind,
Expand Down Expand Up @@ -105,24 +105,22 @@ impl_fmt_display!(DbError, DbErrorKind);
from_error!(
diesel::result::Error,
DbError,
|error: diesel::result::Error| DbError::from(DbErrorKind::Mysql(MysqlError::from(error)))
|error: diesel::result::Error| DbError::from(DbErrorKind::Sql(SqlError::from(error)))
);
from_error!(
diesel::result::ConnectionError,
DbError,
|error: diesel::result::ConnectionError| DbError::from(DbErrorKind::Mysql(MysqlError::from(
error
)))
|error: diesel::result::ConnectionError| DbError::from(DbErrorKind::Sql(SqlError::from(error)))
);
from_error!(
diesel::r2d2::PoolError,
DbError,
|error: diesel::r2d2::PoolError| DbError::from(DbErrorKind::Mysql(MysqlError::from(error)))
|error: diesel::r2d2::PoolError| DbError::from(DbErrorKind::Sql(SqlError::from(error)))
);
from_error!(
diesel_migrations::RunMigrationsError,
DbError,
|error: diesel_migrations::RunMigrationsError| DbError::from(DbErrorKind::Mysql(
MysqlError::from(error)
))
|error: diesel_migrations::RunMigrationsError| DbError::from(DbErrorKind::Sql(SqlError::from(
error
)))
);