-
Notifications
You must be signed in to change notification settings - Fork 92
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
Conversation
There are still test failures with:
|
There was a problem hiding this 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.
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- In which order the middlewares are registered and why
- 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.
fn method(&self) -> Method { | ||
self.method.clone() | ||
fn method(&self) -> reqwest::Method { | ||
self.method.as_str().parse().unwrap() |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
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 |
* 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) ...
We would like to update the Sentry SDK, which requires to update
axum
and related dependencies. Asreqwest
has not updated to the latest version ofhttp
andhyper
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 theaxum
repository.#skip-changelog