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

feat: Add normalized ReportableError to errors #1559

Merged
merged 6 commits into from May 20, 2024

Conversation

jrconlin
Copy link
Member

@jrconlin jrconlin commented May 10, 2024

We want to do things like add tags and other features to sync errors the
way that we do in other packages. To do so, we're backporting
ReportableError from Autopush to Syncstorage.

This also addresses:

  • some clippy fixes required by 1.78
  • address comment

Closes SYNC-4262

We want to do things like add tags and other features to sync errors the
way that we do in other packages. To do so, we're backporting
ReportableError from Autopush to Syncstorage.

This also addresses some clippy fixes required by 1.78

Closes SYNC-4262
* More clippy updates
@jrconlin jrconlin requested review from pjenvey and taddes May 10, 2024 20:41
Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

This is missing the middleware integration piece

syncserver-db-common/src/error.rs Outdated Show resolved Hide resolved
syncstorage-mysql/src/diesel_ext.rs Outdated Show resolved Hide resolved
syncstorage-db-common/src/params.rs Outdated Show resolved Hide resolved
This continues to use the `Taggable` trait, which we may want to
port to autopush.
@jrconlin jrconlin requested a review from pjenvey May 14, 2024 16:42
Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

Darnit, oops, I had a couple pending comment nitpicks, but go ahead and merge as is if you want

.wrap_fn(middleware::weave::set_weave_timestamp)
.wrap_fn(tokenserver::logging::handle_request_log_line)
.wrap_fn(middleware::sentry::report_error)
//.wrap_fn(middleware::sentry::report_error)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//.wrap_fn(middleware::sentry::report_error)

}

/// Experimental: return key value pairs for Sentry Event's extra data
/// TODO: should probably return Vec<(&str, Value)> or Vec<(String, Value)>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// TODO: should probably return Vec<(&str, Value)> or Vec<(String, Value)>

// These are our wrappers
.wrap_fn(middleware::sentry::report_error)
//.wrap_fn(middleware::sentry::report_error)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//.wrap_fn(middleware::sentry::report_error)

@jrconlin jrconlin merged commit 7718130 into master May 20, 2024
8 checks passed
@jrconlin jrconlin deleted the feat/SYNC-4262_rep_error branch May 20, 2024 15:51
@jrconlin
Copy link
Member Author

I'll file a quick follow-up PR that has these fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants