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.

Sign up for GitHub

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

Jump to bottom

ApiErrors from middlewares aren't reported to sentry #504

Closed
pjenvey opened this issue Mar 19, 2020 · 0 comments · Fixed by #604
Closed

ApiErrors from middlewares aren't reported to sentry #504

pjenvey opened this issue Mar 19, 2020 · 0 comments · Fixed by #604
Assignees
@jrconlin
Labels
5 Estimate - l - Moderately complex, will require some effort but clearly defined. bug Something isn't working
Projects
Services Engineering

Comments

@pjenvey
Copy link
Member

pjenvey commented Mar 19, 2020

Despite SentryMiddleware being the outtermost/last middleware, it does not receive errors triggered from other middlewares.

See actix-web#1051 for some discussion of this behavior.

These errors should be reported to sentry but it may be tricky to do so for all potential cases: e.g. carefully distinguishing between an ApiError from a handler that SentryMiddleware will report vs one from the middleware (especially amid the Future chains).

The more important ones to report are from DbTransactionMiddleware (from the db.commit/rollback() calls). A potential "good enough" fix here may be just adding special case reporting for those two.
@pjenvey pjenvey added bug Something isn't working 5 Estimate - l - Moderately complex, will require some effort but clearly defined. labels Mar 19, 2020
@pjenvey pjenvey added this to Backlog: Misc in Services Engineering via automation Mar 19, 2020
@pjenvey pjenvey moved this from Backlog: Misc to Prioritized in Services Engineering Mar 19, 2020
@tublitzed tublitzed moved this from Prioritized to Scheduled in Services Engineering Apr 20, 2020
jrconlin added a commit that referenced this issue Apr 22, 2020
@jrconlin
feat: log messages from middleware to sentry
22fa6c9 
Actix currently discards any errors generated in the middleware. You are
supposed to pass them in the body or some similar sub-optimal means.

Not a super fan of this approach, but basically, this patch attaches a
the error to the request or response extensions bucket. Since sentry is
also a middleware and gets run on every response, it sweeps the
request/response extension buckets and reports any errors it finds.

This has the downside that some context may be lost from the error (e.g.
I STRONGLY suspect the backtrace will be useless)

WIP

Closes #504
@jrconlin jrconlin mentioned this issue Apr 23, 2020
Merged
@tublitzed tublitzed moved this from Scheduled to In Review in Services Engineering Apr 23, 2020
jrconlin added a commit that referenced this issue Apr 24, 2020
@jrconlin
feat: log messages from middleware to sentry
d63e51f 
Actix currently discards any errors generated in the middleware. You are
supposed to pass them in the body or some similar sub-optimal means.

Not a super fan of this approach, but basically, this patch attaches a
the error to the request or response extensions bucket. Since sentry is
also a middleware and gets run on every response, it sweeps the
request/response extension buckets and reports any errors it finds.

I created several new `ApiErrorKind`s to encapsulate the errors being
generated by the middleware layers. Partly to help isolate them and
partly to help any future filtering we may want to do.

Closes #504
@pjenvey pjenvey mentioned this issue May 8, 2020
Closed
jrconlin added a commit that referenced this issue Jun 3, 2020
@jrconlin
feat: log messages from middleware to sentry
05ca426 
Actix currently discards any errors generated in the middleware. You are
supposed to pass them in the body or some similar sub-optimal means.

`debug!` statements have been added in the sentry middleware to note if an error has been detected and logged. You can trigger one such error (Bad Hawk Id) by starting a server and trying to fetch from `http://localhost:8000/test`

Closes #504
@pjenvey pjenvey moved this from In Review to Done in Services Engineering Jun 8, 2020
pjenvey pushed a commit that referenced this issue Jun 9, 2020
@jrconlin
feat: log messages from middleware to sentry (#604)
b6ced47 
* feat: log messages from middleware to sentry

Actix currently discards any errors generated in the middleware. You are
supposed to pass them in the body or some similar sub-optimal means.

`debug!` statements have been added in the sentry middleware to note if an error has been detected and logged. You can trigger one such error (Bad Hawk Id) by starting a server and trying to fetch from `http://localhost:8000/test`

Closes #504
@tublitzed tublitzed moved this from Done to Archived in Services Engineering Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jrconlin jrconlin

Labels
5 Estimate - l - Moderately complex, will require some effort but clearly defined. bug Something isn't working
Projects
Services Engineering
  
Archived
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: log messages from middleware to sentry mozilla-services/syncstorage-rs
2 participants
@pjenvey @jrconlin