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

Feat before request #1125

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tengfeisky
Copy link

Description

This PR adds request parameter to after_request middleware

Summary

This PR adds the request parameter to the after_request middleware function to enable proper propagation of trace_id between request and response headers. This change supports distributed tracing functionality by ensuring that the trace_id remains consistent throughout the request lifecycle and can be properly passed to downstream services.

PR Checklist

Please ensure that:

  • The PR contains a descriptive title
  • The PR contains a descriptive summary of the changes
  • You build and test your changes before submitting a PR.
  • You have added relevant documentation
  • You have added relevant tests. We prefer integration tests wherever possible

Pre-Commit Instructions:

Copy link

vercel bot commented Feb 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
robyn ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 19, 2025 10:21am

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Hey @tengfeisky 👋

Thanks for the PR :D I have some comments. Could you have a look?


@app.get("/sync/global/middlewares/with_request")
def sync_global_middlewares_with_request(request: Request):
print(request.headers)
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

Used for testing


@app.get("/async/global/middlewares/with_request")
def sync_global_middlewares_with_request(request: Request):
print(request.headers)
Copy link
Member

Choose a reason for hiding this comment

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

same question here.

Comment on lines +44 to +48
if let Ok(tuple) = function_args.downcast::<PyTuple>(py) {
handler.call(tuple, Some(kwargs))
} else {
handler.call((function_args,), Some(kwargs))
}
Copy link
Member

Choose a reason for hiding this comment

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

can you please explain this line of code to me?

Copy link
Author

Choose a reason for hiding this comment

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

In Rust, this syntax (function_args,) creates a nested tuple in Python, similar to ((arg1,arg2),).

@@ -1,5 +1,6 @@
use crate::executors::{
execute_http_function, execute_middleware_function, execute_startup_handler,
execute_http_function, execute_middleware_after_request, execute_middleware_function,
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do slightly better here.

i.e. execute_middleware_after_request and execute_middleware_function are not correct nomenclature imo.

Copy link
Author

Choose a reason for hiding this comment

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

What is your suggestion then?

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.

2 participants