feat:support gateway API information #621
Conversation
Summary of ChangesHello @FAUST-BENCHOU, 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 introduces a significant feature to enhance the observability of request routing by integrating Kubernetes Gateway API information directly into the access logs. By capturing and logging details such as the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 adds support for logging Gateway API information (Gateway, HTTPRoute, InferencePool) in the access logs. The changes span across the access logging and routing components, including updating data structures, log formatting, and introducing logic to capture this information from the request context. The tests have been updated accordingly to cover the new fields. My feedback focuses on improving code maintainability and performance by refactoring repetitive code blocks and using more efficient string building techniques.
| if entry.Gateway != "" { | ||
| line += fmt.Sprintf(" gateway=%s", entry.Gateway) | ||
| } | ||
| if entry.HTTPRoute != "" { | ||
| line += fmt.Sprintf(" http_route=%s", entry.HTTPRoute) | ||
| } | ||
| if entry.InferencePool != "" { | ||
| line += fmt.Sprintf(" inference_pool=%s", entry.InferencePool) | ||
| } |
There was a problem hiding this comment.
This block of code, along with other parts of the function, repeatedly concatenates strings using +=. This can be inefficient as it may lead to multiple memory allocations for each concatenation. For better performance and readability, consider refactoring the formatText function to use a strings.Builder.
Here's an example of how the whole function could be improved:
import (
"fmt"
"strings"
"time"
)
func (l *accessLoggerImpl) formatText(entry *AccessLogEntry) (string, error) {
var sb strings.Builder
timestamp := entry.Timestamp.Format(time.RFC3339Nano)
fmt.Fprintf(&sb, `[%s] "%s %s %s" %d`,
timestamp, entry.Method, entry.Path, entry.Protocol,
entry.StatusCode)
if entry.Error != nil {
fmt.Fprintf(&sb, " error=%s:%s", entry.Error.Type, entry.Error.Message)
}
appendKV := func(k, v string) {
if v != "" {
fmt.Fprintf(&sb, " %s=%s", k, v)
}
}
appendKV("model_name", entry.ModelName)
appendKV("model_route", entry.ModelRoute)
appendKV("model_server", entry.ModelServer)
appendKV("selected_pod", entry.SelectedPod)
appendKV("request_id", entry.RequestID)
appendKV("gateway", entry.Gateway)
appendKV("http_route", entry.HTTPRoute)
appendKV("inference_pool", entry.InferencePool)
if entry.InputTokens > 0 || entry.OutputTokens > 0 {
fmt.Fprintf(&sb, " tokens=%d/%d", entry.InputTokens, entry.OutputTokens)
}
fmt.Fprintf(&sb, " timings=%dms(%d+%d+%d)",
entry.DurationTotal,
entry.DurationRequestProcessing,
entry.DurationUpstreamProcessing,
entry.DurationResponseProcessing)
return sb.String(), nil
}This approach builds the string more efficiently in a single buffer and is more maintainable.
| var gatewayKeyForLog, httpRouteKey, inferencePoolKey string | ||
| if key, exists := c.Get(GatewayKey); exists { | ||
| if k, ok := key.(string); ok { | ||
| gatewayKeyForLog = k | ||
| } | ||
| } | ||
| if httpRouteName, exists := c.Get("httpRouteName"); exists { | ||
| if name, ok := httpRouteName.(types.NamespacedName); ok { | ||
| httpRouteKey = fmt.Sprintf("%s/%s", name.Namespace, name.Name) | ||
| } | ||
| } | ||
| if inferencePoolName, exists := c.Get("inferencePoolName"); exists { | ||
| if name, ok := inferencePoolName.(types.NamespacedName); ok { | ||
| inferencePoolKey = fmt.Sprintf("%s/%s", name.Namespace, name.Name) | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code has some opportunities for improvement:
- Repetitive Logic: The pattern of getting a value from
gin.Context, checking for existence, and type-asserting is repeated. This can be extracted into helper functions to reduce duplication and improve readability. - Idiomatic Formatting:
fmt.Sprintf("%s/%s", name.Namespace, name.Name)can be replaced with the more idiomaticname.String()method fromtypes.NamespacedName.
Consider refactoring this into helper functions like so:
func getStringFromContext(c *gin.Context, key string) string {
if val, exists := c.Get(key); exists {
if str, ok := val.(string); ok {
return str
}
}
return ""
}
func getNamespacedNameStringFromContext(c *gin.Context, key string) string {
if val, exists := c.Get(key); exists {
if name, ok := val.(types.NamespacedName); ok {
return name.String()
}
}
return ""
}Using these helpers, the block could be simplified to:
// Set Gateway API info from context
gatewayKeyForLog := getStringFromContext(c, GatewayKey)
httpRouteKey := getNamespacedNameStringFromContext(c, "httpRouteName")
inferencePoolKey := getNamespacedNameStringFromContext(c, "inferencePoolName")
|
/assign @YaoZengzeng |
| ctx.Gateway = "default/test-gateway" | ||
| ctx.HTTPRoute = "default/test-httproute" | ||
| ctx.InferencePool = "default/test-inferencepool" | ||
| assert.Equal(t, "default/test-gateway", ctx.Gateway) |
There was a problem hiding this comment.
Can't understand why check here? Just set the value above?
pkg/kthena-router/router/router.go
Outdated
| gatewayKeyForLog = k | ||
| } | ||
| } | ||
| if httpRouteName, exists := c.Get("httpRouteName"); exists { |
There was a problem hiding this comment.
Use constant like GatewayKey.
pkg/kthena-router/router/router.go
Outdated
| httpRouteKey = fmt.Sprintf("%s/%s", name.Namespace, name.Name) | ||
| } | ||
| } | ||
| if inferencePoolName, exists := c.Get("inferencePoolName"); exists { |
|
@FAUST-BENCHOU Add more E2E cases to cover could be better :) |
| // Add Gateway API fields | ||
| if entry.Gateway != "" { | ||
| line += fmt.Sprintf(" gateway=%s", entry.Gateway) | ||
| } | ||
| if entry.HTTPRoute != "" { | ||
| line += fmt.Sprintf(" http_route=%s", entry.HTTPRoute) | ||
| } | ||
| if entry.InferencePool != "" { | ||
| line += fmt.Sprintf(" inference_pool=%s", entry.InferencePool) |
There was a problem hiding this comment.
can you move these after modelRoute and ModelServer above
| inferencePoolKey = fmt.Sprintf("%s/%s", name.Namespace, name.Name) | ||
| } | ||
| } | ||
| accesslog.SetGatewayAPIInfo(c, gatewayKeyForLog, httpRouteKey, inferencePoolKey) |
There was a problem hiding this comment.
@YaoZengzeng We support mix using gateway httpRoute and modelServer， right？
There was a problem hiding this comment.
First attempt to match ModelRoute → ModelServer; if that fails, fall back to HTTPRoute → InferencePool. Both share the same gatewayKey context.
FYI:
kthena/pkg/kthena-router/router/router.go
Lines 285 to 305 in 9e1aa64
FAUST-BENCHOU
force-pushed
the
feat/gatewayapi
branch
from
b1830a9 to
383c71b
Compare
@YaoZengzeng I would like to use e2e tests to verify that the following information is correctly logged in the access logs:
However, the output fields in my tests are always quite simple (not even including the modelname). Here's a brief overview of my testing approach. Could you help me identify any issues or suggest better approaches?
However, the log output I see is: I have already spent quite a bit of time on this since yesterday, but I still don't have much progress. Would you be able to guide me through this or provide any suggestions to help resolve it? you can see my TestAccessLogGatewayInfo test here and my TestAccessLogGatewayInfo results here |
|
|
||
| // Set Gateway API info from context | ||
| var gatewayKeyForLog, httpRouteKey, inferencePoolKey string | ||
| if key, exists := c.Get(GatewayKey); exists { |
There was a problem hiding this comment.
nit: In my opinion, the operations of L393-L407 are similar; can they be abstracted into a function?
You should check following three scenarios:
Might have bug if none of gateway, modelroute, modelserver, httproute, inferencepool are seen ... |
|
@FAUST-BENCHOU also you could directly push the code of E2E into this PR and I could help with debugging. |
FAUST-BENCHOU
force-pushed
the
feat/gatewayapi
branch
from
383c71b to
ee77e55
Compare
|
@YaoZengzeng I find now the e2e test can pass.it seems it's LLM-Mock's problems before.But anyway acesslog works well now |
| require.NotNil(t, accessCtx, "Access log context should be set") | ||
|
|
||
| // Verify AI-specific routing information | ||
| assert.Equal(t, "test-model", accessCtx.ModelName, "ModelName should be set from request") |
There was a problem hiding this comment.
Actually ModelRoute/ModelServer and HTTPRoute/InferencePool are mutually exclusive; they can't be set simultaneously.
This is also the key point of the test.
Example ref: https://github.com/volcano-sh/kthena/blob/main/test/e2e/router/gateway-inference-extension/e2e_test.go#L143
And it will be easy to test in E2E.
Signed-off-by: zhoujinyu <[email protected]>
Signed-off-by: zhoujinyu <[email protected]>
FAUST-BENCHOU
force-pushed
the
feat/gatewayapi
branch
from
ee77e55 to
b633d03
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
i add test accesslog part in TestBothAPIsConfigured |
5 participants
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #617
Special notes for your reviewer:
i add a TestRouter_HandlerFunc_AccessLogRoutingInfo to make sure accesslog can work well in router
Does this PR introduce a user-facing change?: