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

Replace Rocket with Axum. #58

Merged
merged 6 commits into from
Feb 22, 2024
Merged

Replace Rocket with Axum. #58

merged 6 commits into from
Feb 22, 2024

Conversation

plietar
Copy link
Member

@plietar plietar commented Feb 9, 2024

Axum gives us more control over the flow of requests and allows us to use all the tower ecosystem, which has a lot of nice middlewares.

The first immediate benefit we get is better logging using the tracing ecosystem and support for a request ID. Clients can send a x-request-id header, which will be included in all log messages printed by the request. If no ID is provided one is generated randomly by the server. The ID, generated or provided by the client, is also included in the response.

Most of our business code and endpoints are untouched, since Rocket and Axum use fairly similar paradigms anyway. Most of the changes are in auxilary code, such as testing, metrics and response formatting.

@plietar plietar requested a review from r-ash February 9, 2024 12:28
Axum gives us more control over the flow of requests and allows us to
use all the tower ecosystem, which has a lot of nice middlewares.

The first immediate benefit we get is better logging using the tracing
ecosystem and support for a request ID. Clients can send a
`x-request-id` header, which will be included in all log messages
printed by the request. If no ID is provided one is generated randomly
by the server. The ID, generated or provided by the client, is also
included in the response.

Most of our business code and endpoints are untouched, since Rocket and
Axum use fairly similar paradigms anyway. Most of the changes are in
auxilary code, such as testing, metrics and response formatting.
I actually find tracing-mock a little nicer, but it isn't available on
crates.io and importing it from Git is a bit of a mess, because it tries
to pull tracing and tracing-core from Git too.
@plietar plietar changed the base branch from clippy-happy to main February 12, 2024 16:06
@r-ash r-ash requested a review from richfitz February 14, 2024 10:06
Copy link
Contributor

@r-ash r-ash left a comment

Choose a reason for hiding this comment

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

This looks really cool, great to see all the middleware stuff with tower. Lots of new stuff for me here, so not much useful I can add! But all looks really nice


pub fn serve(root: &Path) -> anyhow::Result<()> {
tracing_subscriber::fmt()
.with_max_level(tracing::Level::TRACE)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably make this configurable at some point

Copy link
Contributor

Choose a reason for hiding this comment

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

And the address and port?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree. This used to be configurable with Rocket.toml I guess, and we would have to replace it with our own config file, or even simpler with CLI arguments.

src/metrics.rs Show resolved Hide resolved
src/metrics.rs Show resolved Hide resolved
src/api.rs Show resolved Hide resolved
Copy link
Member

@richfitz richfitz left a comment

Choose a reason for hiding this comment

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

Thanks Paul; this is pretty massive and I don't have a lot (anything really) to add but don't want to hold this up

#[catch(500)]
fn internal_error(_req: &Request) -> Json<FailResponse> {
Json(FailResponse::from(OutpackError {
fn internal_error(_err: Box<dyn Any + Send + 'static>) -> Response {
Copy link
Member

Choose a reason for hiding this comment

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

curious why this does not need to be async but everything else is?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question, and I honestly don't know. Seems to just be a consequence of how the CatchPanicLayer is implemented.

This is the PR that implemented it, but the topic never got brought up: tower-rs/tower-http#214

@plietar plietar merged commit 2e832a4 into main Feb 22, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants