Multiple Improvements and Bug Fixes Related to Analytics Flow #2446
Conversation
O-sura
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
📝 WalkthroughThis PR improves analytics handling across the gateway by:
Unit tests were added and updated to cover MCP analytics mapping, response content type handling, size propagation, and the revised header behavior. WalkthroughChangesThis PR bumps rate-limit policy versions in the gateway build manifest, and modifies gateway analytics logic across two packages. In Sequence Diagram(s)sequenceDiagram
participant Envoy
participant AnalyticsPolicy
participant PolicyEngine
participant Moesif
Envoy->>AnalyticsPolicy: OnResponseHeaders(content-type)
AnalyticsPolicy->>AnalyticsPolicy: derive Capability from JsonRpcMethod
AnalyticsPolicy->>AnalyticsPolicy: set definitive IsError
AnalyticsPolicy->>PolicyEngine: pass response_content_type metadata
PolicyEngine->>PolicyEngine: resolve responseContentType (metadata -> header -> Unknown)
PolicyEngine->>PolicyEngine: set requestSize, responseSize, mcpAnalytics.sessionId
PolicyEngine->>Moesif: Publish(event)
Moesif->>Moesif: populate headers only if valid JSON present
Moesif->>Moesif: attach requestSize to metadata
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
gateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif.go (1)
137-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting header-parsing logic into a helper.
The request and response header parsing blocks are nearly identical (unmarshal JSON string, check non-empty, log warnings). Extracting a shared helper like
parseHeaderProperty(event.Properties, key) map[string]interface{}would reduce duplication and ease future changes.♻️ Proposed helper extraction
+func parseHeaderProperty(properties map[string]interface{}, key string) map[string]interface{} { + result := map[string]interface{}{} + raw, ok := properties[key] + if !ok || raw == nil { + return result + } + slog.Debug("Headers (PUBLISHER): ", key, raw) + jsonStr, ok := raw.(string) + if !ok { + return result + } + var hMap map[string]interface{} + if err := json.Unmarshal([]byte(jsonStr), &hMap); err == nil && len(hMap) > 0 { + result = hMap + } else if err != nil { + slog.Warn("Failed to unmarshal headers", "key", key, "error", err) + } + return result +} + func (m *Moesif) Publish(event *dto.Event) { ... - headers := map[string]interface{}{} - if rawReqHeaders, ok := event.Properties["requestHeaders"]; ok && rawReqHeaders != nil { - ... - } - - rspHeaders := map[string]interface{}{} - if rawRspHeaders, ok := event.Properties["responseHeaders"]; ok && rawRspHeaders != nil { - ... - } + headers := parseHeaderProperty(event.Properties, "requestHeaders") + rspHeaders := parseHeaderProperty(event.Properties, "responseHeaders")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif.go` around lines 137 - 166, The request and response header parsing in moesif publisher is duplicated and should be consolidated. Extract the repeated JSON-unmarshal/empty-check/warning flow from the requestHeaders and responseHeaders blocks in the publisher logic into a shared helper (for example, a parseHeaderProperty-style function) and reuse it for both keys so future changes only need to be made once.gateway/system-policies/analytics/analytics.go (1)
769-788: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNil-check on Line 785 is now dead code.
isErroris unconditionally set on Line 778, soprops.IsErroris nevernil. Theprops.IsError != nildisjunct on Line 785 always evaluates true, making the function always return a non-nil&propsregardless ofErrorCode/ServerInfo. This matches the accompanying test expectation (propsalways non-nil), but the conditional is now vestigial and could mislead future maintainers into thinking a nil-return path still exists.♻️ Suggested simplification
- if props.IsError != nil || props.ErrorCode != nil || props.ServerInfo != nil { - return &props - } - return nil + return &props🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/system-policies/analytics/analytics.go` around lines 769 - 788, The final nil-check in the JSON-RPC response parser is dead code because props.IsError is always set in this block, so the return condition in the function that builds props should be simplified to remove the props.IsError != nil disjunct. Update the logic around the isError assignment and the final if/return path so the condition only reflects the fields that can actually be nil, keeping the behavior of returning &props when any meaningful response data is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@gateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif.go`:
- Around line 137-166: The request and response header parsing in moesif
publisher is duplicated and should be consolidated. Extract the repeated
JSON-unmarshal/empty-check/warning flow from the requestHeaders and
responseHeaders blocks in the publisher logic into a shared helper (for example,
a parseHeaderProperty-style function) and reuse it for both keys so future
changes only need to be made once.
In `@gateway/system-policies/analytics/analytics.go`:
- Around line 769-788: The final nil-check in the JSON-RPC response parser is
dead code because props.IsError is always set in this block, so the return
condition in the function that builds props should be simplified to remove the
props.IsError != nil disjunct. Update the logic around the isError assignment
and the final if/return path so the condition only reflects the fields that can
actually be nil, keeping the behavior of returning &props when any meaningful
response data is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e935065-019f-4972-af6f-42190177712b
📒 Files selected for processing (7)
gateway/build-manifest.yamlgateway/gateway-runtime/policy-engine/internal/analytics/analytics.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.gogateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif.gogateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif_test.gogateway/system-policies/analytics/analytics.gogateway/system-policies/analytics/analytics_test.go
2 participants
Purpose
This PR will contain the following bug-fixes/improvements to the existing analytics publishing flow
sessionIdto match API ManagerCapability,isErrorresponseContentType,requestSizemissing for all API types related analytics eventsanalytics-header-filterpolicy is configured)sessionIdto match API Manager, notmcp_session_id#2389