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

build: Update axum and http #2844

Merged
merged 12 commits into from
Dec 18, 2023
Merged

build: Update axum and http #2844

merged 12 commits into from
Dec 18, 2023

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Dec 12, 2023

We would like to update the Sentry SDK, which requires to update axum and related dependencies. As reqwest has not updated to the latest version of http and hyper yet, we will temporarily run two versions of the frameworks side-by-side and convert between their types in the forward endpoint.

The axum-server library also has not released their latest version yet. Since the only thing needed from axum-server is graceful shutdown, we implement this ourselves now following the example in the axum repository.

#skip-changelog

@jan-auer jan-auer marked this pull request as ready for review December 14, 2023 15:11
@jan-auer jan-auer requested a review from a team as a code owner December 14, 2023 15:11
@jan-auer jan-auer self-assigned this Dec 14, 2023
@jan-auer
Copy link
Member Author

There are still test failures with:

failed to parse JSON response, EOF while parsing a value at line 1 column 0

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!

Using new request extractor helped to simplify code a lot, removing all explicit generics and bounds. Nice!

Left few comments/questions.

Comment on lines +58 to +61
// Build the router middleware into a single service which runs _after_ routing. Service
// builder order defines layers added first will be called first. This means:
// - Requests go from top to bottom
// - Responses go from bottom to top
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move this to the function docs instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do, does it make more sense for you there? The order of the middlewares is something that's important to the ServiceBuilder, hence the placement within the function body.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me yes.
Since the entire function is actually dedicated to putting app together and the docs will do just good if they will be on the function level.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I was moving it and adding back an old comment that got lost, I looked at it again but it simply doesn't make sense to me. This code comment is relevant only for the implementation of the function, as it clarifies on two specifics:

  1. In which order the middlewares are registered and why
  2. Why some middlewares wrap the router and some others do not.

To a user of this function, none of these matter. In fact, the user of this function does not even see that router middlewares are applied.

relay-server/src/actors/server.rs Outdated Show resolved Hide resolved
relay-server/src/actors/server.rs Outdated Show resolved Hide resolved
fn method(&self) -> Method {
self.method.clone()
fn method(&self) -> reqwest::Method {
self.method.as_str().parse().unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

should we fallback to some default method instead of unwrap here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. The panic here is unreachable, since the parsed method is already valid. A default would also create an invalid request.

@jan-auer
Copy link
Member Author

Using new request extractor helped to simplify code a lot

That's right. We might have even more potential to simplify code now, since we no longer need to extract a generic Request, which means we can derive FromRequest on structs containing Request again.

@jan-auer jan-auer enabled auto-merge (squash) December 18, 2023 09:42
@jan-auer jan-auer merged commit bfa6be2 into master Dec 18, 2023
20 checks passed
@jan-auer jan-auer deleted the build/update-http-axum branch December 18, 2023 09:58
jan-auer added a commit that referenced this pull request Dec 18, 2023
@jan-auer jan-auer restored the build/update-http-axum branch December 18, 2023 13:37
jan-auer added a commit that referenced this pull request Dec 19, 2023
* master: (35 commits)
  fix(spans): Parse quotes in MySQL (#2846)
  ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter (#2849)
  fix(spans): Detect hex with fallback scrubber (#2868)
  release: 23.12.0
  Revert "ci: Update upload-artifact and download-artifact actions" (#2866)
  Revert "build: Update axum and http" (#2863)
  feat(spans): Allow resource.img spans (#2855)
  build: Update axum and http (#2844)
  fix(build): Add additional dependencies to the release build (#2858)
  ci: Update upload-artifact and download-artifact actions (#2861)
  feat(spans): Parse timestamps from strings (#2857)
  fix(spans): Scrub integer file extensions (#2856)
  feat(spans): Remove unused transaction tag from resource metrics (#2853)
  ref(cardinality): Recover buckets on cardinality limiter failure (#2852)
  feat(server): Org rate limit per metric bucket (#2836)
  ref(spans): List metric tags explicitly (#2834)
  feat(spans): Resource response sizes as measurements (#2845)
  feat(crons): Add thresholds to monitor config payload (#2842)
  feat(spans): Allow ingestion of metrics summary on spans (#2823)
  ref(crons): Add documentation to CheckInMessageType (#2840)
  ...
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.

None yet

2 participants