Skip to content

allow configuration of the metrics server opts #240

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

Merged
jbw976 merged 2 commits into from
Oct 31, 2025
+103 −2
Merged

allow configuration of the metrics server opts #240

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8bbf7a
allow configuration of the metrics server opts
awprice Oct 21, 2025
f339cb7
address coderabbit feedback
awprice Oct 21, 2025
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
23 changes: 17 additions & 6 deletions sdk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ type ServeOptions struct {
MetricsAddress string
MetricsRegistry *prometheus.Registry
UnaryInterceptors []grpc.UnaryServerInterceptor
MetricsServerOpts []grpcprometheus.ServerMetricsOption
}

// A ServeOption configures how a Function is served.
Expand Down Expand Up @@ -173,16 +174,26 @@ func WithMetricsRegistry(registry *prometheus.Registry) ServeOption {
}
}

// WithMetricsServerOpts configures the options for the Metrics Server.
// Note: Metrics collection is enabled only when MetricsAddress is non-empty.
func WithMetricsServerOpts(opts ...grpcprometheus.ServerMetricsOption) ServeOption {
return func(o *ServeOptions) error {
o.MetricsServerOpts = opts
return nil
}
}

// Serve the supplied Function by creating a gRPC server and listening for
// RunFunctionRequests. Blocks until the server returns an error.
func Serve(fn v1.FunctionRunnerServiceServer, o ...ServeOption) error {
//nolint:forcetypeassert // prometheus.DefaultRegisterer is always *prometheus.Registry
so := &ServeOptions{
Network: DefaultNetwork,
Address: DefaultAddress,
MaxRecvMsgSize: DefaultMaxRecvMsgSize,
MetricsAddress: DefaultMetricsAddress,
MetricsRegistry: prometheus.DefaultRegisterer.(*prometheus.Registry), // Use default registry
Network: DefaultNetwork,
Address: DefaultAddress,
MaxRecvMsgSize: DefaultMaxRecvMsgSize,
MetricsAddress: DefaultMetricsAddress,
MetricsRegistry: prometheus.DefaultRegisterer.(*prometheus.Registry), // Use default registry
MetricsServerOpts: make([]grpcprometheus.ServerMetricsOption, 0),
}

for _, fn := range o {
Expand Down Expand Up @@ -214,7 +225,7 @@ func Serve(fn v1.FunctionRunnerServiceServer, o ...ServeOption) error {
// Add metrics interceptor if metrics address is provided
if so.MetricsAddress != "" {
// Use Prometheus metrics
metrics = grpcprometheus.NewServerMetrics()
metrics = grpcprometheus.NewServerMetrics(so.MetricsServerOpts...)

// Apply metrics interceptor and custom interceptors
interceptors = append(interceptors, metrics.UnaryServerInterceptor())
Expand Down
90 changes: 90 additions & 0 deletions sdk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
grpcprometheus "github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus"
"github.com/prometheus/client_golang/prometheus"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
Expand Down Expand Up @@ -303,6 +304,90 @@ func TestMetricsServer_WithDefaultRegistryAndDefaultPort(t *testing.T) {
// Wait for server to start
time.Sleep(3 * time.Second)

t.Run("MetricsServerTest On DefaultPort With DefaultRegistry", func(t *testing.T) {
// Test gRPC connection
conn, err := grpc.NewClient(fmt.Sprintf("localhost:%d", grpcPort),
grpc.WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("Failed to connect: %v", err)
}
defer conn.Close()

client := v1.NewFunctionRunnerServiceClient(conn)

// Make the request
req := &v1.RunFunctionRequest{
Meta: &v1.RequestMeta{Tag: "default-metrics-test"},
}

_, err = client.RunFunction(context.Background(), req)
if err != nil {
t.Errorf("Request failed: %v", err)
}

// Wait for metrics to be collected
time.Sleep(2 * time.Second)

// Verify metrics endpoint is accessible
metricsURL := fmt.Sprintf("http://localhost:%d/metrics", metricsPort)
httpReq, err := http.NewRequestWithContext(context.Background(), http.MethodGet, metricsURL, nil)
if err != nil {
t.Fatalf("Failed to create request: %v", err)
}
resp, err := http.DefaultClient.Do(httpReq)
if err != nil {
t.Fatalf("Failed to get metrics: %v", err)
}
defer resp.Body.Close()

body, err := io.ReadAll(resp.Body)
if err != nil {
t.Fatalf("Failed to read metrics: %v", err)
}

metricsContent := string(body)

// Verify metrics are present
if !strings.Contains(metricsContent, "# HELP") {
t.Error("Expected Prometheus format")
}

// Verify gRPC metrics are present
if !strings.Contains(metricsContent, "grpc_server_started_total") {
t.Error("Expected grpc_server_started_total metric to be present")
}
})
}
Comment on lines 307 to 360
Copy link

@coderabbitai coderabbitai bot Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Default port test is flaky (port 8080 contention) and relies on sleeps.

Two concerns:

  • Port 8080 may be in use on CI/dev machines; the test will intermittently fail.
  • Fixed sleeps slow tests and are brittle vs. startup latency.

Recommend either (a) preflight-check 8080 and skip if busy, plus readiness polling, or (b) use a random free port via WithMetricsServer while keeping a single default-port test elsewhere.

Minimal changes to improve stability:

@@
- // Should use default metrics port 8080
- metricsPort := 8080
+ // Default metrics port is 8080; skip if unavailable to avoid flakiness.
+ metricsPort := 8080
+ if ln, err := net.Listen("tcp", fmt.Sprintf(":%d", metricsPort)); err != nil {
+     t.Skipf("port %d not available, skipping default-port test: %v", metricsPort, err)
+ } else {
+     ln.Close()
+ }
@@
- // Wait for server to start
- time.Sleep(3 * time.Second)
+ // Wait for metrics endpoint to become ready (up to 5s).
+ deadline := time.Now().Add(5 * time.Second)
+ for {
+     if time.Now().After(deadline) {
+         t.Fatalf("metrics endpoint did not become ready at :%d", metricsPort)
+     }
+     req, _ := http.NewRequestWithContext(context.Background(), http.MethodGet,
+         fmt.Sprintf("http://localhost:%d/metrics", metricsPort), nil)
+     if resp, err := http.DefaultClient.Do(req); err == nil {
+         resp.Body.Close()
+         break
+     }
+     time.Sleep(100 * time.Millisecond)
+ }
@@
- resp, err := http.DefaultClient.Do(httpReq)
+ resp, err := http.DefaultClient.Do(httpReq)
  if err != nil {
      t.Fatalf("Failed to get metrics: %v", err)
  }
  defer resp.Body.Close()
+ if resp.StatusCode != http.StatusOK {
+     t.Fatalf("unexpected status code: %d", resp.StatusCode)
+ }

Nit: s/DefaultRegisrty/DefaultRegistry in the subtest name.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In sdk_test.go around lines 307 to 360, the subtest "MetricsServerTest On
DefaultPort With DefaultRegistry" is flaky because it assumes port 8080 and uses
fixed sleeps; change it to avoid port contention and brittle sleeps by (1)
obtaining a free ephemeral port for the metrics server (or preflight-check port
8080 and skip the test if in use) and wire that port into the test via the
server creation helper (e.g., WithMetricsServer(port)), (2) replace time.Sleep
with a readiness polling loop that repeatedly GETs the /metrics endpoint with a
short timeout until it returns 200 or a deadline is reached, and (3) fix the
subtest name typo to "DefaultRegistry". Ensure the test defers closing
connections and uses context timeouts for RPC/HTTP calls.

Copy link
Member

@jbw976 jbw976 Oct 31, 2025

Choose a reason for hiding this comment

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

This pattern exists before this PR, so I wouldn't force us to change it here

Copy link

@coderabbitai coderabbitai bot Oct 31, 2025

Choose a reason for hiding this comment

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

@jbw976, understood! You're right that this is pre-existing behavior, and it makes sense not to expand the scope of this PR to address it.

If you'd like, I can open a follow-up issue to track improving the test stability (handling port contention and replacing fixed sleeps with readiness polling) for future work. Just let me know!

🧠 Learnings used 
Learnt from: CR
Repo: crossplane/function-sdk-go PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-19T23:33:14.658Z
Learning: Applies to **/!(*_test).go : Breaking Changes: Do not remove, rename, or change the signature/behavior of any exported Go functions, types, methods, or fields without adding the 'breaking-change' label; fail the PR otherwise (library repo: all exported APIs are public).


// TestMetricsServer_WithCustomMetricsServerOpts verifies that metrics server uses custom metrics server opts.
func TestMetricsServer_WithCustomMetricsServerOpts(t *testing.T) {
Copy link
Member

@jbw976 jbw976 Oct 31, 2025

Choose a reason for hiding this comment

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

we'll probably want to refactor these tests at some point to reduce the duplication, but it's fine for now 🤓

// Create mock server
mockServer := &MockFunctionServer{
rsp: &v1.RunFunctionResponse{
Meta: &v1.ResponseMeta{Tag: "default-metrics-test"},
},
}

// Get ports
grpcPort := getAvailablePort(t)
// Should use default metrics port 8080
metricsPort := 8080

serverDone := make(chan error, 1)
go func() {
err := Serve(mockServer,
Listen("tcp", fmt.Sprintf(":%d", grpcPort)),
Insecure(true),
WithMetricsServerOpts(
grpcprometheus.WithServerHandlingTimeHistogram(),
),
)
serverDone <- err
}()

// Wait for server to start
time.Sleep(3 * time.Second)

t.Run("MetricsServerTest On DefaultPort With DefaultRegisrty", func(t *testing.T) {
// Test gRPC connection
conn, err := grpc.NewClient(fmt.Sprintf("localhost:%d", grpcPort),
Expand Down Expand Up @@ -355,6 +440,11 @@ func TestMetricsServer_WithDefaultRegistryAndDefaultPort(t *testing.T) {
if !strings.Contains(metricsContent, "grpc_server_started_total") {
t.Error("Expected grpc_server_started_total metric to be present")
}

// Verify gRPC Histogram metrics are present
if !strings.Contains(metricsContent, "grpc_server_handling_seconds_bucket") {
t.Error("Expected grpc_server_handling_seconds_bucket metric to be present")
}
})
}

Expand Down
Loading