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

feat: actix-web 3 support #282

Merged
merged 6 commits into from
Oct 20, 2020
Merged

feat: actix-web 3 support #282

merged 6 commits into from
Oct 20, 2020

Conversation

aramperes
Copy link
Contributor

@aramperes aramperes commented Oct 17, 2020 •
edited
Loading

I refactored the middleware by basing it on 'official' v3 middleware (logger, cors, etc.) Some of the Sentry implementation was re-used from https://github.com/mozilla-services/autopush-rs/blob/9b93817776bb7c8f27383185a1b5ce08607a6b35/autoendpoint/src/middleware/sentry.rs (v2.0 middleware, adapted for v3.0 and wrap-style middleware).

Example is updated and tested locally.

Bumped minimum Rust version to 1.42.0 because that's what actix-web 3.0 targets.

Resolves #143

@aramperes aramperes force-pushed the actix-3 branch 3 times, most recently from 9311d0e to 7d3476d Compare October 18, 2020 04:23
@jan-auer jan-auer requested a review from Swatinem October 18, 2020 13:20
@OskarPersson
Copy link

I've tried this branch with the provided example locally on my machine and can't get it to produce an issue on Sentry. The error message is rendered as expected in the browser but no issue is created.

@onelson
Copy link

onelson commented Oct 18, 2020

@OskarPersson I wonder if there could be any overlap with #239 or #277

@aramperes
Copy link
Contributor Author

aramperes commented Oct 18, 2020 •
edited
Loading

@OskarPersson @onelson

I ran the updated basic.rs sample (after changing the DSN to a real sentry.io one), and the event/issue is created properly:

image

Can you provide a counter-example? Maybe your machine specs, Sentry version, Rust version, etc?

Swatinem
Swatinem reviewed Oct 19, 2020
View reviewed changes
Copy link
Member

@Swatinem Swatinem 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 awesome!

I left a few comments where I think we can further simplify things.

@aramperes
Code review changes.
c61f4f3 
* Clean-up dependencies
* Clean-up unnecessary actix-to-std conversions
* Use given Hub instead of creating a new one for every request
@aramperes
Code review changes and documentation/sample update.
d61a211
@aramperes aramperes requested a review from Swatinem October 19, 2020 16:19
Swatinem
Swatinem reviewed Oct 19, 2020
View reviewed changes
@Swatinem
Copy link
Member

Some unit tests for this would be nice, also to make sure that concurrent requests don’t clash with each other.

@aramperes
Copy link
Contributor Author

@Swatinem I'll look into adding some unit tests combining actix_rt::test and Sentry.

@aramperes
Copy link
Contributor Author

I added 2 unit tests: one that tests explicit events via sentry::capture_message, and the other for events dispatched by the middleware through service errors.

Both tests also ensure the Hubs are isolated by checking whether last_event_id is set in different contexts, and finally for the main Hub to ensure it was never accessed.

Swatinem
Swatinem reviewed Oct 20, 2020
View reviewed changes
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

Sorry for the churn here.

I wonder if we can use the sentry::test module to capture the events (not sure if it uses main or current), and then just fire some requests and block_on having them all complete.

sentry-actix/src/lib.rs Outdated
// Call the service twice to check the isolation between them
for _ in 0..2 {
let req = TestRequest::get().uri("/test").to_request();
let res = call_service(&mut app, req).await;
Copy link
Member

Choose a reason for hiding this comment

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

doesn’t the await here make these sequencial instead of concurrent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I was mainly testing the isolation from sequential requests (i.e. making sure the middleware isn't sticky).

Testing the concurrent isolation would be a different beast, I think. How would you determine that both requests used different Hub instances? Is there a way to identify individual Hubs?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to identify individual Hubs?

you could use configure_scope to set different properties. But I think this is very good now

@Swatinem Swatinem merged commit 4188efd into getsentry:master Oct 20, 2020
@Swatinem
Copy link
Member

This is amazing! Thanks so much 🚀

@Swatinem Swatinem mentioned this pull request Oct 20, 2020
Merged
@aramperes aramperes deleted the actix-3 branch October 20, 2020 22:51
@daggy1234
Copy link

Thanks been waiting for this!

jan-auer added a commit that referenced this pull request Nov 25, 2020
@jan-auer
Merge branch 'master' into fix/linux-debug-images
e170eca 
* master: (59 commits)
  fix: Correctly apply environment from env (#293)
  fix: Make Rust 1.48 clippy happy (#294)
  docs: Document integrations and the Hub better (#291)
  ref: Remove deprecated error-chain and failure crates (#290)
  release: 0.21.0
  meta: Update Changelog
  feat: End sessions with explicit status (#289)
  fix: Scope transaction name can be overriden in sentry-actix (#287)
  fix: sentry-actix should not capture client errors (#286)
  fix: Clean up sentry-actix toml (#285)
  ref: Remove empty integrations (#283)
  feat: Add support for actix-web 3 (#282)
  feat: Preliminary work to integrate Performance Monitoring (#276)
  ref: Introduce a SessionFlusher (#279)
  fix: Set a default environment based on debug_assertions (#280)
  ref: Rearchitect the log and slog Integrations (#268)
  ref: Deprecate public fields on Integrations (#267)
  ci: Make testfast actually fast (#273)
  fix: Update surf and unbreak CI (#274)
  ci: Use smarter cache action (#272)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swatinem Swatinem Swatinem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

actix-web 3.0
5 participants
@aramperes @OskarPersson @onelson @Swatinem @daggy1234