From ce631e8b785bf95eb461d0aa1421db06363d8ae8 Mon Sep 17 00:00:00 2001 From: Wes Tarle Date: Sun, 29 Mar 2026 15:12:57 -0400 Subject: [PATCH 1/4] feat: add TransportTelemetryData extraction to otelAttributeTransport --- auth/grpctransport/grpctransport.go | 51 ++++- auth/grpctransport/grpctransport_otel_test.go | 83 ++++++-- auth/httptransport/httptransport_otel_test.go | 194 ++++++++++++++++++ auth/httptransport/transport.go | 69 +++++-- 4 files changed, 364 insertions(+), 33 deletions(-) diff --git a/auth/grpctransport/grpctransport.go b/auth/grpctransport/grpctransport.go index 290a3d7dfcf6..94bf5bf5c988 100644 --- a/auth/grpctransport/grpctransport.go +++ b/auth/grpctransport/grpctransport.go @@ -22,9 +22,11 @@ import ( "errors" "fmt" "log/slog" + "net" "net/http" "os" "strconv" + "strings" "cloud.google.com/go/auth" "cloud.google.com/go/auth/credentials" @@ -460,6 +462,9 @@ func addOpenTelemetryStatsHandler(dialOpts []grpc.DialOption, opts *Options) []g if opts.DisableTelemetry { return dialOpts } + if gax.IsFeatureEnabled("METRICS") { + dialOpts = append(dialOpts, grpc.WithChainUnaryInterceptor(openTelemetryUnaryClientInterceptor())) + } if !gax.IsFeatureEnabled("TRACING") && !gax.IsFeatureEnabled("LOGGING") { return append(dialOpts, grpc.WithStatsHandler(otelgrpc.NewClientHandler())) } @@ -475,8 +480,9 @@ func addOpenTelemetryStatsHandler(dialOpts []grpc.DialOption, opts *Options) []g scopedLogger = opts.Logger.With(staticLogAttrs...) } } - otelOpts := []otelgrpc.Option{ - otelgrpc.WithSpanAttributes(staticAttrs...), + var otelOpts []otelgrpc.Option + if gax.IsFeatureEnabled("TRACING") { + otelOpts = append(otelOpts, otelgrpc.WithSpanAttributes(staticAttrs...)) } return append(dialOpts, grpc.WithStatsHandler(&otelHandler{ Handler: otelgrpc.NewClientHandler(otelOpts...), @@ -484,6 +490,47 @@ func addOpenTelemetryStatsHandler(dialOpts []grpc.DialOption, opts *Options) []g })) } +// Extract the host and port from a target address +func extractHostPort(target string) (string, int) { + if idx := strings.Index(target, "://"); idx != -1 { + target = target[idx+3:] + // Ensure any trailing slashes from the scheme suffix are stripped + for strings.HasPrefix(target, "/") { + target = target[1:] + } + } + host, portStr, err := net.SplitHostPort(target) + if err != nil { + return target, 0 + } + port, err := strconv.Atoi(portStr) + if err != nil { + return host, 0 + } + return host, port +} + +// openTelemetryUnaryClientInterceptor returns an interceptor that populates +// TransportTelemetryData with the server peer address. +func openTelemetryUnaryClientInterceptor() grpc.UnaryClientInterceptor { + return func(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { + transportData := gax.ExtractTransportTelemetry(ctx) + if transportData == nil { + return invoker(ctx, method, req, reply, cc, opts...) + } + + err := invoker(ctx, method, req, reply, cc, opts...) + + if target := cc.Target(); target != "" { + host, port := extractHostPort(target) + transportData.SetServerAddress(host) + transportData.SetServerPort(port) + } + + return err + } +} + // otelHandler is a wrapper around the OpenTelemetry gRPC client handler that // adds custom Google Cloud-specific attributes to spans and metrics. type otelHandler struct { diff --git a/auth/grpctransport/grpctransport_otel_test.go b/auth/grpctransport/grpctransport_otel_test.go index 2edf40a3bd81..f7cfda540620 100644 --- a/auth/grpctransport/grpctransport_otel_test.go +++ b/auth/grpctransport/grpctransport_otel_test.go @@ -845,7 +845,7 @@ func TestHandleRPC_ActionableErrors(t *testing.T) { } } -func TestDial_TracingAndLogging_Combinations(t *testing.T) { +func TestDial_Telemetry_Combinations(t *testing.T) { // Ensure any lingering HTTP/2 connections are closed to avoid goroutine leaks. defer http.DefaultTransport.(*http.Transport).CloseIdleConnections() @@ -867,36 +867,82 @@ func TestDial_TracingAndLogging_Combinations(t *testing.T) { name string logging bool tracing bool + metrics bool wantLog bool wantTracingAttrs bool + wantMetricsAttrs bool }{ { - name: "both disabled", + name: "all disabled", logging: false, tracing: false, + metrics: false, wantLog: false, wantTracingAttrs: false, + wantMetricsAttrs: false, }, { - name: "tracing enabled, logging disabled", + name: "tracing enabled", logging: false, tracing: true, + metrics: false, wantLog: false, wantTracingAttrs: true, + wantMetricsAttrs: false, }, { - name: "tracing disabled, logging enabled", + name: "logging enabled", logging: true, tracing: false, + metrics: false, + wantLog: true, + wantTracingAttrs: false, + wantMetricsAttrs: false, + }, + { + name: "metrics enabled", + logging: false, + tracing: false, + metrics: true, + wantLog: false, + wantTracingAttrs: false, + wantMetricsAttrs: true, + }, + { + name: "tracing and logging enabled", + logging: true, + tracing: true, + metrics: false, wantLog: true, wantTracingAttrs: true, + wantMetricsAttrs: false, }, { - name: "both enabled", + name: "tracing and metrics enabled", + logging: false, + tracing: true, + metrics: true, + wantLog: false, + wantTracingAttrs: true, + wantMetricsAttrs: true, + }, + { + name: "logging and metrics enabled", + logging: true, + tracing: false, + metrics: true, + wantLog: true, + wantTracingAttrs: false, + wantMetricsAttrs: true, + }, + { + name: "all enabled", logging: true, tracing: true, + metrics: true, wantLog: true, wantTracingAttrs: true, + wantMetricsAttrs: true, }, } @@ -916,6 +962,11 @@ func TestDial_TracingAndLogging_Combinations(t *testing.T) { } else { t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_TRACING", "false") } + if tt.metrics { + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_METRICS", "true") + } else { + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_METRICS", "false") + } l, err := net.Listen("tcp", "localhost:0") if err != nil { @@ -951,8 +1002,11 @@ func TestDial_TracingAndLogging_Combinations(t *testing.T) { } defer pool.Close() + data := &gax.TransportTelemetryData{} + ctx := gax.InjectTransportTelemetry(context.Background(), data) + client := echo.NewEchoerClient(pool) - _, _ = client.Echo(context.Background(), &echo.EchoRequest{Message: "hello"}) + _, _ = client.Echo(ctx, &echo.EchoRequest{Message: "hello"}) logOutput := logBuf.String() hasLog := strings.TrimSpace(logOutput) != "" @@ -961,16 +1015,19 @@ func TestDial_TracingAndLogging_Combinations(t *testing.T) { t.Errorf("got log: %v, want: %v\noutput: %s", hasLog, tt.wantLog, logOutput) } - spans := exporter.GetSpans() - if len(spans) != 1 { - t.Fatalf("len(spans) = %d, want 1", len(spans)) + hasMetricsAttrs := data.ServerAddress() != "" + if hasMetricsAttrs != tt.wantMetricsAttrs { + t.Errorf("got metrics attrs: %v, want: %v", hasMetricsAttrs, tt.wantMetricsAttrs) } + spans := exporter.GetSpans() hasTracingAttrs := false - for _, attr := range spans[0].Attributes { - if attr.Key == "gcp.client.version" && attr.Value.AsString() == "1.2.3" { - hasTracingAttrs = true - break + for _, span := range spans { + for _, attr := range span.Attributes { + if attr.Key == "gcp.client.version" && attr.Value.AsString() == "1.2.3" { + hasTracingAttrs = true + break + } } } diff --git a/auth/httptransport/httptransport_otel_test.go b/auth/httptransport/httptransport_otel_test.go index 843c491cfe5b..c3a36e9cd9da 100644 --- a/auth/httptransport/httptransport_otel_test.go +++ b/auth/httptransport/httptransport_otel_test.go @@ -18,6 +18,7 @@ import ( "context" "net/http" "net/http/httptest" + "strconv" "testing" "time" @@ -669,3 +670,196 @@ func TestNewClient_OpenTelemetry_Disabled(t *testing.T) { }) } } + +func TestTelemetryTransport(t *testing.T) { + gax.TestOnlyResetIsFeatureEnabled() + defer gax.TestOnlyResetIsFeatureEnabled() + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_METRICS", "true") + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTeapot) + })) + defer ts.Close() + + ctx := context.Background() + + // 1. Setup the TransportTelemetryData + data := &gax.TransportTelemetryData{} + ctx = gax.InjectTransportTelemetry(ctx, data) + + // 2. Setup the target URL + req, err := http.NewRequestWithContext(ctx, "GET", ts.URL, nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + + // 3. RoundTrip with otelAttributeTransport + base := http.DefaultTransport + trans := &otelAttributeTransport{base: base} + + resp, err := trans.RoundTrip(req) + if err != nil { + t.Fatalf("failed round trip: %v", err) + } + defer resp.Body.Close() + + // 4. Verify the mutated TransportTelemetryData + u, _ := req.URL.Parse(ts.URL) + expectedHost := u.Hostname() + expectedPort, _ := strconv.Atoi(u.Port()) + + if data.ServerAddress() != expectedHost { + t.Errorf("expected ServerAddress to be %q, got %q", expectedHost, data.ServerAddress()) + } + if data.ServerPort() != expectedPort { + t.Errorf("expected ServerPort to be %d, got %d", expectedPort, data.ServerPort()) + } +} + +func TestTelemetryTransport_NoTransportTelemetryData(t *testing.T) { + gax.TestOnlyResetIsFeatureEnabled() + defer gax.TestOnlyResetIsFeatureEnabled() + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_METRICS", "true") + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + ctx := context.Background() // No TransportTelemetryData injected + + req, err := http.NewRequestWithContext(ctx, "GET", ts.URL, nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + + trans := &otelAttributeTransport{base: http.DefaultTransport} + + resp, err := trans.RoundTrip(req) + if err != nil { + t.Fatalf("failed round trip: %v", err) + } + defer resp.Body.Close() + + // Should just succeed without panicking and without trying to mutate non-existent data. +} + +func TestNewClient_TracingAndMetrics_Combinations(t *testing.T) { + // Ensure any lingering HTTP/2 connections are closed to avoid goroutine leaks. + defer http.DefaultTransport.(*http.Transport).CloseIdleConnections() + + exporter := tracetest.NewInMemoryExporter() + tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter)) + defer tp.Shutdown(context.Background()) + + // Restore the global tracer provider after the test to avoid side effects. + defer func(prev oteltrace.TracerProvider) { otel.SetTracerProvider(prev) }(otel.GetTracerProvider()) + otel.SetTracerProvider(tp) + + tests := []struct { + name string + metrics bool + tracing bool + wantMetricsAttrs bool + wantTracingAttrs bool + }{ + { + name: "both disabled", + metrics: false, + tracing: false, + wantMetricsAttrs: false, + wantTracingAttrs: false, + }, + { + name: "tracing enabled, metrics disabled", + metrics: false, + tracing: true, + wantMetricsAttrs: false, + wantTracingAttrs: true, + }, + { + name: "tracing disabled, metrics enabled", + metrics: true, + tracing: false, + wantMetricsAttrs: true, + wantTracingAttrs: false, + }, + { + name: "both enabled", + metrics: true, + tracing: true, + wantMetricsAttrs: true, + wantTracingAttrs: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exporter.Reset() + gax.TestOnlyResetIsFeatureEnabled() + defer gax.TestOnlyResetIsFeatureEnabled() + + if tt.metrics { + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_METRICS", "true") + } else { + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_METRICS", "false") + } + if tt.tracing { + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_TRACING", "true") + } else { + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_TRACING", "false") + } + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + opts := &Options{ + DisableAuthentication: true, + InternalOptions: &InternalOptions{ + TelemetryAttributes: map[string]string{ + "gcp.client.version": "1.2.3", + }, + }, + } + + client, err := NewClient(opts) + if err != nil { + t.Fatalf("NewClient() = %v, want nil", err) + } + + data := &gax.TransportTelemetryData{} + ctx := gax.InjectTransportTelemetry(context.Background(), data) + req, err := http.NewRequestWithContext(ctx, "GET", ts.URL, nil) + if err != nil { + t.Fatal(err) + } + resp, err := client.Do(req) + if err != nil { + t.Fatalf("client.Do() = %v, want nil", err) + } + resp.Body.Close() + + hasMetricsAttrs := data.ServerAddress() != "" + if hasMetricsAttrs != tt.wantMetricsAttrs { + t.Errorf("got metrics attrs: %v, want: %v", hasMetricsAttrs, tt.wantMetricsAttrs) + } + + spans := exporter.GetSpans() + hasTracingAttrs := false + for _, span := range spans { + for _, attr := range span.Attributes { + if attr.Key == "gcp.client.version" && attr.Value.AsString() == "1.2.3" { + hasTracingAttrs = true + break + } + } + } + + if hasTracingAttrs != tt.wantTracingAttrs { + t.Errorf("got tracing attrs: %v, want: %v", hasTracingAttrs, tt.wantTracingAttrs) + } + }) + } +} diff --git a/auth/httptransport/transport.go b/auth/httptransport/transport.go index 2ece1ea360eb..5caba435c8d5 100644 --- a/auth/httptransport/transport.go +++ b/auth/httptransport/transport.go @@ -21,6 +21,7 @@ import ( "fmt" "net" "net/http" + "net/http/httptrace" "os" "strconv" "time" @@ -180,6 +181,9 @@ func addOpenTelemetryTransport(trans http.RoundTripper, opts *Options) http.Roun if opts.DisableTelemetry { return trans } + if gax.IsFeatureEnabled("METRICS") || gax.IsFeatureEnabled("TRACING") { + trans = &otelAttributeTransport{base: trans} + } if !gax.IsFeatureEnabled("TRACING") { return otelhttp.NewTransport(trans) } @@ -190,9 +194,7 @@ func addOpenTelemetryTransport(trans http.RoundTripper, opts *Options) http.Roun otelOpts := []otelhttp.Option{ otelhttp.WithSpanOptions(trace.WithAttributes(staticAttrs...)), } - return otelhttp.NewTransport(&otelAttributeTransport{ - base: trans, - }, otelOpts...) + return otelhttp.NewTransport(trans, otelOpts...) } // otelAttributeTransport is a wrapper around an http.RoundTripper that adds @@ -205,28 +207,52 @@ type otelAttributeTransport struct { // OpenTelemetry span with static and dynamic attributes, as well as detailed // error information. func (t *otelAttributeTransport) RoundTrip(req *http.Request) (*http.Response, error) { - span := trace.SpanFromContext(req.Context()) - if span.IsRecording() { - var attrs []attribute.KeyValue - attrs = append(attrs, attribute.String("rpc.system.name", "http")) - if resName, ok := callctx.TelemetryFromContext(req.Context(), "resource_name"); ok { - attrs = append(attrs, attribute.String("gcp.resource.destination.id", resName)) - } - if resendCountStr, ok := callctx.TelemetryFromContext(req.Context(), "resend_count"); ok { - if count, err := strconv.Atoi(resendCountStr); err == nil { - attrs = append(attrs, attribute.Int("http.request.resend_count", count)) + var span trace.Span + if gax.IsFeatureEnabled("TRACING") { + span = trace.SpanFromContext(req.Context()) + if span.IsRecording() { + var attrs []attribute.KeyValue + attrs = append(attrs, attribute.String("rpc.system.name", "http")) + if resName, ok := callctx.TelemetryFromContext(req.Context(), "resource_name"); ok { + attrs = append(attrs, attribute.String("gcp.resource.destination.id", resName)) + } + if resendCountStr, ok := callctx.TelemetryFromContext(req.Context(), "resend_count"); ok { + if count, err := strconv.Atoi(resendCountStr); err == nil { + attrs = append(attrs, attribute.Int("http.request.resend_count", count)) + } } + if urlTemplate, ok := callctx.TelemetryFromContext(req.Context(), "url_template"); ok { + attrs = append(attrs, attribute.String("url.template", urlTemplate)) + span.SetName(fmt.Sprintf("%s %s", req.Method, urlTemplate)) + } + span.SetAttributes(attrs...) } - if urlTemplate, ok := callctx.TelemetryFromContext(req.Context(), "url_template"); ok { - attrs = append(attrs, attribute.String("url.template", urlTemplate)) - span.SetName(fmt.Sprintf("%s %s", req.Method, urlTemplate)) + } + + var data *gax.TransportTelemetryData + if gax.IsFeatureEnabled("METRICS") { + data = gax.ExtractTransportTelemetry(req.Context()) + if data != nil { + traceHook := &httptrace.ClientTrace{ + GotConn: func(info httptrace.GotConnInfo) { + if info.Conn != nil && info.Conn.RemoteAddr() != nil { + host, portStr, err := net.SplitHostPort(info.Conn.RemoteAddr().String()) + if err == nil { + data.SetServerAddress(host) + if port, pErr := strconv.Atoi(portStr); pErr == nil { + data.SetServerPort(port) + } + } + } + }, + } + req = req.WithContext(httptrace.WithClientTrace(req.Context(), traceHook)) } - span.SetAttributes(attrs...) } resp, err := t.base.RoundTrip(req) - if span.IsRecording() { + if gax.IsFeatureEnabled("TRACING") && span != nil && span.IsRecording() { if err != nil { var errorType string switch { @@ -254,6 +280,13 @@ func (t *otelAttributeTransport) RoundTrip(req *http.Request) (*http.Response, e } } + if data != nil && data.ServerAddress() == "" && req.URL != nil { + data.SetServerAddress(req.URL.Hostname()) + if port, pErr := strconv.Atoi(req.URL.Port()); pErr == nil { + data.SetServerPort(port) + } + } + return resp, err } From 412a499918ade0f66cbf57f057fe55cb3eabcd20 Mon Sep 17 00:00:00 2001 From: Wes Tarle Date: Tue, 31 Mar 2026 23:38:40 -0400 Subject: [PATCH 2/4] fix: address code review feedback for transport telemetry - Avoid expensive net.SplitHostPort per-RPC in gRPC interceptor - Extract HTTP host and port directly from req.URL instead of physical connection - Add scheme-based fallback for implicit HTTP ports - Add implicit port extraction tests --- auth/grpctransport/grpctransport.go | 26 +++++------ auth/grpctransport/grpctransport_otel_test.go | 29 ++++++++++++ auth/httptransport/httptransport_otel_test.go | 44 +++++++++++++++++++ auth/httptransport/transport.go | 37 +++++++--------- 4 files changed, 101 insertions(+), 35 deletions(-) diff --git a/auth/grpctransport/grpctransport.go b/auth/grpctransport/grpctransport.go index 94bf5bf5c988..1ee72d4105da 100644 --- a/auth/grpctransport/grpctransport.go +++ b/auth/grpctransport/grpctransport.go @@ -370,7 +370,7 @@ func dial(ctx context.Context, secure bool, opts *Options) (*grpc.ClientConn, er // Add tracing, but before the other options, so that clients can override the // gRPC stats handler. // This assumes that gRPC options are processed in order, left to right. - grpcOpts = addOpenTelemetryStatsHandler(grpcOpts, opts) + grpcOpts = addOpenTelemetryStatsHandler(grpcOpts, opts, transportCreds.Endpoint) grpcOpts = append(grpcOpts, opts.GRPCDialOpts...) return grpc.DialContext(ctx, transportCreds.Endpoint, grpcOpts...) @@ -458,12 +458,13 @@ func (c *grpcCredentialsProvider) RequireTransportSecurity() bool { return c.secure } -func addOpenTelemetryStatsHandler(dialOpts []grpc.DialOption, opts *Options) []grpc.DialOption { +func addOpenTelemetryStatsHandler(dialOpts []grpc.DialOption, opts *Options, endpoint string) []grpc.DialOption { if opts.DisableTelemetry { return dialOpts } if gax.IsFeatureEnabled("METRICS") { - dialOpts = append(dialOpts, grpc.WithChainUnaryInterceptor(openTelemetryUnaryClientInterceptor())) + host, port := extractHostPort(endpoint) + dialOpts = append(dialOpts, grpc.WithChainUnaryInterceptor(openTelemetryUnaryClientInterceptor(host, port))) } if !gax.IsFeatureEnabled("TRACING") && !gax.IsFeatureEnabled("LOGGING") { return append(dialOpts, grpc.WithStatsHandler(otelgrpc.NewClientHandler())) @@ -494,7 +495,7 @@ func addOpenTelemetryStatsHandler(dialOpts []grpc.DialOption, opts *Options) []g func extractHostPort(target string) (string, int) { if idx := strings.Index(target, "://"); idx != -1 { target = target[idx+3:] - // Ensure any trailing slashes from the scheme suffix are stripped + // Ensure any leading slashes from the scheme suffix are stripped for strings.HasPrefix(target, "/") { target = target[1:] } @@ -512,21 +513,20 @@ func extractHostPort(target string) (string, int) { // openTelemetryUnaryClientInterceptor returns an interceptor that populates // TransportTelemetryData with the server peer address. -func openTelemetryUnaryClientInterceptor() grpc.UnaryClientInterceptor { +func openTelemetryUnaryClientInterceptor(host string, port int) grpc.UnaryClientInterceptor { return func(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { transportData := gax.ExtractTransportTelemetry(ctx) - if transportData == nil { - return invoker(ctx, method, req, reply, cc, opts...) + if transportData != nil { + if host != "" { + transportData.SetServerAddress(host) + } + if port != 0 { + transportData.SetServerPort(port) + } } err := invoker(ctx, method, req, reply, cc, opts...) - if target := cc.Target(); target != "" { - host, port := extractHostPort(target) - transportData.SetServerAddress(host) - transportData.SetServerPort(port) - } - return err } } diff --git a/auth/grpctransport/grpctransport_otel_test.go b/auth/grpctransport/grpctransport_otel_test.go index f7cfda540620..c3fe034cf93b 100644 --- a/auth/grpctransport/grpctransport_otel_test.go +++ b/auth/grpctransport/grpctransport_otel_test.go @@ -1051,3 +1051,32 @@ func (m *mockStatsHandler) TagConn(ctx context.Context, info *stats.ConnTagInfo) } func (m *mockStatsHandler) HandleConn(ctx context.Context, cs stats.ConnStats) {} + +func TestExtractHostPort(t *testing.T) { + tests := []struct { + target string + wantHost string + wantPort int + }{ + {"localhost:8080", "localhost", 8080}, + {"[::1]:443", "::1", 443}, + {"google.com", "google.com", 0}, + {"dns:///localhost:8080", "localhost", 8080}, + {"dns:///google.com:443", "google.com", 443}, + {"xds:///my-service:80", "my-service", 80}, + {"dns:///[::1]:8080", "::1", 8080}, + {"google.com:foo", "google.com", 0}, + } + + for _, tt := range tests { + t.Run(tt.target, func(t *testing.T) { + gotHost, gotPort := extractHostPort(tt.target) + if gotHost != tt.wantHost { + t.Errorf("extractHostPort(%q) host = %q, want %q", tt.target, gotHost, tt.wantHost) + } + if gotPort != tt.wantPort { + t.Errorf("extractHostPort(%q) port = %v, want %v", tt.target, gotPort, tt.wantPort) + } + }) + } +} diff --git a/auth/httptransport/httptransport_otel_test.go b/auth/httptransport/httptransport_otel_test.go index c3a36e9cd9da..8893c4c1289d 100644 --- a/auth/httptransport/httptransport_otel_test.go +++ b/auth/httptransport/httptransport_otel_test.go @@ -863,3 +863,47 @@ func TestNewClient_TracingAndMetrics_Combinations(t *testing.T) { }) } } + +func TestTelemetryTransport_ImplicitPort(t *testing.T) { + gax.TestOnlyResetIsFeatureEnabled() + defer gax.TestOnlyResetIsFeatureEnabled() + t.Setenv("GOOGLE_SDK_GO_EXPERIMENTAL_METRICS", "true") + + tests := []struct { + urlStr string + expectedPort int + }{ + {"http://example.com/foo", 80}, + {"https://example.com/bar", 443}, + {"http://example.com:8080/baz", 8080}, + } + + for _, tt := range tests { + t.Run(tt.urlStr, func(t *testing.T) { + ctx := context.Background() + data := &gax.TransportTelemetryData{} + ctx = gax.InjectTransportTelemetry(ctx, data) + + req, err := http.NewRequestWithContext(ctx, "GET", tt.urlStr, nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + + // we just want to test the transport round trip parsing, we mock the base roundtripper + base := &mockRoundTripper{} + trans := &otelAttributeTransport{base: base} + + _, _ = trans.RoundTrip(req) + + if data.ServerPort() != tt.expectedPort { + t.Errorf("for url %q, expected ServerPort to be %d, got %d", tt.urlStr, tt.expectedPort, data.ServerPort()) + } + }) + } +} + +type mockRoundTripper struct{} + +func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: 200}, nil +} diff --git a/auth/httptransport/transport.go b/auth/httptransport/transport.go index 5caba435c8d5..cda1447f9a90 100644 --- a/auth/httptransport/transport.go +++ b/auth/httptransport/transport.go @@ -21,7 +21,6 @@ import ( "fmt" "net" "net/http" - "net/http/httptrace" "os" "strconv" "time" @@ -232,21 +231,22 @@ func (t *otelAttributeTransport) RoundTrip(req *http.Request) (*http.Response, e var data *gax.TransportTelemetryData if gax.IsFeatureEnabled("METRICS") { data = gax.ExtractTransportTelemetry(req.Context()) - if data != nil { - traceHook := &httptrace.ClientTrace{ - GotConn: func(info httptrace.GotConnInfo) { - if info.Conn != nil && info.Conn.RemoteAddr() != nil { - host, portStr, err := net.SplitHostPort(info.Conn.RemoteAddr().String()) - if err == nil { - data.SetServerAddress(host) - if port, pErr := strconv.Atoi(portStr); pErr == nil { - data.SetServerPort(port) - } - } - } - }, + if data != nil && req.URL != nil { + host := req.URL.Hostname() + if host != "" { + data.SetServerAddress(host) + } + portStr := req.URL.Port() + if portStr == "" { + if req.URL.Scheme == "https" { + portStr = "443" + } else if req.URL.Scheme == "http" { + portStr = "80" + } + } + if port, pErr := strconv.Atoi(portStr); pErr == nil { + data.SetServerPort(port) } - req = req.WithContext(httptrace.WithClientTrace(req.Context(), traceHook)) } } @@ -280,13 +280,6 @@ func (t *otelAttributeTransport) RoundTrip(req *http.Request) (*http.Response, e } } - if data != nil && data.ServerAddress() == "" && req.URL != nil { - data.SetServerAddress(req.URL.Hostname()) - if port, pErr := strconv.Atoi(req.URL.Port()); pErr == nil { - data.SetServerPort(port) - } - } - return resp, err } From 2d8ecf69d0d4024d16fb92748b0af1d794f3c9a6 Mon Sep 17 00:00:00 2001 From: Wes Tarle Date: Wed, 1 Apr 2026 11:59:34 -0400 Subject: [PATCH 3/4] fix: use strings.TrimLeft for stripping leading slashes --- auth/grpctransport/grpctransport.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/auth/grpctransport/grpctransport.go b/auth/grpctransport/grpctransport.go index 1ee72d4105da..6e331fa4e71e 100644 --- a/auth/grpctransport/grpctransport.go +++ b/auth/grpctransport/grpctransport.go @@ -496,9 +496,7 @@ func extractHostPort(target string) (string, int) { if idx := strings.Index(target, "://"); idx != -1 { target = target[idx+3:] // Ensure any leading slashes from the scheme suffix are stripped - for strings.HasPrefix(target, "/") { - target = target[1:] - } + target = strings.TrimLeft(target, "/") } host, portStr, err := net.SplitHostPort(target) if err != nil { From 9a0e5f90c9fbfed9873587e7486e274a20adfc09 Mon Sep 17 00:00:00 2001 From: Wes Tarle Date: Wed, 1 Apr 2026 16:09:09 -0400 Subject: [PATCH 4/4] fix: safely strip DNS authority when extracting gRPC host --- auth/grpctransport/grpctransport.go | 6 ++++-- auth/grpctransport/grpctransport_otel_test.go | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/auth/grpctransport/grpctransport.go b/auth/grpctransport/grpctransport.go index 6e331fa4e71e..26cc024275b3 100644 --- a/auth/grpctransport/grpctransport.go +++ b/auth/grpctransport/grpctransport.go @@ -495,8 +495,10 @@ func addOpenTelemetryStatsHandler(dialOpts []grpc.DialOption, opts *Options, end func extractHostPort(target string) (string, int) { if idx := strings.Index(target, "://"); idx != -1 { target = target[idx+3:] - // Ensure any leading slashes from the scheme suffix are stripped - target = strings.TrimLeft(target, "/") + // Ensure any leading authorities (like 8.8.8.8 in dns://8.8.8.8/foo) are stripped + if slashIdx := strings.Index(target, "/"); slashIdx != -1 { + target = target[slashIdx+1:] + } } host, portStr, err := net.SplitHostPort(target) if err != nil { diff --git a/auth/grpctransport/grpctransport_otel_test.go b/auth/grpctransport/grpctransport_otel_test.go index c3fe034cf93b..5f4b2d573d23 100644 --- a/auth/grpctransport/grpctransport_otel_test.go +++ b/auth/grpctransport/grpctransport_otel_test.go @@ -1066,6 +1066,7 @@ func TestExtractHostPort(t *testing.T) { {"xds:///my-service:80", "my-service", 80}, {"dns:///[::1]:8080", "::1", 8080}, {"google.com:foo", "google.com", 0}, + {"dns://8.8.8.8/lb.example.com:443", "lb.example.com", 443}, } for _, tt := range tests {