docs: clarify send/recv msg size comments by ankurs · Pull Request #75 · go-coldbrew/core

ankurs · 2026-04-09T08:15:40Z

Summary

Clarify that GRPCMaxSendMsgSize limits response size from the service and GRPCMaxRecvMsgSize limits request size to the service
Separate per-field comments (previously a shared comment block)
Regenerate READMEs via make doc

Test plan

make test passes
make lint clean
make doc run, READMEs updated

Summary by CodeRabbit

New Features
- Added configuration options to control debug logging: disable the default interceptor and customize the header name triggering per-request debug logging.
- Enhanced validation with warnings for gRPC TLS files, OTLP endpoint format, log level settings, and timeout configurations.
Chores
- Updated internal module dependencies and improved test infrastructure with goroutine-leak detection.

Config validation (4.1): - TLS file existence check via os.Stat - OTLP endpoint host:port format validation - Log level validation against known levels - Timeout vs shutdown duration sanity check DebugLogInterceptor wiring: - Add DisableDebugLogInterceptor and DebugLogHeaderName config fields - Wire to interceptors in processConfig - Update getCustomHeaderMatcher to variadic headers for HTTP support Also adds goleak.VerifyTestMain for goroutine leak detection.

Address review: use os.IsNotExist for "not found" vs general "could not be accessed" with error details. Add TestGetCustomHeaderMatcher_MultipleHeaders for variadic header matching.

… v0.2.2

Clarify that GRPCMaxSendMsgSize limits response size FROM the service and GRPCMaxRecvMsgSize limits request size TO the service. Separate per-field comments. Regenerate READMEs via make doc.

coderabbitai · 2026-04-09T08:15:55Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3c384576-250d-44b1-9ef7-3c6c1564894d

📥 Commits

Reviewing files that changed from the base of the PR and between de96de5 and 93e229d.

⛔ Files ignored due to path filters (1)

