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

ref(server): Use async/await in all endpoints #1862

Merged
merged 9 commits into from
Feb 22, 2023
Merged

ref(server): Use async/await in all endpoints #1862

merged 9 commits into from
Feb 22, 2023

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Feb 20, 2023 •
edited
Loading

Moves all endpoints and most HTTP utilities to async/await. This is to prepare
upgrading the web framework.

#skip-changelog

@jan-auer
Copy link
Member Author

jan-auer commented Feb 21, 2023 •
edited
Loading

The failing integration test shows that error reporting has changed with my refactored store endpoints. Since the endpoints now return an error, actix_web::pipeline logs a duplicate error message (but without the causes). This is undesirable since it would create noise in Sentry.

See the following excerpt. The first line is our manual log, the other two are now added by actix_web:

[relay_server::endpoints::common] ERROR: error handling request: failed to queue envelope
  caused by: envelope buffer capacity exceeded
[actix_web::pipeline] ERROR: Error occurred during request handling, status: 503 Service Unavailable failed to queue envelope
[actix_web::pipeline] DEBUG: QueueFailed(BufferError)

I'm looking into a quick fix. It's useful to return an error, but on the other hand we want clean error logs and reports.

Update: Like in the past, the only workaround is to convert BadStoreRequest before it hits the actix-web pipeline. Since we already needed specific boilerplate to create actix-web handlers, I decided to create a simple wrapper functions that converts the error and creates a compat future.

olksdr
olksdr reviewed Feb 21, 2023
View reviewed changes
relay-server/src/endpoints/common.rs Outdated Show resolved Hide resolved
relay-server/src/endpoints/minidump.rs Outdated Show resolved Hide resolved
@jan-auer jan-auer marked this pull request as ready for review February 21, 2023 11:53
@jan-auer jan-auer requested a review from a team February 21, 2023 11:53
olksdr
olksdr approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

relay-server/src/service.rs Outdated Show resolved Hide resolved
@jan-auer @olksdr
Update relay-server/src/service.rs
61c9976 
Co-authored-by: Oleksandr <1931331+olksdr@users.noreply.github.com>
@jan-auer jan-auer self-assigned this Feb 22, 2023
jjbayer
jjbayer approved these changes Feb 22, 2023
View reviewed changes
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Did not look at the details but in general this looks good to me! Great improvement in code readability.

relay-server/src/extractors/shared_payload.rs

/// Consume the remaining response body.
pub async fn consume(mut self) {
while let Ok(Some(_)) = self.chunk().await {}
Copy link
Member

Choose a reason for hiding this comment

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

If self.chunk() returns an error, this function will silently fail. Is that the same behavior as we had before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we want to consume until there's an error and then bail.

@jan-auer jan-auer merged commit 5e89b77 into master Feb 22, 2023
@jan-auer jan-auer deleted the ref/endpoints-async branch February 22, 2023 11:48
iker-barriocanal
iker-barriocanal reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

So much simpler! 🙌

jan-auer added a commit that referenced this pull request Feb 23, 2023
@jan-auer
Merge branch 'master' into remove-unsupported-macos-15
aa44723 
* master:
  feat(metrics): Tag the sample decision on count_per_root_project (#1870)
  feat(protocol): Add sourcemap debug image type to protocol (#1869)
  ref(statsd): Revert back the adition of metric names as tag on Sentry errors (#1873)
  feat(profiling): Add PHP support (#1871)
  fix(panic): revert sentry-types to 0.20.1 (#1872)
  ref(server): Use async/await in all endpoints (#1862)
  ref: Buffer envelopes for broken project states (#1856)
  meta: Remove accidentally added GeoIP file (#1866)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iker-barriocanal iker-barriocanal iker-barriocanal left review comments

@jjbayer jjbayer jjbayer approved these changes

@olksdr olksdr olksdr approved these changes

Assignees

@jan-auer jan-auer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jan-auer @jjbayer @olksdr @iker-barriocanal