Fix issue of emitting two analytics event per LLM proxy call - #2879
Merged
Conversation
Tharanidk
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Induwara04,
Krishanx92,
PasanT9,
Piumal1999,
RakhithaRR,
Tharsanan1,
Thushani-Jayasekera,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
lasanthaS,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub,
tharindu1st and
thivindu
as code owners
July 27, 2026 02:26
Contributor
📝 WalkthroughWalkthroughLLM proxy operations now mark internal loopback requests, analytics suppresses duplicate loopback provider events using direct peer addresses, and Moesif includes AI metadata for provider and proxy events. Tests cover marker propagation, suppression, address validation, logging, and publisher metadata. ChangesLLM loopback analytics handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LLMTransformer
participant AnalyticsPolicy
participant PolicyEngine
participant Moesif
LLMTransformer->>AnalyticsPolicy: add internal loopback header
AnalyticsPolicy->>PolicyEngine: stamp loopback analytics metadata
PolicyEngine->>PolicyEngine: detect direct loopback provider event
PolicyEngine-->>Moesif: publish non-suppressed event with AI metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif_test.go (1)
295-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse fatal assertions before dependent access.
assert.Lenandassert.NotNildo not stop execution. A regression can therefore panic atmoesif.events[0]or a type assertion, obscuring the actual failure. Userequireguards or comma-ok type assertions.Proposed test hardening
- assert.Len(t, moesif.events, 1) + require.Len(t, moesif.events, 1) metadata := getMetadata(moesif.events[0]) - assert.NotNil(t, metadata["aiMetadata"]) - assert.NotNil(t, metadata["aiTokenUsage"]) + require.NotNil(t, metadata["aiMetadata"]) + require.NotNil(t, metadata["aiTokenUsage"])Add the
requireimport from the existing testify dependency.🤖 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_test.go` around lines 295 - 305, Harden the test assertions around moesif.events and metadata in the current test by importing testify/require, replacing the event-count and nil checks with fatal require assertions before indexing or type assertions, and validating type assertions with require so regressions report assertion failures instead of panicking.
🤖 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_test.go`:
- Around line 295-305: Harden the test assertions around moesif.events and
metadata in the current test by importing testify/require, replacing the
event-count and nil checks with fatal require assertions before indexing or type
assertions, and validating type assertions with require so regressions report
assertion failures instead of panicking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d16d2622-41da-45e1-8040-36084e2937f1
📒 Files selected for processing (4)
gateway/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.go
O-sura
reviewed
Jul 28, 2026
O-sura
reviewed
Jul 28, 2026
O-sura
requested changes
Jul 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@gateway/system-policies/analytics/analytics.go`:
- Around line 176-180: Stop treating InternalLoopbackMetadataKey from
reqCtx.Headers as trusted in the analytics policy path. Strip or reserve this
header at the public request boundary, and have the proxy-to-provider forwarding
path attach an unforgeable internal marker that runtime can recognize before
setting analyticsMetadata. Add a regression test covering a direct LlmProvider
loopback request with an injected marker and verify its event is not dropped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb5ece17-3c01-4c66-97b3-32e7b6509f48
📒 Files selected for processing (12)
gateway/gateway-controller/pkg/constants/constants.gogateway/gateway-controller/pkg/utils/llm_deployment_policy_test.gogateway/gateway-controller/pkg/utils/llm_deployment_test.gogateway/gateway-controller/pkg/utils/llm_transformer.gogateway/gateway-controller/pkg/utils/llm_transformer_multiprovider_test.gogateway/gateway-controller/pkg/utils/llm_transformer_test.gogateway/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
🚧 Files skipped from review as they are similar to previous changes (2)
- gateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif_test.go
- gateway/gateway-runtime/policy-engine/internal/analytics/publishers/moesif.go
Contributor
|
The latest changes fixed the issue highlighted here in #2879 (comment) but it introduces another issue. The marker header isn't stripped at ingress, and # baseline → 1 analytics event
curl -k -sS -o /dev/null -w 'HTTP=%{http_code}\n' -X POST \
https://localhost:8443/openai/latest/chat/completions \
-H 'Content-Type: application/json' \
-d '{"model":"gpt-4o-mini","messages":[{"role":"user","content":"Hi"}]}'
# same call + two headers → HTTP 200, ZERO analytics events
curl -k -sS -o /dev/null -w 'HTTP=%{http_code}\n' -X POST \
https://localhost:8443/openai/latest/chat/completions \
-H 'Content-Type: application/json' \
-H 'x-wso2-internal-loopback: 1' \
-H 'X-Forwarded-For: 127.0.0.1' \
-d '{"model":"gpt-4o-mini","messages":[{"role":"user","content":"Hi"}]}'Possible fix would be to use DownstreamDirectRemoteAddress field from the access log instead of the DownstreamRemoteAddress when reading the IP |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@gateway/gateway-runtime/policy-engine/internal/analytics/analytics.go`:
- Around line 383-390: Prevent client-controlled InternalLoopbackMetadataKey
values from influencing analytics by stripping or overwriting the marker at the
client-facing ingress boundary and injecting it only from a trusted proxy hop,
or validating its authenticity before metadata creation. Update the relevant
ingress metadata handling and add a regression test covering a loopback
directRemoteIP with a client-supplied marker, ensuring the LlmProvider event is
not discarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ac0a846-4e93-4cc1-a2a6-aaa83a76a8ac
📒 Files selected for processing (3)
gateway/gateway-controller/pkg/eventlistener/llm_provider_processor_test.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.go
O-sura
reviewed
Jul 28, 2026
O-sura
reviewed
Jul 28, 2026
O-sura
previously approved these changes
Jul 28, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.go (1)
397-403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSerialize logger replacement or inject a logger.
captureLogschanges the process-wide default logger, and this package has not.Parallelusage to isolate the test suite, but it still makes log-capture tests order/interference-sensitive. Pass the handler/logger explicitly or run the affected tests sequentially.🤖 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/analytics_test.go` around lines 397 - 403, Update captureLogs and its callers to avoid replacing the process-wide slog default; inject the logger or handler into the code under test and capture logs through that explicit dependency. If explicit injection is not feasible, ensure all affected tests execute sequentially and restore logger state safely without introducing order-dependent behavior.
🤖 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/analytics_test.go`:
- Around line 397-403: Update captureLogs and its callers to avoid replacing the
process-wide slog default; inject the logger or handler into the code under test
and capture logs through that explicit dependency. If explicit injection is not
feasible, ensure all affected tests execute sequentially and restore logger
state safely without introducing order-dependent behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5549d101-25a0-443f-99e3-3e24b65a9037
📒 Files selected for processing (2)
gateway/gateway-runtime/policy-engine/internal/analytics/analytics.gogateway/gateway-runtime/policy-engine/internal/analytics/analytics_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/gateway-runtime/policy-engine/internal/analytics/analytics.go
O-sura
approved these changes
Jul 29, 2026
Krishanx92
approved these changes
Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Invoking an LLM proxy produced two analytics events for a single client call, because the proxy forwards to its provider over an internal loopback on the same Envoy listener. This caused:
while the per-API "AI API Details" panel showed the correct count.
the provider (loopback) event carried token/cost/model but showed the user as
anonymous.Fix
Token usage/cost/model is already extracted on the proxy route (the LLM response returns through it) and present in
event.Properties, but the publisher only emitted it forLlmProvider. The guard now includesLlmProxy, so the proxy event is a complete AI record.Before publishing, an
LlmProviderevent whose downstream remote address is loopback(127.0.0.1/::1) is dropped, since it is the internal hop of a proxy call, not a real client request. Directly invoked providers (real client address) are unaffected and still publish.Moesif Dashboard
When I send 3 requests from the proxy and 4 requests from the provider, the analytics dashboard is showing as below.
Resolved: #2441