go.sum is excluded by !**/*.sum

📒 Files selected for processing (8)

README.md
config/README.md
config/config.go
config/config_test.go
core.go
core_test.go
go.mod
goleak_test.go

📝 Walkthrough

Walkthrough

This PR adds configuration-driven control for debug logging interceptors via two new config fields (DisableDebugLogInterceptor, DebugLogHeaderName), extends Config.Validate() with additional warning checks for TLS files, OTLP endpoints, log levels, and timeout/shutdown duration mismatches, refactors header matcher to support multiple exact-match headers, and introduces goroutine leak detection in tests.

Changes

Cohort / File(s)	Summary
Documentation Updates `README.md`, `config/README.md`	Updated GitHub source URL line-number anchors for API reference links to `SetOTELGRPCClientOptions`, `SetOTELGRPCServerOptions`, `SetOTELOptions`, `New`, and `Config.Validate()` to reflect current source positions; no functional changes.
Configuration Fields & Validation `config/config.go`	Added new exported fields `DisableDebugLogInterceptor` and `DebugLogHeaderName` to control debug logging behavior. Extended `Config.Validate()` to warn on missing TLS certificate/key files, malformed OTLP endpoint strings, unrecognized log levels, and timeout values exceeding shutdown duration. Added imports `net`, `os`, `strings`.
Configuration Tests `config/config_test.go`	Added four new test functions to validate `Config.Validate()` behavior for missing TLS files, invalid OTLP endpoint formats, unrecognized log levels, and timeout/shutdown duration mismatches; asserts specific warning message content.
Core Functionality `core.go`	Integrated debug logging interceptor setup in `processConfig()` based on `DisableDebugLogInterceptor` and `DebugLogHeaderName` config fields. Refactored `getCustomHeaderMatcher()` to accept variadic headers instead of single header, enabling exact-match forwarding of multiple headers in HTTP→gRPC gateway.
Core Tests `core_test.go`	Added `TestGetCustomHeaderMatcher_MultipleHeaders` to verify the refactored header matcher correctly handles multiple explicit header names.
Dependency Management & Goroutine Leak Detection `go.mod`, `goleak_test.go`	Updated module versions for `github.com/go-coldbrew/{errors,interceptors,log,tracing}` and added `go.uber.org/goleak v1.3.0`. Added `goleak_test.go` with `TestMain` to detect and whitelist expected goroutine leaks from external components (gRPC, OpenTelemetry, Sentry, rollbar-go).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

feat: config validation improvements, wire DebugLogInterceptor #74: Applies identical config and interceptor changes (DisableDebugLogInterceptor, DebugLogHeaderName wiring, header matching refactor).
chore: remove deprecated Jaeger client, keep OTEL as sole tracing backend #50: Modifies processConfig() for gRPC setup and configuration-driven initialization.
Increase test coverage to 83.8% and fix SetupHystrixPrometheus panic #43: Introduces and extends custom header matching and HTTP→gRPC gateway forwarding logic.

Suggested reviewers

vestor
fajran
kevinjom
svetha-cvl

Poem

🐰 Config fields dance in the debug log light,
Headers match true, multiple and bright!
Validation guards against the broken and wrong,
Goroutines rest where they belong—
All safely leashed, this fix sings strong! 🎉

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title accurately reflects a key documentation change (clarifying send/recv msg size comments), but the PR contains substantial functional changes beyond this narrow scope—including new config fields, validation logic, interceptor wiring, and dependency upgrades.	Revise the title to reflect the primary change, such as 'feat: add debug log interceptor config and validation' or use a broader title like 'refactor: enhance config and validation with debug logging support' that captures the main functional work.
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/config-msg-size-comments

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 and usage tips.}

Copilot

Pull request overview

This PR updates the generated documentation around gRPC max send/recv message size semantics, and also introduces additional runtime/config/test changes in the core package (config validation enhancements, HTTP header forwarding updates, and goroutine leak checking).

Changes:

Clarify GRPCMaxSendMsgSize (response) vs GRPCMaxRecvMsgSize (request) field documentation and regenerate READMEs.
Extend HTTP→gRPC metadata header forwarding to support multiple exact-match headers (trace + debug-log header) and add related config plumbing.
Add goleak-based TestMain and expand Config.Validate() checks with accompanying tests; bump related dependencies.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
README.md	Regenerated gomarkdoc output (updated symbol links/line refs).
config/README.md	Regenerated config docs; clarifies gRPC send/recv size comments and documents new debug-log config fields.
config/config.go	Adds debug-log config fields; clarifies gRPC send/recv size semantics; expands `Validate()` with TLS file, OTLP endpoint, log level, and timing checks.
config/config_test.go	Adds tests covering the new `Validate()` warnings.
core.go	Wires new debug-log config into interceptors; updates header matcher to accept multiple exact headers; forwards debug-log header through grpc-gateway.
core_test.go	Adds test for multi-header matching behavior.
goleak_test.go	Adds package-level `TestMain` to detect goroutine leaks with a set of ignore rules.
go.mod	Bumps internal dependencies and adds `go.uber.org/goleak`.
go.sum	Updates checksums for bumped/added dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-09T08:19:41Z

+	if c.config.DisableDebugLogInterceptor {
+		interceptors.SetDisableDebugLogInterceptor(true)
+	}
+	if c.config.DebugLogHeaderName != "" {
+		interceptors.SetDebugLogHeaderName(c.config.DebugLogHeaderName)
+	}


The PR title/description says this is a docs-only clarification, but this hunk introduces new runtime behavior (new config flags affecting interceptor behavior) and is coupled with other non-doc changes in the PR (goleak TestMain, config validation additions, dependency bumps). Please update the PR title/description to reflect the functional changes, or split the functional changes into a separate PR to keep review scope clear.

Recreated PR as #76 with clean branch from main — only the doc/comment changes. The previous branch had stale commits from the prior PR.

Copilot · 2026-04-09T08:19:41Z

+		for _, prefix := range prefixes {
+			if len(prefix) > 0 && strings.HasPrefix(key, strings.ToLower(prefix)) {
+				return key, true


getCustomHeaderMatcher lowercases each configured prefix on every header match attempt (strings.ToLower(prefix) inside the per-request matcher). Since this function runs for every inbound HTTP header, consider normalizing prefixes once (similar to lowerHeaders) to avoid repeated work on hot paths.

Good catch — fixed in #76. Prefixes are now normalized once at init alongside headers.

ankurs added 4 commits April 9, 2026 13:22

fix: differentiate TLS not-found vs access errors, add multi-header test

7ce60c4

Address review: use os.IsNotExist for "not found" vs general "could not be accessed" with error details. Add TestGetCustomHeaderMatcher_MultipleHeaders for variadic header matching.

chore: bump interceptors v0.1.23, errors v0.2.14, log v0.3.2, tracing…

01f2c6a

… v0.2.2

docs: clarify send/recv msg size comments, regenerate READMEs

93e229d

Clarify that GRPCMaxSendMsgSize limits response size FROM the service and GRPCMaxRecvMsgSize limits request size TO the service. Separate per-field comments. Regenerate READMEs via make doc.

Copilot AI review requested due to automatic review settings April 9, 2026 08:15

Copilot started reviewing on behalf of ankurs April 9, 2026 08:16 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

ankurs closed this Apr 9, 2026

ankurs deleted the feat/config-msg-size-comments branch April 9, 2026 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: clarify send/recv msg size comments#75

docs: clarify send/recv msg size comments#75
ankurs wants to merge 4 commits intomainfrom
feat/config-msg-size-comments

ankurs commented Apr 9, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 9, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (2 warnings)

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

ankurs Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

ankurs Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ankurs commented Apr 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (2 warnings)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

ankurs Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

ankurs Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ankurs commented Apr 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 9, 2026 •

edited

Loading