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

actix-web 1.0 #143

Open
clux opened this issue May 27, 2019 · 6 comments
Open

actix-web 1.0 #143

clux opened this issue May 27, 2019 · 6 comments

Comments

@clux
Copy link

@clux clux commented May 27, 2019

Hey! First of all, thanks a lot for all these popular integrations. Really makes sentry a breeze to use in rust.

Been trying the actix-web 1.0 rc that recently came up. Judging by their 1.0 migration guide middleware should just be mounted slightly differently:

instead of

    let app = App::new()
         .middleware(middleware::Logger::default())

use .wrap() method

    let app = App::new()
         .wrap(middleware::Logger::default())

but there does appear to be more to it under the hood because the sentry middleware is the last thing for me that fails during the migration:

    .wrap(sentry_actix::SentryMiddleware::new())

| .wrap(sentry_actix::SentryMiddleware::new())
| ^^^^ the trait actix_service::transform::IntoTransform<_, <impl actix_service::NewService as actix_service::NewService>::Service> is not implemented for sentry_actix::SentryMiddleware

Haven't looked into what's actually needed to fix this here, but hopefully it's an easy impl 🙏

@Jake-Shadle

This comment has been minimized.

Copy link

@Jake-Shadle Jake-Shadle commented Jun 5, 2019

It's a little more involved unfortunately, as they've completely changed how middleware works, as well as many API changes that makes this more difficult than I had wanted. There is also actix/actix-web#681 which is a problem since a lot of error information, particularly backtraces, is now lost by the time sentry wants to post errors.

@Jake-Shadle

This comment has been minimized.

Copy link

@Jake-Shadle Jake-Shadle commented Jun 5, 2019

Oh, and here's a WIP branch that gets at least most of the way. EmbarkStudios@ea827a6

@mitsuhiko

This comment has been minimized.

Copy link
Member

@mitsuhiko mitsuhiko commented Jun 19, 2019

So I started looking into this now and the fact that those errors are no longer Fails makes it pretty hard to do useful error reporting here. Not sure what we can do atm.

@repi

This comment has been minimized.

Copy link

@repi repi commented Jun 19, 2019

A small side note, we at Embark moved away from using actix-web to just use hyper directly to reduce complexity & amount of dependencies, so no longer need this.

@pandaman64

This comment has been minimized.

Copy link

@pandaman64 pandaman64 commented Sep 9, 2019

I'm interested in implementing this. What's the main blocker?

@jan-auer

This comment has been minimized.

Copy link
Member

@jan-auer jan-auer commented Sep 9, 2019

@pandaman64 The last time we checked the blocker was that there was no nice way to get the route name from Actix in order to print the transaction label. There's a partial implementation that lives here, and that could be ported:

https://github.com/getsentry/symbolicator/blob/f1b4285b94371a8f57f97509e2d6d3e9687463ba/src/middleware/sentry.rs

Also, this uses SentryFutureExt, which we were thinking about porting over to the Sentry SDK:

https://github.com/getsentry/symbolicator/blob/f1b4285b94371a8f57f97509e2d6d3e9687463ba/src/utils/sentry.rs

DSpeckhals added a commit to DSpeckhals/bible.rs that referenced this issue Oct 2, 2019
This commit fixes a bug where a search value that starts with a number
and ends with a space would throw a database error which was unhandled
by the middleware. To fix this error, the query string is trimmed and an
empty result set is returned. The error handling was also improved to
log the underlying database error instead of a generic message.

In the process, I realized that it would be nice to be alerted to these
types of issues instead of accidentally noticing them in a log. Sentry
is a relatively simple option, so I added it. Unfortunately, though
Sentry provides middleware for actix-web, it doesn't work yet for
versions > 1.0. I copied and pasted code from [this
issue](getsentry/sentry-rust#143) that lets
the middleware work in a less-featured-than-ideal manner. I also added
the Sentry panic handler.
DSpeckhals added a commit to DSpeckhals/bible.rs that referenced this issue Oct 2, 2019
This commit fixes a bug where a search value that starts with a number
and ends with a space would throw a database error which was unhandled
by the middleware. To fix this error, the query string is trimmed and an
empty result set is returned. The error handling was also improved to
log the underlying database error instead of a generic message.

In the process, I realized that it would be nice to be alerted to these
types of issues instead of accidentally noticing them in a log. Sentry
is a relatively simple option, so I added it. Unfortunately, though
Sentry provides middleware for actix-web, it doesn't work yet for
versions > 1.0. I copied and pasted code from [this
issue](getsentry/sentry-rust#143) that lets
the middleware work in a less-featured-than-ideal manner. I also added
the Sentry panic handler.
DSpeckhals added a commit to DSpeckhals/bible.rs that referenced this issue Oct 3, 2019
This commit fixes a bug where a search value that starts with a number
and ends with a space would throw a database error which was unhandled
by the middleware. To fix this error, the query string is trimmed and an
empty result set is returned. The error handling was also improved to
log the underlying database error instead of a generic message.

In the process, I realized that it would be nice to be alerted to these
types of issues instead of accidentally noticing them in a log. Sentry
is a relatively simple option, so I added it. Unfortunately, though
Sentry provides middleware for actix-web, it doesn't work yet for
versions > 1.0. I copied and pasted code from [this
issue](getsentry/sentry-rust#143) that lets
the middleware work in a less-featured-than-ideal manner. I also added
the Sentry panic handler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.