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

[autoendpoint] Add sentry support to web handlers #155

Closed
AzureMarker opened this issue Jun 16, 2020 · 6 comments · Fixed by #196 or #231
Closed

[autoendpoint] Add sentry support to web handlers #155

AzureMarker opened this issue Jun 16, 2020 · 6 comments · Fixed by #196 or #231
Assignees
Labels
5 Estimate - Medium

Comments

@AzureMarker
Copy link
Contributor

Top-level panics are caught by sentry already, but panics and other errors in Actix handlers are caught at the worker level by Actix instead of sentry.

Some possible solutions:

@AzureMarker AzureMarker added the 3 Estimate - Small...ish label Jun 16, 2020
@AzureMarker AzureMarker self-assigned this Jun 16, 2020
@AzureMarker AzureMarker added this to the Autoendpoint Rust Server milestone Jun 16, 2020
@jrconlin
Copy link
Member

Yeah, the sentry middleware is definitely a hack. The original actix author felt that errors should not progress into the middleware and should be handled by the functions. I don't know if that issue will be re-opened now that the project is under different management.

@pjenvey
Copy link
Member

pjenvey commented Jun 16, 2020

actix-web middleware is just not super friendly to use as of actix-web 1.0.

They require some extra boilerplate to use (the Transform), use their own ServiceRequest/ServiceResponse (vs the regular HttpRequest/Response) which can be limiting particularly once they're consumed by calling into the wrapped middleware (e.g. self.service.call(sevice_request)).

With all these drawbacks + the strange error handling, I generally don't recommend using actix-web middlewares at all unless there's a very compelling reason and or it's for something very simple.

@AzureMarker
Copy link
Contributor Author

Using extractors (FromRequest) may be a better way to achieve some of what we need.

@AzureMarker AzureMarker added this to Prioritized in Services Engineering Jun 16, 2020
@AzureMarker AzureMarker moved this from Prioritized to In Progress in Services Engineering Jun 22, 2020
@AzureMarker
Copy link
Contributor Author

sentry-actix does not support Actix 1.0, so we may have to re-use our "hack" middleware:
getsentry/sentry-rust#143

@tublitzed tublitzed moved this from In Progress to Prioritized in Services Engineering Jun 22, 2020
@AzureMarker
Copy link
Contributor Author

This issue is now deprioritized, but here are some learnings:

  • sentry 0.18 does not support std::error::Error, and some useful utilities (i.e. backtrace_to_stacktrace) are private. sentry 0.19 fixes most of these problems. We need to upgrade sentry before this issue can be completed.
  • sentry 0.19 splits into multiple crates (sentry-core, sentry-backtrace, etc). The sentry crate combines these and acts like the 0.18 version (feature flags to select features). We should continue to use the sentry crate because it provides the transport integrations (curl).
  • Since sentry-actix does not work with Actix 1.0, we need to write our own middleware. The one used in syncstorage-rs is complicated by support for errors in middleware. We can make a simpler sentry middleware by not supporting errors in middleware.
  • We can write our own Sentry Actix middleware pretty easily via App::wrap_fn:
    App::new().wrap_fn(sentry_middleware)
    
    // ...
    
    fn sentry_middleware(
        req: ServiceRequest,
        service: &mut impl Service<
            Request = ServiceRequest,
            Response = ServiceResponse,
            Error = actix_web::Error,
        >,
    ) -> impl Future<Output = Result<ServiceResponse, actix_web::Error>> {
        let response = service.call(req);
    
        async move {
            let response: Result<ServiceResponse, actix_web::Error> = response.await;
            // Check for error and send to sentry
            response
        }
    }

@AzureMarker AzureMarker moved this from Prioritized to Scheduled in Services Engineering Jul 20, 2020
@AzureMarker AzureMarker added 5 Estimate - Medium and removed 3 Estimate - Small...ish labels Jul 21, 2020
@AzureMarker
Copy link
Contributor Author

Bumped to 5 due to sentry not working with Actix 1.0/2.0. We need to make our own middleware.

@AzureMarker AzureMarker moved this from Scheduled to In Progress in Services Engineering Jul 21, 2020
@AzureMarker AzureMarker moved this from In Progress to In Review in Services Engineering Jul 23, 2020
Services Engineering automation moved this from In Review to Done Jul 27, 2020
AzureMarker added a commit that referenced this issue Jul 27, 2020
* Update sentry to 0.19

* Override status_code() in ApiError's ResponseError impl

* Use the default Sentry transport and work around a Sentry issue

Working around this issue by using the `debug-logs` feature:
getsentry/sentry-rust#237

* Add custom Sentry middleware

Closes #155
@tublitzed tublitzed moved this from Done to Archived in Services Engineering Jul 30, 2020
This was referenced Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment