Skip to content

[APIP] Traffic-logging fixes - #3192

Open
DDH13 wants to merge 3 commits into
wso2:mainfrom
DDH13:main.traffic-log-fix
Open

[APIP] Traffic-logging fixes#3192
DDH13 wants to merge 3 commits into
wso2:mainfrom
DDH13:main.traffic-log-fix

Conversation

@DDH13

@DDH13 DDH13 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

This doesn't address the issue of the gateway-health probe request being sent to datadog

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The translator now enables access logs for stdout or collector ALS independently. It builds file and gRPC sinks separately. gRPC access logs always exclude reserved gateway health paths and can also exclude configured path prefixes.

Changes

Access log sink activation and assembly

Layer / File(s) Summary
Independent sink activation and construction
gateway/gateway-controller/pkg/xds/translator.go, gateway/gateway-controller/pkg/xds/translator_test.go
The listener enables access logs when stdout or collector ALS is configured. File and gRPC sinks are built independently. File errors remain fatal, while gRPC errors are logged and skipped. Tests verify sink combinations and HCM attachment.

gRPC access-log filtering

Layer / File(s) Summary
Reserved health-path and ignore-prefix filtering
gateway/gateway-controller/pkg/xds/translator.go, gateway/gateway-controller/pkg/xds/translator_test.go
The gRPC sink always excludes the reserved gateway health path. Configured ignore prefixes are combined with this filter. Tests verify suppression and fail-open behavior without a path header.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Listener
  participant AccessLogConfig
  participant HCM
  participant CollectorALS
  Listener->>AccessLogConfig: enable stdout or collector ALS
  AccessLogConfig->>HCM: attach configured access-log sinks
  AccessLogConfig->>CollectorALS: apply health-path and ignore-prefix filters
  CollectorALS-->>HCM: emit permitted request logs
Loading

Suggested reviewers: anugayan, arshardh, ashera96

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description states the main fixes and related issues but omits most required template sections, including approach, tests, security checks, and environment. Complete the required template sections and add implementation details, test coverage, security confirmations, documentation impact, related PRs, and test environment.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the traffic-logging fixes described in the pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@DDH13
DDH13 requested a review from Piumal1999 as a code owner August 11, 2026 05:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-controller/pkg/xds/translator.go`:
- Around line 2641-2647: Update the caller around createGRPCAccessLog so its
error is returned when the collector is enabled and stdout logging is disabled,
instead of continuing with an empty accessLogs list; preserve the existing
warning-and-continue behavior when stdout logging remains available. Add a
regression test covering an invalid collector buffer size in collector-only mode
and assert the listener creation propagates the configuration error.
🪄 Autofix

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: 2723fe11-ac18-416b-9b1a-88f7bccd2873

📥 Commits

Reviewing files that changed from the base of the PR and between 8bce858 and 7cc23c5.

📒 Files selected for processing (2)
  • gateway/gateway-controller/pkg/xds/translator.go
  • gateway/gateway-controller/pkg/xds/translator_test.go

Comment thread gateway/gateway-controller/pkg/xds/translator.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant