[router][grpc] Fix error message format in grpc chat handler#11307
[router][grpc] Fix error message format in grpc chat handler#11307
Conversation
…ate_impl` to use the same JSON format
Summary of ChangesHello @CatherineSue, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request standardizes the error response format for the gRPC chat completions endpoint to align with the OpenAI specification. It achieves this by centralizing error handling logic into new utility functions that generate structured JSON error messages, and then refactoring the existing router implementations to use these new helpers. This change ensures consistent and predictable error reporting across the gRPC interface. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively standardizes the error responses for the gRPC router endpoints to match the OpenAI format. The introduction of centralized error helper functions in src/routers/grpc/utils.rs is a great improvement for maintainability and consistency. The changes have been applied thoroughly across both pd_router.rs and router.rs, including non-streaming and streaming handlers. My review includes one suggestion to further refactor the new helper functions to reduce code duplication.
| pub fn internal_error_static(msg: &'static str) -> Response { | ||
| error!("{}", msg); | ||
| (StatusCode::INTERNAL_SERVER_ERROR, msg).into_response() | ||
| ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| Json(json!({ | ||
| "error": { | ||
| "message": msg, | ||
| "type": "internal_error", | ||
| "code": 500 | ||
| } | ||
| })), | ||
| ) | ||
| .into_response() | ||
| } | ||
|
|
||
| pub fn internal_error_message(message: String) -> Response { | ||
| error!("{}", message); | ||
| (StatusCode::INTERNAL_SERVER_ERROR, message).into_response() | ||
| ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| Json(json!({ | ||
| "error": { | ||
| "message": message, | ||
| "type": "internal_error", | ||
| "code": 500 | ||
| } | ||
| })), | ||
| ) | ||
| .into_response() | ||
| } | ||
|
|
||
| pub fn bad_request_error(message: String) -> Response { | ||
| error!("{}", message); | ||
| ( | ||
| StatusCode::BAD_REQUEST, | ||
| Json(json!({ | ||
| "error": { | ||
| "message": message, | ||
| "type": "invalid_request_error", | ||
| "code": 400 | ||
| } | ||
| })), | ||
| ) | ||
| .into_response() | ||
| } | ||
|
|
||
| pub fn service_unavailable_error(message: String) -> Response { | ||
| warn!("{}", message); | ||
| ( | ||
| StatusCode::SERVICE_UNAVAILABLE, | ||
| Json(json!({ | ||
| "error": { | ||
| "message": message, | ||
| "type": "service_unavailable", | ||
| "code": 503 | ||
| } | ||
| })), | ||
| ) | ||
| .into_response() | ||
| } |
There was a problem hiding this comment.
These new error helper functions are a great addition for standardizing error responses. However, there's a noticeable amount of code duplication among them. All four functions follow the same pattern: log a message, then construct and return a JSON response with a status code.
To improve maintainability and reduce redundancy, you could introduce a private helper function that encapsulates this common logic. This would make it easier to modify the error structure or logging behavior in the future. See the suggestion below for a possible refactoring.
fn create_error_response(
status: StatusCode,
error_type: &'static str,
code: u16,
message: String,
) -> Response {
(
status,
Json(json!({
"error": {
"message": message,
"type": error_type,
"code": code
}
})),
)
.into_response()
}
pub fn internal_error_static(msg: &'static str) -> Response {
error!("{}", msg);
create_error_response(
StatusCode::INTERNAL_SERVER_ERROR,
"internal_error",
500,
msg.to_string(),
)
}
pub fn internal_error_message(message: String) -> Response {
error!("{}", message);
create_error_response(
StatusCode::INTERNAL_SERVER_ERROR,
"internal_error",
500,
message,
)
}
pub fn bad_request_error(message: String) -> Response {
error!("{}", message);
create_error_response(
StatusCode::BAD_REQUEST,
"invalid_request_error",
400,
message,
)
}
pub fn service_unavailable_error(message: String) -> Response {
warn!("{}", message);
create_error_response(
StatusCode::SERVICE_UNAVAILABLE,
"service_unavailable",
503,
message,
)
}There was a problem hiding this comment.
Meh, will refactor later. After all, your implementation doesn't give much fewer lines.
3 participants
Motivation
This PR makes sure the error response from grpc router chat completions endpoint matches the OpenAI standard.
Modifications
Accuracy Tests
Benchmarking and Profiling
Checklist