Skip to content

feat: add sane defaults for gRPC connection keepalive #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ankurs wants to merge 4 commits into from
Closed

feat: add sane defaults for gRPC connection keepalive #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fdf7489
feat: add sane defaults for gRPC connection keepalive
ankurs Apr 8, 2026
34260b2
fix: use baseline comparison in keepalive tests
ankurs Apr 8, 2026
b42b9cd
chore: bump Go to 1.25.9 to fix stdlib CVEs
ankurs Apr 8, 2026
5b61b39
fix: rewrite KeepaliveAllZero test to compare against positive config
ankurs Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ const SupportPackageIsVersion1 = true
```

<a name="ConfigureInterceptors"></a>
## func [ConfigureInterceptors](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L388>)
## func [ConfigureInterceptors](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L389>)

```go
func ConfigureInterceptors(DoNotLogGRPCReflection bool, traceHeaderName string, responseTimeLogLevel string, responseTimeLogErrorOnly bool)
Expand All @@ -127,7 +127,7 @@ func ConfigureInterceptors(DoNotLogGRPCReflection bool, traceHeaderName string,
ConfigureInterceptors configures the interceptors package with the provided settings.

<a name="InitializeVTProto"></a>
## func [InitializeVTProto](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L443>)
## func [InitializeVTProto](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L444>)

```go
func InitializeVTProto()
Expand All @@ -138,7 +138,7 @@ InitializeVTProto initializes the vtproto package for use with the service
https://github.com/planetscale/vtprotobuf?tab=readme-ov-file#mixing-protobuf-implementations-with-grpc

<a name="OTELMeterProvider"></a>
## func [OTELMeterProvider](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L343>)
## func [OTELMeterProvider](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L344>)

```go
func OTELMeterProvider() otelmetric.MeterProvider
Expand All @@ -147,7 +147,7 @@ func OTELMeterProvider() otelmetric.MeterProvider
OTELMeterProvider returns the global OTel MeterProvider. This is a convenience accessor for code that needs the interface type.

<a name="SetOTELGRPCClientOptions"></a>
## func [SetOTELGRPCClientOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L558>)
## func [SetOTELGRPCClientOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L575>)

```go
func SetOTELGRPCClientOptions(opts ...otelgrpc.Option)
Expand All @@ -156,7 +156,7 @@ func SetOTELGRPCClientOptions(opts ...otelgrpc.Option)
Deprecated: Use SetOTELOptions instead. Only applies when OTEL\_USE\_LEGACY\_INSTRUMENTATION=true.

<a name="SetOTELGRPCServerOptions"></a>
## func [SetOTELGRPCServerOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L552>)
## func [SetOTELGRPCServerOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L569>)

```go
func SetOTELGRPCServerOptions(opts ...otelgrpc.Option)
Expand All @@ -165,7 +165,7 @@ func SetOTELGRPCServerOptions(opts ...otelgrpc.Option)
Deprecated: Use SetOTELOptions instead. Only applies when OTEL\_USE\_LEGACY\_INSTRUMENTATION=true.

<a name="SetOTELOptions"></a>
## func [SetOTELOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L565>)
## func [SetOTELOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L582>)

```go
func SetOTELOptions(opts grpcotel.Options)
Expand All @@ -174,7 +174,7 @@ func SetOTELOptions(opts grpcotel.Options)
SetOTELOptions configures the native gRPC stats/opentelemetry integration. Must be called during init, before the gRPC server starts. When set, processConfig\(\) will NOT overwrite these with auto\-built options.

<a name="SetupAutoMaxProcs"></a>
## func [SetupAutoMaxProcs](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L409>)
## func [SetupAutoMaxProcs](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L410>)

```go
func SetupAutoMaxProcs()
Expand All @@ -192,7 +192,7 @@ func SetupEnvironment(env string)
SetupEnvironment sets the environment This is used to identify the environment in Sentry and New Relic env is the environment to set for the service \(e.g. prod, staging, dev\)

<a name="SetupHystrixPrometheus"></a>
## func [SetupHystrixPrometheus](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L380>)
## func [SetupHystrixPrometheus](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L381>)

```go
func SetupHystrixPrometheus()
Expand All @@ -210,7 +210,7 @@ func SetupLogger(logLevel string, jsonlogs bool) error
SetupLogger sets up the logger It uses the coldbrew logger to log messages to stdout logLevel is the log level to set for the logger jsonlogs is a boolean to enable or disable json logs

<a name="SetupNROpenTelemetry"></a>
## func [SetupNROpenTelemetry](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L357>)
## func [SetupNROpenTelemetry](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L358>)

```go
func SetupNROpenTelemetry(serviceName, license, version string, ratio float64) error
Expand All @@ -237,7 +237,7 @@ func SetupNewRelic(serviceName, apiKey string, tracing bool) error
SetupNewRelic sets up the New Relic tracing and monitoring agent for the service It uses the New Relic Go Agent to send traces to New Relic One APM and Insights serviceName is the name of the service apiKey is the New Relic license key tracing is a boolean to enable or disable tracing

<a name="SetupOTELMetrics"></a>
## func [SetupOTELMetrics](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L294>)
## func [SetupOTELMetrics](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L293>)

```go
func SetupOTELMetrics(config OTLPConfig, interval time.Duration) (*sdkmetric.MeterProvider, error)
Expand All @@ -248,7 +248,7 @@ SetupOTELMetrics creates a MeterProvider with an OTLP gRPC exporter that reuses
Call this after SetupOpenTelemetry so the shared resource is available.

<a name="SetupOpenTelemetry"></a>
## func [SetupOpenTelemetry](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L234>)
## func [SetupOpenTelemetry](<https://github.com/go-coldbrew/core/blob/main/initializers.go#L224>)

```go
func SetupOpenTelemetry(config OTLPConfig) error
Expand Down Expand Up @@ -324,7 +324,7 @@ type CB interface {
```

<a name="New"></a>
### func [New](<https://github.com/go-coldbrew/core/blob/main/core.go#L878>)
### func [New](<https://github.com/go-coldbrew/core/blob/main/core.go#L895>)

```go
func New(c config.Config) CB
Expand Down
12 changes: 6 additions & 6 deletions config/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,19 +144,19 @@ type Config struct {
// MaxConnectionIdle is a duration for the amount of time after which an
// idle connection would be closed by sending a GoAway. Idleness duration is
// defined since the most recent time the number of outstanding RPCs became
// zero or the connection establishment.
// zero or the connection establishment. Set to -1 to disable (infinite).
// https://github.com/grpc/grpc-go/blob/v1.48.0/keepalive/keepalive.go#L50
GRPCServerMaxConnectionIdleInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_IDLE_IN_SECONDS"`
GRPCServerMaxConnectionIdleInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_IDLE_IN_SECONDS" default:"300"`
// MaxConnectionAge is a duration for the maximum amount of time a
// connection may exist before it will be closed by sending a GoAway. A
// random jitter of +/-10% will be added to MaxConnectionAge to spread out
// connection storms.
// connection storms. Set to -1 to disable (infinite).
// https://github.com/grpc/grpc-go/blob/v1.48.0/keepalive/keepalive.go#L50
GRPCServerMaxConnectionAgeInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_AGE_IN_SECONDS"`
GRPCServerMaxConnectionAgeInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_AGE_IN_SECONDS" default:"1800"`
// MaxConnectionAgeGrace is an additive period after MaxConnectionAge after
// which the connection will be forcibly closed.
// which the connection will be forcibly closed. Set to -1 to disable (infinite).
// https://github.com/grpc/grpc-go/blob/v1.48.0/keepalive/keepalive.go#L50
GRPCServerMaxConnectionAgeGraceInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_AGE_GRACE_IN_SECONDS"`
GRPCServerMaxConnectionAgeGraceInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_AGE_GRACE_IN_SECONDS" default:"30"`

// DisableAutoMaxProcs disables the automatic setting of GOMAXPROCS
// This is useful when running in a container where the container runtime sets GOMAXPROCS for you already
Expand Down
12 changes: 6 additions & 6 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,19 +76,19 @@ type Config struct {
// MaxConnectionIdle is a duration for the amount of time after which an
// idle connection would be closed by sending a GoAway. Idleness duration is
// defined since the most recent time the number of outstanding RPCs became
// zero or the connection establishment.
// zero or the connection establishment. Set to -1 to disable (infinite).
// https://github.com/grpc/grpc-go/blob/v1.48.0/keepalive/keepalive.go#L50
GRPCServerMaxConnectionIdleInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_IDLE_IN_SECONDS"`
GRPCServerMaxConnectionIdleInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_IDLE_IN_SECONDS" default:"300"`
// MaxConnectionAge is a duration for the maximum amount of time a
// connection may exist before it will be closed by sending a GoAway. A
// random jitter of +/-10% will be added to MaxConnectionAge to spread out
// connection storms.
// connection storms. Set to -1 to disable (infinite).
// https://github.com/grpc/grpc-go/blob/v1.48.0/keepalive/keepalive.go#L50
GRPCServerMaxConnectionAgeInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_AGE_IN_SECONDS"`
GRPCServerMaxConnectionAgeInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_AGE_IN_SECONDS" default:"1800"`
// MaxConnectionAgeGrace is an additive period after MaxConnectionAge after
// which the connection will be forcibly closed.
// which the connection will be forcibly closed. Set to -1 to disable (infinite).
// https://github.com/grpc/grpc-go/blob/v1.48.0/keepalive/keepalive.go#L50
GRPCServerMaxConnectionAgeGraceInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_AGE_GRACE_IN_SECONDS"`
GRPCServerMaxConnectionAgeGraceInSeconds int `envconfig:"GRPC_SERVER_MAX_CONNECTION_AGE_GRACE_IN_SECONDS" default:"30"`

// DisableAutoMaxProcs disables the automatic setting of GOMAXPROCS
// This is useful when running in a container where the container runtime sets GOMAXPROCS for you already
Expand Down
6 changes: 3 additions & 3 deletions core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,9 +628,9 @@ func (c *cb) getGRPCServerOptions() []grpc.ServerOption {
so = append(so, grpc.MaxSendMsgSize(c.config.GRPCMaxSendMsgSize))
}

if c.config.GRPCServerMaxConnectionAgeGraceInSeconds > 0 ||
c.config.GRPCServerMaxConnectionAgeInSeconds > 0 ||
c.config.GRPCServerMaxConnectionIdleInSeconds > 0 {
if c.config.GRPCServerMaxConnectionAgeGraceInSeconds != 0 ||
c.config.GRPCServerMaxConnectionAgeInSeconds != 0 ||
c.config.GRPCServerMaxConnectionIdleInSeconds != 0 {
option := keepalive.ServerParameters{}
if c.config.GRPCServerMaxConnectionIdleInSeconds > 0 {
option.MaxConnectionIdle = time.Duration(
Expand Down
76 changes: 71 additions & 5 deletions core_coverage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,12 +449,78 @@

// --- Group 2: gRPC Server Options ---

func TestGetGRPCServerOptions_Default(t *testing.T) {
func testKeepaliveBaseline() int {
base := &cb{config: config.Config{}}
return len(base.getGRPCServerOptions())
}

func TestGetGRPCServerOptions_WithEnvconfigDefaults(t *testing.T) {
// removed t.Parallel() — core tests mutate package-level globals
c := &cb{config: config.Config{}}
// Validates behavior when config has the envconfig default values (300, 1800, 30).
// Keepalive option should be appended beyond the baseline.
baseline := testKeepaliveBaseline()
c := &cb{config: config.Config{
GRPCServerMaxConnectionIdleInSeconds: 300,
GRPCServerMaxConnectionAgeInSeconds: 1800,
GRPCServerMaxConnectionAgeGraceInSeconds: 30,
}}
opts := c.getGRPCServerOptions()
if len(opts) < 2 {
t.Fatalf("expected at least 2 server options, got %d", len(opts))
if len(opts) <= baseline {
t.Fatalf("expected keepalive option beyond baseline %d, got %d", baseline, len(opts))
}
Comment thread
ankurs marked this conversation as resolved.
}

func TestGetGRPCServerOptions_KeepaliveDisabledWithNegativeOne(t *testing.T) {
// removed t.Parallel() — core tests mutate package-level globals
// Setting all values to -1 should still append keepalive params (outer != 0 check),
// but individual parameters are not set on ServerParameters (inner > 0 check),
// so gRPC uses infinity for each.
baseline := testKeepaliveBaseline()
c := &cb{config: config.Config{
GRPCServerMaxConnectionIdleInSeconds: -1,
GRPCServerMaxConnectionAgeInSeconds: -1,
GRPCServerMaxConnectionAgeGraceInSeconds: -1,
}}
opts := c.getGRPCServerOptions()
if len(opts) <= baseline {
t.Fatalf("expected keepalive option beyond baseline %d, got %d", baseline, len(opts))
}
Comment thread
ankurs marked this conversation as resolved.
}

func TestGetGRPCServerOptions_KeepaliveMixed(t *testing.T) {
// removed t.Parallel() — core tests mutate package-level globals
// Mix of -1 (disabled) and positive (enabled) values should still append
// keepalive params relative to the all-zero baseline.
baseline := testKeepaliveBaseline()
c := &cb{config: config.Config{
GRPCServerMaxConnectionIdleInSeconds: -1,
GRPCServerMaxConnectionAgeInSeconds: 1800,
GRPCServerMaxConnectionAgeGraceInSeconds: 30,
}}
opts := c.getGRPCServerOptions()
if len(opts) <= baseline {
t.Fatalf("expected keepalive option beyond baseline %d, got %d", baseline, len(opts))
}
}

func TestGetGRPCServerOptions_KeepaliveAllZero(t *testing.T) {
// removed t.Parallel() — core tests mutate package-level globals
// All zeros should NOT add keepalive params (outer != 0 check is false).
// Compare against a positive-value config to prove zero omits keepalive.
zeroConfig := &cb{config: config.Config{
GRPCServerMaxConnectionIdleInSeconds: 0,
GRPCServerMaxConnectionAgeInSeconds: 0,
GRPCServerMaxConnectionAgeGraceInSeconds: 0,
}}
positiveConfig := &cb{config: config.Config{
GRPCServerMaxConnectionIdleInSeconds: 300,
GRPCServerMaxConnectionAgeInSeconds: 1800,
GRPCServerMaxConnectionAgeGraceInSeconds: 30,
}}
zeroOpts := zeroConfig.getGRPCServerOptions()
positiveOpts := positiveConfig.getGRPCServerOptions()
if len(zeroOpts) >= len(positiveOpts) {
t.Fatalf("expected fewer options with all-zero keepalive than positive, got %d vs %d", len(zeroOpts), len(positiveOpts))
}
Comment thread
ankurs marked this conversation as resolved.
}

Expand Down Expand Up @@ -1303,7 +1369,7 @@
}

func TestConfigureInterceptors_BothBranches(t *testing.T) {
ConfigureInterceptors(true, "X-My-Trace", "info", false)
ConfigureInterceptors(true, "X-My-Trace", "info", false, 60)

Check failure on line 1372 in core_coverage_test.go

View workflow job for this annotation

GitHub Actions / lint 

too many arguments in call to ConfigureInterceptors

Check failure on line 1372 in core_coverage_test.go

View workflow job for this annotation

GitHub Actions / test 

too many arguments in call to ConfigureInterceptors
}
Comment on lines 1371 to 1373
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix the ConfigureInterceptors call signature.

Line 1372 passes five arguments, but ConfigureInterceptors in initializers.go currently accepts four, so this test does not compile. If the extra timeout is intentional, the matching signature change needs to land in the same PR; otherwise, drop the trailing 60.

Suggested fix 
 func TestConfigureInterceptors_BothBranches(t *testing.T) {
-	ConfigureInterceptors(true, "X-My-Trace", "info", false, 60)
+	ConfigureInterceptors(true, "X-My-Trace", "info", false)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func TestConfigureInterceptors_BothBranches(t *testing.T) {
ConfigureInterceptors(true, "X-My-Trace", "info", false)
ConfigureInterceptors(true, "X-My-Trace", "info", false, 60)
}
func TestConfigureInterceptors_BothBranches(t *testing.T) {
ConfigureInterceptors(true, "X-My-Trace", "info", false)
}
🧰 Tools
🪛 GitHub Actions: Go

[error] 1372-1372: golangci-lint build/test failure: too many arguments in call to ConfigureInterceptors in core_coverage_test.go:1372:59.

🪛 GitHub Check: lint

[failure] 1372-1372:
too many arguments in call to ConfigureInterceptors

🪛 GitHub Check: test

[failure] 1372-1372:
too many arguments in call to ConfigureInterceptors

🪛 golangci-lint (2.11.4)

[error] 1372-1372: too many arguments in call to ConfigureInterceptors

(typecheck)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@core_coverage_test.go` around lines 1371 - 1373, The test calls
ConfigureInterceptors with five arguments but the current ConfigureInterceptors
function (named ConfigureInterceptors) only accepts four; either remove the
trailing 60 from the TestConfigureInterceptors_BothBranches call or update
ConfigureInterceptors's signature to accept a timeout parameter and thread that
new parameter through its implementation (e.g., add a timeout int/uint param to
ConfigureInterceptors and update any call sites and internal uses). Locate the
ConfigureInterceptors function and either align the test to its existing
four-arg signature or add the fifth timeout argument to the function and its
callers.


func TestConfig_Validate_HTTPCompressionMinSize(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/go-coldbrew/core

go 1.25.8
go 1.25.9
Comment thread
ankurs marked this conversation as resolved.

require (
github.com/afex/hystrix-go v0.0.0-20180502004556-fa1af6a1f4f5
Expand Down
Loading