perf: configurable HTTP compression, NR auto-disable, response time log config#58
perf: configurable HTTP compression, NR auto-disable, response time log config#58
Conversation
…og config - Add DISABLE_HTTP_COMPRESSION and HTTP_COMPRESSION_MIN_SIZE config (default: 256 bytes). Applies to both gzip and zstd. - Auto-disable NewRelic when license key is empty to avoid interceptor overhead for services that don't use NR. - Add RESPONSE_TIME_LOG_LEVEL config (default: "info"). Invalid values fall back to info. Wired via ConfigureInterceptors. - Add RESPONSE_TIME_LOG_ERROR_ONLY config (default: false). When true, only logs response time for requests that return errors.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request adds four new configuration fields to the Config struct for HTTP compression control ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds new runtime configuration knobs for HTTP response compression and response-time logging, and reduces New Relic overhead when it’s effectively unconfigured.
Changes:
- Add HTTP compression toggles and a minimum-size threshold for applying compression.
- Auto-disable New Relic when
DISABLE_NEW_RELIC=falsebut no license key is set. - Add interceptor configuration for response-time log level and “error-only” response-time logging.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
core.go |
Adds NR auto-disable logic and makes HTTP compression conditional with a configurable min-size. |
initializers.go |
Extends ConfigureInterceptors to configure response-time logging settings. |
config/config.go |
Introduces new env-configured fields for compression and response-time logging. |
core_coverage_test.go |
Updates the interceptor configuration call site for the new signature. |
go.sum |
Removes go-coldbrew/interceptors sums (currently inconsistent with go.mod). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
config/config.go (1)
138-151: New configuration fields are well-structured.The new throughput tuning fields are properly documented with sensible defaults. A few observations:
The comment on line 141 mentions "gzip/zstd compression" but the implementation in
core.goonly usesgzhttp(gzip). Consider updating the comment to reflect the actual implementation or add zstd support if intended.Consider adding validation for
HTTPCompressionMinSizein theValidate()method to warn if negative values are configured.Optional: Add validation for HTTPCompressionMinSize
func (c Config) Validate() []string { var warnings []string + if c.HTTPCompressionMinSize < 0 { + warnings = append(warnings, "HTTPCompressionMinSize should be non-negative") + } if c.GRPCPort < 0 || c.GRPCPort > 65535 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 138 - 151, Update the config comments and validation: change the comment on DisableHTTPCompression/HTTPCompressionMinSize to reflect that only gzip is currently used (gzhttp in core.go) or note "gzip (zstd not implemented)" so it matches the implementation, and add a validation check inside the existing Validate() method to ensure HTTPCompressionMinSize is non-negative (return or log a validation error if HTTPCompressionMinSize < 0); reference the config struct fields DisableHTTPCompression, HTTPCompressionMinSize, ResponseTimeLogLevel and the Validate() function and mention gzhttp/core.go as the place tied to gzip behavior.core_coverage_test.go (1)
1167-1168: Test updated to match new signature.The test correctly passes the new
responseTimeLogLevelandresponseTimeLogErrorOnlyparameters.Consider adding test cases for edge conditions:
- Invalid log level string (to verify fallback behavior)
responseTimeLogErrorOnlyset totrue- Empty
responseTimeLogLevelstringOptional: Additional test cases
func TestConfigureInterceptors_BothBranches(t *testing.T) { ConfigureInterceptors(true, "X-My-Trace", "info", false) } + +func TestConfigureInterceptors_InvalidLogLevel(t *testing.T) { + // Should not panic; falls back to info level + ConfigureInterceptors(false, "", "invalid-level", false) +} + +func TestConfigureInterceptors_ErrorOnly(t *testing.T) { + ConfigureInterceptors(false, "", "debug", true) +} + +func TestConfigureInterceptors_EmptyLogLevel(t *testing.T) { + // Empty log level should skip SetResponseTimeLogLevel call + ConfigureInterceptors(false, "", "", true) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core_coverage_test.go` around lines 1167 - 1168, Add unit tests for ConfigureInterceptors to cover edge conditions: create separate Test cases that call ConfigureInterceptors using an invalid log level string (e.g., "invalid") to assert it falls back to the default level, a case with responseTimeLogErrorOnly set to true to verify only error responses are logged, and a case with an empty responseTimeLogLevel string to confirm default behavior; reference the ConfigureInterceptors function and assert expected logging behavior or no panics for each scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/config.go`:
- Around line 138-151: Update the config comments and validation: change the
comment on DisableHTTPCompression/HTTPCompressionMinSize to reflect that only
gzip is currently used (gzhttp in core.go) or note "gzip (zstd not implemented)"
so it matches the implementation, and add a validation check inside the existing
Validate() method to ensure HTTPCompressionMinSize is non-negative (return or
log a validation error if HTTPCompressionMinSize < 0); reference the config
struct fields DisableHTTPCompression, HTTPCompressionMinSize,
ResponseTimeLogLevel and the Validate() function and mention gzhttp/core.go as
the place tied to gzip behavior.
In `@core_coverage_test.go`:
- Around line 1167-1168: Add unit tests for ConfigureInterceptors to cover edge
conditions: create separate Test cases that call ConfigureInterceptors using an
invalid log level string (e.g., "invalid") to assert it falls back to the
default level, a case with responseTimeLogErrorOnly set to true to verify only
error responses are logged, and a case with an empty responseTimeLogLevel string
to confirm default behavior; reference the ConfigureInterceptors function and
assert expected logging behavior or no panics for each scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfa55c2b-c9f9-4c2e-9ae4-82b6cb38139f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
config/config.gocore.gocore_coverage_test.goinitializers.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- options v0.2.6 -> v0.2.7 (RWMutex+map) - log v0.2.8 -> v0.2.9 (RWMutex+map, nil ctx guard) - errors v0.2.6 -> v0.2.9 (OTEL trace ID linkage) - interceptors v0.1.13 -> v0.1.15 (NR short-circuit, error-only logging) - Remove replace directive
Summary
DISABLE_HTTP_COMPRESSIONandHTTP_COMPRESSION_MIN_SIZE(default: 256 bytes). Applies to both gzip and zstd. Responses smaller than min-size are sent uncompressed.DISABLE_NEW_RELICis false butNEW_RELIC_LICENSE_KEYis empty, NR is auto-disabled to avoid interceptor overhead.RESPONSE_TIME_LOG_LEVELconfig (default: "info"). Invalid values fall back to info.RESPONSE_TIME_LOG_ERROR_ONLYconfig (default: false). When true, only logs response time for errored requests.Note: Requires go-coldbrew/interceptors with
SetResponseTimeLogErrorOnly(PR go-coldbrew/interceptors#28). Usesreplacedirective for local testing; will bump interceptors version after merge.Test plan
go test -race ./...passes (with local replace)Summary by CodeRabbit