enhancement: forward tags in maxim logs through headers#466
enhancement: forward tags in maxim logs through headers#466Pratham-Mishra04 merged 1 commit intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
TejasGhatte
changed the title
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds context-driven trace/generation naming and tag aggregation for Maxim: HTTP layer now collects x-bf-maxim-* headers into a tags map and explicit fields; Maxim PreHook reads them, augments tags, extracts latest chat input, and conditionally creates trace/generation unless pre-existing IDs are present. Multiple modules bump version strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant H as Bifrost HTTP
participant Ctx as ConvertToBifrostContext
participant P as Maxim PreHook
participant L as Logging Service
C->>H: HTTP request with headers (x-bf-maxim-*)
H->>Ctx: Build context from RequestCtx
Note over Ctx: Extract explicit IDs<br/>Collect other x-bf-maxim-* into maxim-tags<br/>Set TraceName/GenerationName keys
Ctx-->>H: *context.Context
H->>P: Invoke PreHook(ctx, req)
alt GenerationID present in ctx
P-->>H: Early return (no trace/gen create)
else No GenerationID
P->>P: Determine requestType, latest chat text
P->>P: Merge tags (model, action, provided tags)
P->>L: Create Trace (name from requestType or TraceName)
L-->>P: TraceID
P->>L: Create Generation (use GenerationName if provided)
L-->>P: GenerationID
P-->>H: Updated ctx with TraceID/GenerationID
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pratham-Mishra04
left a comment
Pratham-Mishra04
left a comment
There was a problem hiding this comment.
one small change
TejasGhatte
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
from
dd0fc66 to
14d7cd1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/maxim/main.go (1)
171-187: Nil deref risk: lastMsg.Content may be nil.Add a guard before accessing
ContentStr/ContentBlocksto prevent panics on malformed inputs.- lastMsg := (*req.Input.ChatCompletionInput)[len(*req.Input.ChatCompletionInput)-1] - if lastMsg.Content.ContentStr != nil { + lastMsg := (*req.Input.ChatCompletionInput)[len(*req.Input.ChatCompletionInput)-1] + if lastMsg.Content != nil && lastMsg.Content.ContentStr != nil { latestMessage = *lastMsg.Content.ContentStr - } else if lastMsg.Content.ContentBlocks != nil { + } else if lastMsg.Content != nil && lastMsg.Content.ContentBlocks != nil { // Find the last text content block for i := len(*lastMsg.Content.ContentBlocks) - 1; i >= 0; i-- { block := (*lastMsg.Content.ContentBlocks)[i] if block.Type == "text" && block.Text != nil { latestMessage = *block.Text break } } // If no text block found, use placeholder if latestMessage == "" { latestMessage = "-" } + } else { + latestMessage = "-" }
♻️ Duplicate comments (1)
plugins/maxim/main.go (1)
63-64: Confirm single source of truth for tags key; ensure no leftover duplicates.Looks good to standardize on
TagsKey = "maxim-tags". Please confirm there are no remainingMaximTagsKeydeclarations/usages in the repo and that transports set the value usingmaxim.ContextKey("maxim-tags").Run:
#!/bin/bash # Find any duplicate/legacy keys rg -nP -C2 '\b(MaximTagsKey|TagsKey)\b|ContextKey\("maxim-tags"\)|"maxim-tags"' # Spot other ContextKey definitions referencing maxim tags rg -nP -C3 'type\s+ContextKey\s+string|maxim-tags'
🧹 Nitpick comments (2)
plugins/maxim/main.go (2)
162-163: Use constants for action tag values to avoid typos and enable reuse.Example:
- tags["action"] = "chat_completion" + const tagActionChat = "chat_completion" + tags["action"] = tagActionChat ... - tags["action"] = "text_completion" + const tagActionText = "text_completion" + tags["action"] = tagActionTextAlso applies to: 191-192
149-149: Default requestType to a non-empty value.- var requestType string + requestType := "unknown"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- MCP integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
plugins/maxim/main.go(4 hunks)transports/bifrost-http/lib/ctx.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/bifrost-http/lib/ctx.go
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/maxim/main.go (1)
transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
🔇 Additional comments (3)
plugins/maxim/main.go (3)
118-118: LGTM: local tags map introduction is clear.
153-160: LGTM: always include the model tag; initializes map when nil.
162-163: LGTM: sets action tag for both request types.Also applies to: 191-192
TejasGhatte
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
from
14d7cd1 to
e52b553
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/lib/ctx.go (1)
92-114: Fix key normalization: x-bf-maxim-TraceName/GenerationName not recognized (treated as tags).Because
keyStris lowercased,x-bf-maxim-TraceNamebecomestracenameand won’t match"trace-name". Same forGenerationNameand the*-IDkeys. These end up in the tags map instead of the dedicated context keys.Patch to accept both hyphen-case and camel/pascal-case forms:
@@ - if strings.HasPrefix(keyStr, "x-bf-maxim-") { - labelName := strings.TrimPrefix(keyStr, "x-bf-maxim-") + if strings.HasPrefix(keyStr, "x-bf-maxim-") { + labelName := strings.TrimPrefix(keyStr, "x-bf-maxim-") + // normalize for matching: remove separators; labelName is already lowercased + norm := strings.ReplaceAll(strings.ReplaceAll(labelName, "-", ""), "_", "") @@ - if labelName == string(maxim.GenerationIDKey) { + if labelName == string(maxim.GenerationIDKey) || norm == "generationid" { bifrostCtx = context.WithValue(bifrostCtx, maxim.ContextKey(labelName), string(value)) } @@ - if labelName == string(maxim.TraceIDKey) { + if labelName == string(maxim.TraceIDKey) || norm == "traceid" { bifrostCtx = context.WithValue(bifrostCtx, maxim.ContextKey(labelName), string(value)) } @@ - if labelName == string(maxim.SessionIDKey) { + if labelName == string(maxim.SessionIDKey) || norm == "sessionid" { bifrostCtx = context.WithValue(bifrostCtx, maxim.ContextKey(labelName), string(value)) } @@ - if labelName == string(maxim.TraceNameKey) { + if labelName == string(maxim.TraceNameKey) || norm == "tracename" { bifrostCtx = context.WithValue(bifrostCtx, maxim.ContextKey(labelName), string(value)) } @@ - if labelName == string(maxim.GenerationNameKey) { + if labelName == string(maxim.GenerationNameKey) || norm == "generationname" { bifrostCtx = context.WithValue(bifrostCtx, maxim.ContextKey(labelName), string(value)) }
♻️ Duplicate comments (1)
plugins/maxim/main.go (1)
148-157: Sanitize and bound incoming header tags before copying (security/perf).Still copies untrusted keys/values verbatim. Apply bounded, normalized merge to prevent oversized or reserved key injection. This mirrors prior feedback.
@@ - // retrieve all tags from context - // the transport layer now stores all maxim tags in a single map - if tagsValue := (*ctx).Value(TagsKey); tagsValue != nil { - if tagsMap, ok := tagsValue.(map[string]string); ok { - tags = make(map[string]string) - for key, value := range tagsMap { - tags[key] = value - } - } - } + // retrieve and sanitize tags from context (transport aggregates them) + if tagsValue := (*ctx).Value(TagsKey); tagsValue != nil { + if tagsMap, ok := tagsValue.(map[string]string); ok { + const maxIncomingTags = 50 + const maxKeyLen = 64 + const maxValLen = 256 + reserved := map[string]struct{}{"model": {}, "action": {}} + + tags = make(map[string]string) + count := 0 + for k, v := range tagsMap { + lk := strings.ToLower(strings.TrimSpace(k)) + // whitelist [a-z0-9._-] + buf := make([]byte, 0, len(lk)) + for i := 0; i < len(lk); i++ { + c := lk[i] + if (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '.' || c == '_' || c == '-' { + buf = append(buf, c) + } + } + if len(buf) == 0 { + continue + } + if len(buf) > maxKeyLen { + buf = buf[:maxKeyLen] + } + key := string(buf) + if _, isReserved := reserved[key]; isReserved { + continue + } + val := strings.TrimSpace(v) + if len(val) > maxValLen { + val = val[:maxValLen] + } + tags[key] = val + count++ + if count >= maxIncomingTags { + break + } + } + } + }Add import:
@@ -import ( +import ( "context" "encoding/json" "fmt" + "strings"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- MCP integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
plugins/maxim/main.go(6 hunks)plugins/maxim/version(1 hunks)transports/bifrost-http/lib/ctx.go(2 hunks)transports/version(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- plugins/maxim/version
- transports/version
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/maxim/main.go (1)
transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)
transports/bifrost-http/lib/ctx.go (1)
plugins/maxim/main.go (6)
TraceNameKey(62-62)ContextKey(54-54)GenerationNameKey(64-64)GenerationIDKey(63-63)TraceIDKey(61-61)SessionIDKey(60-60)
🔇 Additional comments (6)
plugins/maxim/main.go (6)
62-66: New context keys LGTM; ensure transport uses TagsKey constant.The additions look good. Confirm
transports/bifrost-http/lib/ctx.gousesmaxim.TagsKey(not a string literal) to avoid mismatches. See my transport comment.
118-123: Variable additions LGTM.Introducing
traceName,generationName, andtagshere is clean and keeps the hook state self-contained.
140-147: Reading names from context LGTM.This correctly prefers context overrides for trace/generation names. Interop depends on transport recognizing both hyphen and camel forms; addressed in transport comment.
165-172: Ensure reserved tags always win.Setting
tags["model"] = req.Modelafter merging guarantees override. Consider doing the same foraction(you already do it in each branch).
214-222: Trace naming fallback LGTM.Good default and override behavior; avoids empty names.
246-260: Generation config LGTM.Passing
Tags: &tagsensures all accumulated tags land on the generation. Name override is correct.
TejasGhatte
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
from
e52b553 to
2943c90
Compare
TejasGhatte
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
from
2943c90 to
58ea6bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
transports/bifrost-http/lib/ctx.go (1)
115-120: Sanitize and bound header-derived tag keys/values (security/perf).Normalize, filter, cap, and limit count; skip reserved keys.
- // apart from these all headers starting with x-bf-maxim- are keys for tags - // collect them in the maximTags map - if labelName != string(maxim.GenerationIDKey) && labelName != string(maxim.TraceIDKey) && labelName != string(maxim.SessionIDKey) && labelName != string(maxim.TraceNameKey) && labelName != string(maxim.GenerationNameKey) { - maximTags[labelName] = string(value) - } + // Apart from explicit keys, treat remaining x-bf-maxim-* headers as tags (sanitized/bounded) + if labelName != string(maxim.GenerationIDKey) && labelName != string(maxim.TraceIDKey) && labelName != string(maxim.SessionIDKey) && labelName != string(maxim.TraceNameKey) && labelName != string(maxim.GenerationNameKey) { + if maximTags == nil { + maximTags = make(map[string]string) + } + const maxIncomingTags = 50 + const maxKeyLen = 64 + const maxValLen = 256 + if len(maximTags) >= maxIncomingTags { + return true + } + // normalize and sanitize key (keyStr is already lower-cased) + k := labelName + kb := make([]byte, 0, len(k)) + for i := 0; i < len(k); i++ { + c := k[i] + if (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '.' || c == '_' || c == '-' { + kb = append(kb, c) + } + } + if len(kb) == 0 || len(kb) > maxKeyLen { + return true + } + // prevent reserved keys from being injected + if string(kb) == "model" || string(kb) == "action" { + return true + } + v := strings.TrimSpace(string(value)) + if len(v) > maxValLen { + v = v[:maxValLen] + } + maximTags[string(kb)] = v + }plugins/maxim/main.go (1)
148-157: Sanitize and bound incoming tag keys/values from context (security/perf).Headers are untrusted; tags can be huge or inject reserved keys. Normalize to lowercase, filter allowed chars, cap lengths and count, and skip reserved keys.
Apply this diff and add strings import:
@@ import ( "context" "encoding/json" "fmt" + "strings" @@ - // retrieve all tags from context - // the transport layer now stores all maxim tags in a single map - if tagsValue := (*ctx).Value(TagsKey); tagsValue != nil { - if tagsMap, ok := tagsValue.(map[string]string); ok { - tags = make(map[string]string) - for key, value := range tagsMap { - tags[key] = value - } - } - } + // Retrieve and sanitize tags from context (transport aggregates x-bf-maxim-* here) + if tagsValue := (*ctx).Value(TagsKey); tagsValue != nil { + if tagsMap, ok := tagsValue.(map[string]string); ok { + const maxIncomingTags = 50 + const maxKeyLen = 64 + const maxValLen = 256 + reserved := map[string]struct{}{"model": {}, "action": {}} + tags = make(map[string]string, len(tagsMap)) + count := 0 + for k, v := range tagsMap { + lk := strings.ToLower(strings.TrimSpace(k)) + if _, isReserved := reserved[lk]; isReserved { + continue + } + // allow only [a-z0-9._-] in keys + kb := make([]byte, 0, len(lk)) + for i := 0; i < len(lk); i++ { + c := lk[i] + if (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || c == '.' || c == '_' || c == '-' { + kb = append(kb, c) + } + } + if len(kb) == 0 || len(kb) > maxKeyLen { + continue + } + val := strings.TrimSpace(v) + if len(val) > maxValLen { + val = val[:maxValLen] + } + tags[string(kb)] = val + count++ + if count >= maxIncomingTags { + break + } + } + } + }
🧹 Nitpick comments (4)
plugins/maxim/main.go (1)
214-218: Fallback name when requestType is empty.If neither ChatCompletionInput nor TextCompletionInput is set, name becomes "bifrost_". Provide a sane default.
- name := fmt.Sprintf("bifrost_%s", requestType) + rt := requestType + if rt == "" { rt = "unknown" } + name := fmt.Sprintf("bifrost_%s", rt)transports/bifrost-http/lib/ctx.go (3)
80-82: Lazy-allocate the tag map.Avoid alloc when no x-bf-maxim-* headers are present.
- // Initialize tags map for collecting maxim tags - maximTags := make(map[string]string) + // Initialize tags map for collecting maxim tags (lazy allocate) + var maximTags map[string]string
194-197: Use the public key constant directly.Minor clarity nit; avoid redundant type conversion.
- if len(maximTags) > 0 { - bifrostCtx = context.WithValue(bifrostCtx, maxim.ContextKey(maxim.TagsKey), maximTags) - } + if len(maximTags) > 0 { + bifrostCtx = context.WithValue(bifrostCtx, maxim.TagsKey, maximTags) + }
115-120: Align transport and plugin tag sanitization & limitsTransport stores maxim tags under maxim.TagsKey — verified. Ensure reserved keys/prefix (x-bf-maxim-), and any length/character limits are identical in transport and plugin to avoid drift. (plugins/maxim/main.go:65, transports/bifrost-http/lib/ctx.go:196)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/version(1 hunks)framework/version(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/main.go(6 hunks)plugins/maxim/version(1 hunks)plugins/mocker/version(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/version(1 hunks)transports/bifrost-http/lib/ctx.go(3 hunks)transports/version(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- plugins/semanticcache/version
- plugins/governance/version
- framework/version
- plugins/mocker/version
- plugins/jsonparser/version
- plugins/logging/version
- plugins/telemetry/version
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/maxim/version
- transports/version
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/maxim/main.go (3)
transports/bifrost-http/lib/ctx.go (1)
ContextKey(68-68)transports/bifrost-http/handlers/completions.go (1)
CompletionRequest(69-99)core/schemas/bifrost.go (1)
ModelParameters(225-242)
transports/bifrost-http/lib/ctx.go (1)
plugins/maxim/main.go (7)
TraceNameKey(62-62)ContextKey(54-54)GenerationNameKey(64-64)GenerationIDKey(63-63)TraceIDKey(61-61)SessionIDKey(60-60)TagsKey(65-65)
🔇 Additional comments (7)
core/version (1)
1-1: Patch version bump looks good.No functional impact from this change.
plugins/maxim/main.go (5)
62-66: Public context keys: OK.Clear naming; no duplicate of the old MaximTagsKey remains.
165-172: Initialize tags and enrich with model: OK.Also ensures reserved key “model” cannot be overridden by headers.
174-174: Action tag assignment: OK.Consistently sets action for both paths.
Also applies to: 203-204
246-257: Generation config build looks good.Passing Tags by reference is fine given no subsequent mutation.
259-259: AddGenerationToTrace usage: OK.Matches traceID handling above.
transports/bifrost-http/lib/ctx.go (1)
107-114: Trace/generation name headers: OK.Keys align with maxim.TraceNameKey and maxim.GenerationNameKey.
TejasGhatte
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
from
58ea6bf to
c2ccac0
Compare
TejasGhatte
changed the base branch from
graphite-base/466
to
09-02-feat_redis_as_vector_store
TejasGhatte
changed the base branch from
09-02-feat_redis_as_vector_store
to
graphite-base/466
TejasGhatte
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
from
c2ccac0 to
4abd62c
Compare
TejasGhatte
force-pushed
the
graphite-base/466
branch
from
f8f10ca to
1b4a949
Compare
TejasGhatte
changed the base branch from
graphite-base/466
to
09-02-feat_redis_as_vector_store
TejasGhatte
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
2 times, most recently
from
cab29ff to
bd0e774
Compare
TejasGhatte
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
from
bd0e774 to
f6c8f54
Compare
Merge activity
|
Pratham-Mishra04
changed the base branch from
09-02-feat_redis_as_vector_store
to
graphite-base/466
Pratham-Mishra04
force-pushed
the
09-13-enhancement_forward_tags_in_maxim_logs
branch
from
f6c8f54 to
9f31ae4
Compare
Pratham-Mishra04
deleted the
09-13-enhancement_forward_tags_in_maxim_logs
branch
## Summary Enhance Maxim plugin to support custom tags from HTTP headers and propagate them through the context. Solves #341 ## Changes - Added `TagsKey` and `MaximTagsKey` context keys to the Maxim plugin - Modified the HTTP transport to collect custom Maxim tags from headers with the `x-bf-maxim-` prefix - Enhanced the PreHook function to retrieve existing tags from context and merge them with request-specific tags - Standardized tag handling by storing all Maxim tags in a single map under the `maxim-tags` context key - Improved tag organization by always adding model and action tags regardless of request type ## Type of change - [x] Feature - [x] Refactor ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Plugins ## How to test Test by sending requests with custom Maxim tags in headers: ```sh # Send a request with custom Maxim tags curl -X POST http://localhost:8000/v1/chat/completions \ -H "Content-Type: application/json" \ -H "x-bf-maxim-custom-tag: test-value" \ -H "x-bf-maxim-environment: production" \ -d '{"model": "gpt-3.5-turbo", "messages": [{"role": "user", "content": "Hello"}]}' # Verify tags are properly logged in Maxim logs ``` ## Breaking changes - [x] No ## Related issues Improves logging capabilities by allowing custom tags to be passed from HTTP headers to Maxim. ## Security considerations Custom tags from headers should be validated to prevent injection of malicious data into logs. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI)
2 participants
Summary
Enhance Maxim plugin to support custom tags from HTTP headers and propagate them through the context.
Solves #341
Changes
TagsKeyandMaximTagsKeycontext keys to the Maxim pluginx-bf-maxim-prefixmaxim-tagscontext keyType of change
Affected areas
How to test
Test by sending requests with custom Maxim tags in headers:
Breaking changes
Related issues
Improves logging capabilities by allowing custom tags to be passed from HTTP headers to Maxim.
Security considerations
Custom tags from headers should be validated to prevent injection of malicious data into logs.
Checklist